Re: [PATCH] trace-cmd: Improve /proc/kallsyms parsing

[Steven Rostedt <rostedt@goodmis.org>]

On Sat, 23 Nov 2019 17:41:29 +0900
Kusanagi Kouichi <slash@ac.auone-net.jp> wrote:

> - Use the stricter format string.
> - Avoid allocating memory.
> - Check sscanf return value.
> 

Hi Kusanagi,

Thanks for the patch, small comment below.

> Signed-off-by: Kusanagi Kouichi <slash@ac.auone-net.jp>
> ---
>  lib/trace-cmd/trace-util.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-util.c b/lib/trace-cmd/trace-util.c
> index 8aa3b6c..ef8e4f7 100644
> --- a/lib/trace-cmd/trace-util.c
> +++ b/lib/trace-cmd/trace-util.c
> @@ -80,39 +80,40 @@ void tracecmd_parse_proc_kallsyms(struct tep_handle *pevent,
>  	char *func;
>  	char *line;
>  	char *next = NULL;
> -	char *addr_str;
>  	char *mod;
>  	char ch;
>  
>  	line = strtok_r(file, "\n", &next);
>  	while (line) {
> +		int n, func_start, func_end = 0, mod_start, mod_end = 0;

Small nit. I usually prefer to not have so many variables defined on
one line. Something more like this:

		int func_start, func_end = 0;
		int mod_start, mod_end = 0;
		int n;

Just easier to review.

The rest looks good. I'll play with it to see if there's any instances
that it doesn't work.

Thanks!

-- Steve


> +
>  		mod = NULL;
>  		errno = 0;
> -		sscanf(line, "%ms %c %ms\t[%ms",
> -			     &addr_str, &ch, &func, &mod);
> +		n = sscanf(line, "%16llx %c %n%*s%n%*1[\t][%n%*s%n",
> +			   &addr, &ch, &func_start, &func_end, &mod_start, &mod_end);
> 		if (errno) {
> -			free(addr_str);
> -			free(func);
> -			free(mod);
>  			perror("sscanf");
>  			return;
>  		}
> -		addr = strtoull(addr_str, NULL, 16);
> -		free(addr_str);
>  
> -		/* truncate the extra ']' */
> -		if (mod)
> -			mod[strlen(mod) - 1] = 0;
> +		if (n != 2 || !func_end)
> +			return;
>  
> +		func = line + func_start;
>  		/*
>  		 * Hacks for
>  		 *  - arm arch that adds a lot of bogus '$a' functions
>  		 *  - x86-64 that reports per-cpu variable offsets as absolute */
> -		if (func[0] != '$' && ch != 'A' && ch != 'a')
> +		if (func[0] != '$' && ch != 'A' && ch != 'a') {
> +			line[func_end] = 0;
> +			if (mod_end) {
> +				mod = line + mod_start;
> +				/* truncate the extra ']' */
> +				line[mod_end - 1] = 0;
> +			}
>  			tep_register_function(pevent, func, addr, mod);
> -		free(func);
> -		free(mod);
> +		}
>  
>  		line = strtok_r(NULL, "\n", &next);
>  	}