Re: [PATCH v6 1/3] tracing: eprobe: remove duplicate is_good_name() operation

[Tom Zanussi]

Hi Linyu,

On Tue, 2022-06-21 at 09:59 +0800, Linyu Yuan wrote:
> traceprobe_parse_event_name() already validate SYSTEM and EVENT name,
> there is no need to call is_good_name() after it.
> 
> Add trace_probe_log_set_index(1) to allow report correct error
> if user input wrong SYSTEM.EVENT format.
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> ---
> v2: drop v1 change as it is NACK.
>     add it to remove duplicate is_good_name().
> v3: move it as first patch.
> v4: no change
> v5: add Acked-by tag
> v6: keep is_good_name() check for group and event name
>     add trace_probe_log_set_index(1) to report correct error message.
> 
>  kernel/trace/trace_eprobe.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_eprobe.c
> b/kernel/trace/trace_eprobe.c
> index 7d44785..8979cb9e 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -881,13 +881,12 @@ static int __trace_eprobe_create(int argc,
> const char *argv[])
>         if (!is_good_name(event) || !is_good_name(group))
>                 goto parse_error;
>  
> +       trace_probe_log_set_index(1);

Is this something that you noticed missing in the original code and are
adding now?  If so, please make this a separate patch.  Or is this
something that's needed for the new 'generating event name' code?  If
that's the case, please move this to the other patch.

This one should only contain the code related to the duplicate
is_good_name() removal mentioned in the subject.  Or if this really
does belong here, please provide more explanation of why it's needed if
you remove the duplicate is_good_name() code.

Thanks,

Tom

>         sys_event = argv[1];
>         ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> buf2,
>                                           sys_event - argv[1]);
>         if (ret || !sys_name)
>                 goto parse_error;
> -       if (!is_good_name(sys_event) || !is_good_name(sys_name))
> -               goto parse_error;
>  
>         mutex_lock(&event_mutex);
>         event_call = find_and_get_event(sys_name, sys_event);