LKML Archive on
help / color / mirror / Atom feed
From: Paulo Marques <>
To: Rusty Russell <>
Cc: Andrew Morton <>,,,,,
Subject: Re: [PATCH] call sysrq_timer_list_show from a workqueue
Date: Wed, 09 Jan 2008 15:24:40 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

Rusty Russell wrote:
> On Wednesday 09 January 2008 11:21:59 Andrew Morton wrote:
>> [...]
>> And the fact that incoming arg `namebuf' MUST point at a
>> KSYM_NAME_LEN-sized buffer could be better communicated by using a
>> dedicated struct for this, or by giving the arg a type of `char
>> namebuf[KSYM_NAME_LEN]'.  Or by adding a comment. Or by just ignoring
>> me and doing something more useful.
> Or better, rework all the name lookup interfaces, rather than having: 

Yes, there is some rework we can do here....

> struct module *module_text_address(unsigned long addr);
> struct module *__module_text_address(unsigned long addr);
> int is_module_address(unsigned long addr);
> int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> 			char *name, char *module_name, int *exported);
> char *module_address_lookup(unsigned long addr,
> 			    unsigned long *symbolsize,
> 			    unsigned long *offset,
> 			    char **modname,
> 			    char *namebuf);
> int lookup_module_symbol_name(unsigned long addr, char *symname);
> int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
> 			       unsigned long *offset, char *modname, char *name);
> unsigned long module_kallsyms_lookup_name(const char *name);

All of these are somewhat less important, because most users call the 
kallsyms generic functions which will in turn call these functions if 
the symbol isn't global.

So, they should suffer basically the same transformations as the 
kallsyms_ counterparts and can be considered almost "internal" to the 
kallsyms infrastructure.

> unsigned long kallsyms_lookup_name(const char *name);

This one look fine, as there is no duplication in any other function.

> extern int kallsyms_lookup_size_offset(unsigned long addr,
> 				  unsigned long *symbolsize,
> 				  unsigned long *offset);
> const char *kallsyms_lookup(unsigned long addr,
> 			    unsigned long *symbolsize,
> 			    unsigned long *offset,
> 			    char **modname, char *namebuf);
> int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
> 		        unsigned long *offset, char *modname, char *name);
> int lookup_symbol_name(unsigned long addr, char *symname);

These 4 functions can probably be condensed into just one, by allowing 
NULL pointer arguments to mean "don't need this result":

kallsyms_lookup_size_offset(a,s,o) <=> kallsyms_lookup(a,s,o,NULL,NULL)
lookup_symbol_attrs(a,s,o,m,n) <=> kallsyms_lookup(a,s,o,m,n)
lookup_symbol_name(a,n) <=> kallsyms_lookup(a,NULL,NULL,NULL,n)

> extern int sprint_symbol(char *buffer, unsigned long address);
> extern void __print_symbol(const char *fmt, unsigned long address);

These 2 are probably fine.

There is a difference in the way the module name is passed, because 
kallsyms_lookup assumes it can return just a pointer to the module name.

However, we should probably change that interface so that the caller 
provides the buffer to hold the module name, to avoid races. The 
stop_machine should help already, but returning a pointer that can be 
stale just a little bit later isn't pretty anyway.

I can do a patch for this, but this will touch a few subsystems that use 
these interfaces (there are not a lot of them, though). The major change 
would probably be the allocation of a small buffer (56~60 bytes) in some 
of the callers to hold the module name.

Paulo Marques -

"There cannot be a crisis today; my schedule is already full."

  parent reply	other threads:[~2008-01-09 15:24 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-01-08  6:31 [PATCH] call sysrq_timer_list_show from a workqueue Kyle McMartin
2008-01-08  9:17 ` Ingo Molnar
2008-01-08 11:28   ` Rusty Russell
2008-01-08 11:33     ` Ingo Molnar
2008-01-08 11:50       ` Rusty Russell
2008-01-08 15:51         ` Ingo Molnar
2008-01-08 22:48           ` Rusty Russell
2008-01-08 22:52             ` Ingo Molnar
2008-01-09  0:21         ` Andrew Morton
2008-01-09  3:20           ` Rusty Russell
2008-01-09  3:33             ` Andrew Morton
2008-01-09  4:27               ` Rusty Russell
2008-01-09  4:45                 ` Andrew Morton
2008-01-09 15:24             ` Paulo Marques [this message]
2008-01-09 21:58               ` Rusty Russell
2008-01-08 14:41     ` Kyle McMartin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).