lkml.org 
[lkml]   [2008]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] call sysrq_timer_list_show from a workqueue
    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."


    \
     
     \ /
      Last update: 2008-01-09 16:27    [W:5.156 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site