Re: [PATCH] call sysrq_timer_list_show from a workqueue

From: Paulo Marques
Date: Wed Jan 09 2008 - 10:24:51 EST


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."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/