Re: [RFC] kallsyms_lookup always requires buffers

[Paulo Marques <pmarques@grupopie.com>]

Franck Bui-Huu wrote:
> Hi,
> 
> Some uses of kallsyms_lookup() do not need to find out the name of
> a symbol and its module's name it belongs. This is specially true
> in arch specific code, which needs to unwind the stack to show the
> back trace during opps (mips is an example). In this specific case,
> we just need to retreive the function's size and the offset of the
> active intruction inside it.
> 
> This simple patch adds a new entry "kallsyms_lookup_gently()". The
> name actually sucks but I can't figure out a coherent name with the
> rest of the file. If someone could give me a better idea...

kallsyms_lookup_size_offset() ?

Granted it is a little bigger, but it tells exactly what it does and 
there are not that many users that we have to write this name all that 
often.

> This new entry does exactly the same as kallsyms_lookup() but does
> not require any buffers to store any names. It returns 0 if it fails
> otherwise 1.
> 
> Do you think this can be usefull ?

You tell me, since you're proposing the change ;)

> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index 849043c..30f8d8d 100644
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> @@ -12,6 +12,10 @@ #ifdef CONFIG_KALLSYMS
>  /* Lookup the address for a symbol. Returns 0 if not found. */
>  unsigned long kallsyms_lookup_name(const char *name);
>  
> +extern int kallsyms_lookup_gently(unsigned long addr,
> +				  unsigned long *symbolsize,
> +				  unsigned long *offset);
> +
>  /* Lookup an address.  modname is set to NULL if it's in the kernel. */
>  const char *kallsyms_lookup(unsigned long addr,
>  			    unsigned long *symbolsize,
> @@ -28,6 +32,13 @@ static inline unsigned long kallsyms_loo
>  	return 0;
>  }
>  
> +static inline const int kallsyms_lookup_gently(unsigned long addr,
> +					       unsigned long *symbolsize,
> +					       unsigned long *offset)
> +{
> +	return 0;
> +}
> +
>  static inline const char *kallsyms_lookup(unsigned long addr,
>  					  unsigned long *symbolsize,
>  					  unsigned long *offset,
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index ab16a5a..c398753 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -69,6 +69,15 @@ static inline int is_kernel(unsigned lon
>  	return in_gate_area_no_task(addr);
>  }
>  
> +static int is_kernel_addr(unsigned long addr)

Maybe change the name to kallsyms_is_kernel_addr, because this function 
does more things than what the generic name implies.

> +{
> +	if (all_var)
> +		return is_kernel(addr);
> +
> +	return is_kernel_text(addr) || is_kernel_inittext(addr) ||
> +		is_kernel_extratext(addr);
> +}
> +

[...]
> +/*
> + * Lookup an address but don't bother to find any names.
> + */
> +int kallsyms_lookup_gently(unsigned long addr, unsigned long *symbolsize,
> +			   unsigned long *offset)
> +{
> +	int rv;
> +
> +	if (is_kernel_addr(addr))
> +		rv = !!get_symbol_pos(addr, symbolsize, offset);
> +	else
> +		rv = !!module_address_lookup(addr, symbolsize, offset, NULL);
> +
> +	return rv;
> +}

<minor nitpick>

Why not just:

 > if (is_kernel_addr(addr))
 >	return !!get_symbol_pos(addr, symbolsize, offset);
 >
 > return !!module_address_lookup(addr, symbolsize, offset, NULL);

and just get rid of "rv" completely?

</minor nitpick>

> +EXPORT_SYMBOL_GPL(kallsyms_lookup_gently);

I agree with Arjan here. If kallsyms_lookup wasn't exported, I don't see 
a reason to export this either.

Everything else seems fine.

-- 
Paulo Marques - www.grupopie.com

"The face of a child can say it all, especially the
mouth part of the face."