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

[Tom Zanussi]

Hi Linyu,

On Tue, 2022-06-14 at 08:48 +0800, Linyu Yuan wrote:
> hi Tom,
> 
> On 6/14/2022 5:01 AM, Tom Zanussi wrote:
> > Hi Linhu,
> > 
> > On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
> > > traceprobe_parse_event_name() already validate group and event
> > > name,
> > > there is no need to call is_good_name() after it.
> > > 
> > > 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
> > > 
> > >   kernel/trace/trace_eprobe.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_eprobe.c
> > > b/kernel/trace/trace_eprobe.c
> > > index 7d44785..17d64e3 100644
> > > --- a/kernel/trace/trace_eprobe.c
> > > +++ b/kernel/trace/trace_eprobe.c
> > > @@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc,
> > > const char *argv[])
> > >                  sanitize_event_name(buf1);
> > >                  event = buf1;
> > >          }
> > > -       if (!is_good_name(event) || !is_good_name(group))
> > > -               goto parse_error;
> > traceprobe_parse_event_name() is only called if (event).  In the
> > !event case, wouldn't the is_good_name() checks still be needed
> > (since
> > in that case buf1 is assigned to event)?
> 
> when user input no  event name, it will generate event name from
> second  
> SYSTEM.EVENT,
> 
> and it will validate with following traceprobe_parse_event_name().
> 
> 

Right, but that happens in your second patch '[PATCH v5 2/3] tracing:
auto generate event name when create a group of events', not this one.
 
So if you apply only this patch, the !event case will assign event but
it will remain unchecked when used later in this function.

It would make more sense to remove this check in patch 2/3 along with
the code that does the generating...

> (
> 
> if you agree, i will send a new version to update a minor issue in 
> second patch,
> 
> 
>          sys_event = argv[1];
> -       ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> buf2,
> -                                         sys_event - argv[1]);
> -       if (ret || !sys_name)
> +       ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> buf2, 0);
> +       if (!sys_event || !sys_name)
>                  goto parse_error;
> 
> )
> 
> > 
> > >   
> > >          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;
> > I agree this one isn't needed.

But keep this one in this patch, since it's useful on its own as a
standalone cleanup regardless of whether or not patch 2/3 gets merged.

Tom

> > Thanks,
> > 
> > Tom
> > 
> > >   
> > >          mutex_lock(&event_mutex);
> > >          event_call = find_and_get_event(sys_name, sys_event);