Messages in this thread | | | From | Raphaël Beamonte <> | Date | Mon, 14 Sep 2015 16:59:41 -0400 | Subject | Re: [PATCH 4/5] perf tools: Propagate error info from tp_format |
| |
2015-09-14 16:53 GMT-04:00 Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>: > Em Thu, Sep 10, 2015 at 10:24:52AM +0200, Jiri Olsa escreveu: >> On Wed, Sep 09, 2015 at 05:58:13PM -0300, Arnaldo Carvalho de Melo wrote: >> >> SNIP >> >> > This kind of stuff is ok, as evsel is a local variable and you kept the >> > interface for perf_evsel__syscall_newtp(), i.e. it returns NULL if a new >> > evsel can't be instantiated. >> > >> > Ok, but that is a different interface than the one used by >> > perf_evsel__newtp(), that also instantiates a new evsel. >> > >> > So when one thinks about "foo__new()" we now need to check which one of >> > the two interfaces it uses, if err.h or if the old NULL based failure >> > reporting one. >> > >> > Double tricky if it is foo__new() and foo__new_variant(), as >> > perf_evsel__syscall_newtp() and perf_evsel__newtp(), i.e. both will >> > return a "struct perf_evsel" instance, but one using err.h, the other >> > use NULL. >> > >> > Ok, you marked the ones using a comment, wonder if we couldn't use >> > 'sparse' somehow here, is it used to check IS_ERR() usage in the kernel? >> >> hum, not sure.. will check ;-) >> >> at least we could mark related functions with __must_check >> to force the return value check >> >> > >> > Ah, but what about this in trace__event_handler() in builtin-trace.c? >> > >> > if (evsel->tp_format) { >> > event_format__fprintf(evsel->tp_format, sample->cpu, >> > sample->raw_data, sample->raw_size, >> > trace->output); >> > } >> > >> > >> > Don't we have to use IS_ERR() here? Ok, no, because if setting up >> > evsel->tp_format fails, then that evsel will be destroyed and >> > perf_evsel__newtp() will return ERR_PTR(), so it is ok not no use >> > ERR_PTR(evsel->tp_format) because it will only be != NULL when it was >> > successfully set up. >> > >> > But then, in perf_evsel__newtp_idx if zalloc() fails we will not return >> > ERR_PTR(), but instead NULL, a-ha, this one seems to be a real bug, no? >> >> hate those allocations in declarations.. never do any good ;-) >> >> yep, NULL is not an error, so it's real bug, attached patch should fix it >> >> thanks, >> jirka > > > Ok continuing, found two more problems in this patch, fixed as follows, > merging. > > - Arnaldo > > diff --git a/tools/perf/tests/mmap-basic.c b/tools/perf/tests/mmap-basic.c > index 666b67a4df9d..4bb0c5d2059d 100644 > --- a/tools/perf/tests/mmap-basic.c > +++ b/tools/perf/tests/mmap-basic.c > @@ -65,7 +65,7 @@ int test__basic_mmap(void) > > snprintf(name, sizeof(name), "sys_enter_%s", syscall_names[i]); > evsels[i] = perf_evsel__newtp("syscalls", name); > - if (evsels[i] == NULL) { > + if (IS_ERR(evsels[i]) == NULL) { > pr_debug("perf_evsel__new\n"); > goto out_delete_evlist; > } > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index 08c20ee4e27d..6b5d1b509148 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -234,7 +234,9 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int > struct perf_evsel *evsel = zalloc(perf_evsel__object.size); > int err = -ENOMEM; > > - if (evsel != NULL) { > + if (evsel == NULL) { > + goto out_err; > + } else {
Is the else really necessary after a goto?
> struct perf_event_attr attr = { > .type = PERF_TYPE_TRACEPOINT, > .sample_type = (PERF_SAMPLE_RAW | PERF_SAMPLE_TIME | > @@ -261,6 +263,7 @@ struct perf_evsel *perf_evsel__newtp_idx(const char *sys, const char *name, int > out_free: > zfree(&evsel->name); > free(evsel); > +out_err: > return ERR_PTR(err); > } > > diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c > index 8e3a60e3e15f..802bb868d446 100644 > --- a/tools/perf/util/trace-event.c > +++ b/tools/perf/util/trace-event.c > @@ -100,7 +100,7 @@ struct event_format* > trace_event__tp_format(const char *sys, const char *name) > { > if (!tevent_initialized && trace_event__init2()) > - return NULL; > + return ERR_PTR(-ENOMEM); > > return tp_format(sys, name); > }
| |