On Thu, Jun 06, 2019 at 10:34:24AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jun 06, 2019 at 10:30:19AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Jun 06, 2019 at 05:48:42PM +0800, Leo Yan escreveu:
> > > +++ b/tools/perf/builtin-trace.c
> > > @@ -3664,6 +3664,14 @@ static int trace__config(const char *var, const 
> > > char *value, void *arg)
> > >                                          "event selector. use 'perf list' 
> > > to list available events",
> > >                                          parse_events_option);
> > >           err = parse_events_option(&o, value, 0);
> > > +
> > > +         /*
> > > +          * When parse option successfully parse_events_option() will
> > > +          * return 0, otherwise means the paring failure.  And it
> > > +          * returns 1 for eBPF program building failure; so adjust the
> > > +          * err value to -1 for the failure.
> > > +          */
> > > +         err = err ? -1 : 0;
> > 
> > I'll rewrite the comment above to make it more succint and fix things
> > like 'paring' (parsing):
> > 
> >             /*
> >              * parse_events_option() returns !0 to indicate failure
> >              * while the perf_config code that calls trace__config()
> >              * expects < 0 returns to indicate error, so:
> >              */
> > 
> >              if (err)
> >                     err = -1;
> 
> Even shorter, please let me know if I can keep your
> Signed-off-by/authorship for this one.

Sorry I miss this email.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index f7e4e50bddbd..1a2a605cf068 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3703,7 +3703,12 @@ static int trace__config(const char *var, const char 
> *value, void *arg)
>               struct option o = OPT_CALLBACK('e', "event", &trace->evlist, 
> "event",
>                                              "event selector. use 'perf list' 
> to list available events",
>                                              parse_events_option);
> -             err = parse_events_option(&o, value, 0);
> +             /*
> +              * We can't propagate parse_event_option() return, as it is 1
> +              * for failure while perf_config() expects -1.
> +              */
> +             if (parse_events_option(&o, value, 0))
> +                     err = -1;
>       } else if (!strcmp(var, "trace.show_timestamp")) {
>               trace->show_tstamp = perf_config_bool(var, value);
>       } else if (!strcmp(var, "trace.show_duration")) {


Yeah, the change looks good to me. And very appreciate your effort to
improve the patch quality.

Thanks,
Leo.

Reply via email to