Re: [PATCH] trace-cmd record: Fix syntax in make_pid_filter

[Will Hawkins]

On Fri, Oct 13, 2017 at 2:24 PM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Fri, 13 Oct 2017 13:57:12 -0400
> Will Hawkins <hawkinsw@xxxxxxxxxxxxx> wrote:
>
> > In older versions of the kernel, event filtering by
> > pid does not use the set_event_pid file. Instead,
> > ftrace uses a common filter (created in make_pid_filter)
> > to filter events based on PID.
> >
> > The syntax of the filter generated by this function is
> > overly permissive. When filtering by pid, || is used
> > where && should be (in certain cases) which means that
> > unrelated events are captured.
> >
>
> Thanks for sending this.
>
> > Signed-off-by: Will Hawkins <hawkinsw@xxxxxxxxxxxxx>
> > ---
> >  trace-record.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/trace-record.c b/trace-record.c
> > index a0b6541..d9c2959 100644
> > --- a/trace-record.c
> > +++ b/trace-record.c
> > @@ -935,14 +935,19 @@ static char *make_pid_filter(char *curr_filter, const char *field)
> >       str = filter + curr_len;
> >
> >       for (p = filter_pids; p; p = p->next) {
> > -             if (p == filter_pids)
> > -                     orit = "";
> > -             else
> > -                     orit = "||";
> > -             if (p->exclude)
> > +             if (p->exclude) {
> >                       match = "!=";
> > -             else
> > +                     if (p == filter_pids)
> > +                             orit = "";
> > +                     else
> > +                             orit = "&&";
> > +             } else {
> >                       match = "==";
> > +                     if (p == filter_pids)
> > +                             orit = "";
> > +                     else
> > +                             orit = "||";
> > +             }
>
> A little nicer way of doing this is:
>
>
>         if (p->exclude) {
>                 match = "!=";
>                 orit = "&&";
>         } else {
>                 match = "==";
>                 orit = "||";
>         }
>         if (p == filter_pids)
>                 orit = "";
>
> Care to send a v2?

Absolutely! On its way now. Sorry for not having cleaned it up
properly. I was going for the quick-and-dirty. Never thought about
collapsing the cases!

Will

>
> -- Steve
>
> >               len = sprintf(str, "%s(%s%s%d)", orit, field, match, p->pid);
> >               str += len;
> >       }
>