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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 Jun 2019 15:38:25 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Mike Leach <mike.leach@...aro.org>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH v2 1/4] perf trace: Exit when build eBPF program failure

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.