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.