Date: Fri, 17 Mar 2017 18:05:54 +0900
From: Masami Hiramatsu <mhiramat@kernel.org>
To: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
Cc: mingo@redhat.com, acme@kernel.org, brendan.d.gregg@gmail.com,
        peterz@infradead.org, alexander.shishkin@linux.intel.com,
        wangnan0@huawei.com, jolsa@kernel.org, ak@linux.intel.com,
        treeze.taeung@gmail.com, mathieu.poirier@linaro.org,
        hekuang@huawei.com, sukadev@linux.vnet.ibm.com, ananth@in.ibm.com,
        naveen.n.rao@linux.vnet.ibm.com, adrian.hunter@intel.com,
        linux-kernel@vger.kernel.org, hemant@linux.vnet.ibm.com
Subject: Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf
 record'
Message-Id: <20170317180554.c6b69de1e655eee60e723dbd@kernel.org>
In-Reply-To: <20170314150658.7065-4-ravi.bangoria@linux.vnet.ibm.com>
References: <20170314150658.7065-1-ravi.bangoria@linux.vnet.ibm.com>
        <20170314150658.7065-4-ravi.bangoria@linux.vnet.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3159
Lines: 134

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?

[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@kernel.org>