Re: [for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers

[Masami Hiramatsu]

On Sat, 22 Dec 2018 11:20:12 -0500
Steven Rostedt <rost...@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rost...@goodmis.org>
> 
> There are several locations that compare constants to the beginning of
> string variables to determine what commands should be done, then the
> constant length is used to index into the string. This is error prone as the
> hard coded numbers have to match the size of the constants. Instead, use the
> len returned from str_has_prefix() and remove the open coded string length
> sizes.

Looks good to me.

Acked-by: Masami Hiramatsu <mhira...@kernel.org>

for trace_probe part.

Thanks!

> 
> Cc: Joe Perches <j...@perches.com>
> Cc: Masami  Hiramatsu <mhira...@kernel.org>
> Signed-off-by: Steven Rostedt (VMware) <rost...@goodmis.org>
> ---
>  kernel/trace/trace.c       |  8 +++++---
>  kernel/trace/trace_probe.c | 17 +++++++++--------
>  kernel/trace/trace_stack.c |  6 ++++--
>  3 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index eac2824a18ab..18b86c3974e1 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4408,13 +4408,15 @@ static int trace_set_options(struct trace_array *tr, 
> char *option)
>       int neg = 0;
>       int ret;
>       size_t orig_len = strlen(option);
> +     int len;
>  
>       cmp = strstrip(option);
>  
> -     if (str_has_prefix(cmp, "no")) {
> +     len = str_has_prefix(cmp, "no");
> +     if (len)
>               neg = 1;
> -             cmp += 2;
> -     }
> +
> +     cmp += len;
>  
>       mutex_lock(&trace_types_lock);
>  
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 541375737403..9962cb5da8ac 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -186,19 +186,20 @@ int traceprobe_parse_event_name(const char **pevent, 
> const char **pgroup,
>  static int parse_probe_vars(char *arg, const struct fetch_type *t,
>                           struct fetch_insn *code, unsigned int flags)
>  {
> -     int ret = 0;
>       unsigned long param;
> +     int ret = 0;
> +     int len;
>  
>       if (strcmp(arg, "retval") == 0) {
>               if (flags & TPARG_FL_RETURN)
>                       code->op = FETCH_OP_RETVAL;
>               else
>                       ret = -EINVAL;
> -     } else if (str_has_prefix(arg, "stack")) {
> -             if (arg[5] == '\0') {
> +     } else if ((len = str_has_prefix(arg, "stack"))) {
> +             if (arg[len] == '\0') {
>                       code->op = FETCH_OP_STACKP;
> -             } else if (isdigit(arg[5])) {
> -                     ret = kstrtoul(arg + 5, 10, &param);
> +             } else if (isdigit(arg[len])) {
> +                     ret = kstrtoul(arg + len, 10, &param);
>                       if (ret || ((flags & TPARG_FL_KERNEL) &&
>                                   param > PARAM_MAX_STACK))
>                               ret = -EINVAL;
> @@ -213,10 +214,10 @@ static int parse_probe_vars(char *arg, const struct 
> fetch_type *t,
>  #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
>       } else if (((flags & TPARG_FL_MASK) ==
>                   (TPARG_FL_KERNEL | TPARG_FL_FENTRY)) &&
> -                str_has_prefix(arg, "arg")) {
> -             if (!isdigit(arg[3]))
> +                (len = str_has_prefix(arg, "arg"))) {
> +             if (!isdigit(arg[len]))
>                       return -EINVAL;
> -             ret = kstrtoul(arg + 3, 10, &param);
> +             ret = kstrtoul(arg + len, 10, &param);
>               if (ret || !param || param > PARAM_MAX_STACK)
>                       return -EINVAL;
>               code->op = FETCH_OP_ARG;
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 3641f28c343f..eec648a0d673 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -448,8 +448,10 @@ static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] 
> __initdata;
>  
>  static __init int enable_stacktrace(char *str)
>  {
> -     if (str_has_prefix(str, "_filter="))
> -             strncpy(stack_trace_filter_buf, str+8, COMMAND_LINE_SIZE);
> +     int len;
> +
> +     if ((len = str_has_prefix(str, "_filter=")))
> +             strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE);
>  
>       stack_tracer_enabled = 1;
>       last_stack_tracer_enabled = 1;
> -- 
> 2.19.2
> 
> 


-- 
Masami Hiramatsu <mhira...@kernel.org>