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

[Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>]

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