Re: [EXT] [PATCH 3/8] trace: fix leak with regexp

[David Marchand <david.marchand@redhat.com>]

On Thu, Sep 22, 2022 at 1:00 PM Sunil Kumar Kori <skori@marvell.com> wrote:
> > @@ -210,15 +210,18 @@ rte_trace_regexp(const char *regex, bool enable)
> >               return -EINVAL;
> >
> >       STAILQ_FOREACH(tp, &tp_list, next) {
> > -             if (regexec(&r, tp->name, 0, NULL, 0) == 0) {
> > -                     if (enable)
> > -                             rc = rte_trace_point_enable(tp->handle);
> > -                     else
> > -                             rc = rte_trace_point_disable(tp->handle);
> > -                     found = 1;
> > +             if (regexec(&r, tp->name, 0, NULL, 0) != 0)
> > +                     continue;
> > +
> > +             if (enable)
> > +                     rc = rte_trace_point_enable(tp->handle);
> > +             else
> > +                     rc = rte_trace_point_disable(tp->handle);
> > +             if (rc < 0) {
> > +                     found = 0;
> > +                     break;
> >               }
> > -             if (rc < 0)
> > -                     return rc;
> > +             found = 1;
> >       }
> >       regfree(&r);
> >
> > --
>
> I understand the problem addressed by this fix but may be following changes will be sufficient to fix it.
> Please highlight, If I am missing. Just trying to reduce the line of changes.
>
> @@ -220,8 +220,10 @@ rte_trace_regexp(const char *regex, bool enable)
>                                 rc = rte_trace_point_disable(tp->handle);
>                         found = 1;
>                 }
> -               if (rc < 0)
> -                       return rc;
> +               if (rc < 0) {
> +                       found = 0;
> +                       break;
> +               }
>         }


rc is compared against 0 for all non-matching tracepoints.
This is unnecessary and fragile.

I can split this change in two: one for the fix, and one other where
the loop is updated with a continue and an inverted matching condition
like above.
If going like this, I would update rte_trace_pattern() too, in the second patch.

WDYT?


-- 
David Marchand