Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record'

From: Masami Hiramatsu
Date: Fri Mar 17 2017 - 05:16:34 EST


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@xxxxxxxxxxxxxxxxxx> 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?

[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.

> +
> +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;

> +
> + 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,

--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>