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.bango...@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,
>
