On Sat, 23 Nov 2019 17:41:29 +0900
Kusanagi Kouichi <slash@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
> 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);
> }
|