Re: [PATCH] call sysrq_timer_list_show from a workqueue

[Paulo Marques <pmarques@grupopie.com>]

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 - www.grupopie.com

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