Messages in this thread Patch in this message | | | Date | Mon, 14 Sep 2015 17:53:03 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH 4/5] perf tools: Propagate error info from tp_format |
| |
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 { 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); }
| |