Messages in this thread | | | Subject | Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record' | From | Ravi Bangoria <> | Date | Mon, 20 Mar 2017 09:21:58 +0530 |
| |
Thanks Masami for detailed review.
Please see my comments below.
On Friday 17 March 2017 02:35 PM, Masami Hiramatsu wrote: > Hi Ravi, > > (I avoided to review parser part since it may go to yacc in next version) > > On Tue, 14 Mar 2017 20:36:54 +0530 > Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote: > > [SNIP] >> @@ -1516,9 +1534,10 @@ static bool dry_run; >> * using pipes, etc. >> */ >> static struct option __record_options[] = { >> - OPT_CALLBACK('e', "event", &record.evlist, "event", >> - "event selector. use 'perf list' to list available events", >> - parse_events_option), >> + OPT_CALLBACK_ARG('e', "event", &record.evlist, >> + &record.sdt_event_list, "event", >> + "event selector. use 'perf list' to list available events", >> + record__parse_events_option), > Does --event option NOT requires argument without this patch? > If it should be changed to use OPT_CALLBACK_ARG(), would it be > better merge this part to previous patch?
Ok. Yes, it does. I think macro name is confusing.
This new macro allows passing of extra data from builtin-*.c to libelf. One such macro already exists (OPT_CALLBACK_OPTARG), but the argument is optional for it and thus it ignores the argument. I need a macro in which argument is necessary and it also allows to pass extra data. Hence, I introduced this macro.
Will change macro to OPT_CALLBACK_ARGDATA. Please suggest if you have better name.
> [SNIP] >> +/* >> + * Delete the SDT events from uprobe_events file that >> + * were created initially. >> + */ >> +void remove_sdt_event_list(struct list_head *sdt_events) >> +{ >> + struct sdt_event_list *sdt_event; >> + struct strfilter *filter = NULL; >> + const char *err = NULL; >> + >> + if (list_empty(sdt_events)) >> + return; >> + >> + list_for_each_entry(sdt_event, sdt_events, list) { >> + if (!filter) { >> + filter = strfilter__new(sdt_event->name, &err); >> + if (!filter) >> + goto free_list; > Don't we need to return error code for this case? > >> + } else { >> + strfilter__or(filter, sdt_event->name, &err); > strfilter__or() can fail here. > >> + } >> + } >> + >> + del_perf_probe_events(filter); > Here too, if it is ignored silently by design, please comment it here.
Sure. Will think about handling errors in this function.
> >> + >> +free_list: >> + free_sdt_list(sdt_events); >> +} >> + >> +static int get_sdt_events_from_cache(struct perf_probe_event *pev) >> +{ >> + int ret = 0; >> + >> + pev->ntevs = find_cached_events_all(pev, &pev->tevs); >> + >> + if (pev->ntevs < 0) { >> + pr_err("Error: Cache lookup failed (code: %d)\n", pev->ntevs); >> + ret = pev->ntevs; >> + } else if (!pev->ntevs) { >> + pr_err("Error: %s:%s not found in the cache\n", >> + pev->group, pev->event); >> + ret = -EINVAL; >> + } else if (pev->ntevs > 1) { >> + pr_warning("Warning : Recording on %d occurences of %s:%s\n", >> + pev->ntevs, pev->group, pev->event); >> + } >> + >> + return ret; >> +} >> + >> +static int add_event_to_sdt_evlist(struct probe_trace_event *tev, >> + struct list_head *sdt_evlist) >> +{ >> + struct sdt_event_list *tmp; > Well, strbuf can make this simpler as below ;-) > > struct strbuf buf = STRBUF_INIT;
Sure, will do it.
Thanks :) Ravi
>> + >> + tmp = zalloc(sizeof(*tmp)); >> + if (!tmp) >> + return -ENOMEM; >> + >> + INIT_LIST_HEAD(&tmp->list); > if (strbuf_addf(&buf, "%s:%s", tev->group, tev->event)) > goto error; > > tmp->name = strbuf_detach(&buf); > >> + list_add(&tmp->list, sdt_evlist); >> + >> + return 0; > error: > free(tmp); > > return -ENOMEM; >> +} >> + >> +static int add_events_to_sdt_evlist(struct perf_probe_event *pev, >> + struct list_head *sdt_evlist) >> +{ >> + int i, ret; >> + >> + for (i = 0; i < pev->ntevs; i++) { >> + ret = add_event_to_sdt_evlist(&pev->tevs[i], sdt_evlist); >> + >> + if (ret < 0) >> + return ret; >> + } >> + return 0; >> +} > > Thanks, >
| |