Re: [PATCH v2 1/4] perf trace: Exit when build eBPF program failure

[Leo Yan]

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.