Re: [PATCH 2.6.21-rc1] Extend print_symbol capability

[Paulo Marques]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/