lkml.org 
[lkml]   [2007]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2.6.21-rc1] Extend print_symbol capability
Robert Peterson wrote:
> [...]
> #define KSYM_NAME_LEN 127
> +#define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + KSYM_NAME_LEN + \
> + 2*(BITS_PER_LONG*3/10) + MODULE_NAME_LEN + 1)
>
> #ifdef CONFIG_KALLSYMS
> /* Lookup the address for a symbol. Returns 0 if not found. */
> @@ -22,6 +24,9 @@ const char *kallsyms_lookup(unsigned long addr,
> unsigned long *offset,
> char **modname, char *namebuf);
>
> +/* Look up a kernel symbol and return it in a text buffer. */
> +extern void lookup_symbol(unsigned long addr, char *buffer);

I don't like this name much :(

We already have kallsyms_lookup and kallsyms_lookup_name. The name of
this function should imply that it will print the formatted result into
the buffer, not just lookup a symbol.

Maybe "__sprint_symbol", and change the interface to
"__sprint_symbol(char *buffer, unsigned long addr)"?

> +
> /* Replace "%s" in format with address, if found */
> extern void __print_symbol(const char *fmt, unsigned long address);
>
> @@ -47,6 +52,11 @@ static inline const char *kallsyms_lookup(unsigned
> long addr,
> return NULL;
> }
>
> +static inline void lookup_symbol(unsigned long addr, char *buffer)
> +{
> + return NULL;
> +}

Returning NULL in a function returning "void" doesn't seem right :P

Maybe it should be something like this instead:
{
*buffer = '\0';
}

> [...]

Anyway, the change looks useful, so thanks for the patch :)

--
Paulo Marques - www.grupopie.com

"Very funny Scotty. Now beam up my clothes."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-03-02 17:13    [W:0.055 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site