Date: Fri, 13 Sep 2013 16:01:00 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: Tom Zanussi <tom.zanussi@linux.intel.com>
Cc: masami.hiramatsu.pt@hitachi.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v9 04/10] tracing: Add 'snapshot' event trigger command
Message-ID: <20130913160100.30686d60@gandalf.local.home>
In-Reply-To: <c1245b886d62b5154f20480c63d41a368af7cffa.1378565610.git.tom.zanussi@linux.intel.com>
References: <cover.1378565610.git.tom.zanussi@linux.intel.com>
	<c1245b886d62b5154f20480c63d41a368af7cffa.1378565610.git.tom.zanussi@linux.intel.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: 3079
Lines: 113

On Sat,  7 Sep 2013 10:29:00 -0500
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:


> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -696,6 +696,74 @@ static struct event_command trigger_traceoff_cmd = {
>  	.get_trigger_ops	= onoff_get_trigger_ops,
>  };
>  
> +static void
> +snapshot_trigger(struct event_trigger_data *data)
> +{
> +	tracing_snapshot();
> +}

If CONFIG_TRACER_SNAPSHOT is not defined, then we should not bother
implementing the snapshot trigger. This should be encapsulated around
ifdefs.

> +
> +static void
> +snapshot_count_trigger(struct event_trigger_data *data)
> +{
> +	if (!data->count)
> +		return;
> +
> +	if (data->count != -1)
> +		(data->count)--;
> +
> +	snapshot_trigger(data);
> +}
> +
> +static int
> +register_snapshot_trigger(char *glob, struct event_trigger_ops *ops,
> +			  struct event_trigger_data *data,
> +			  struct ftrace_event_file *file)
> +{
> +	int ret = register_trigger(glob, ops, data, file);
> +
> +	if (ret > 0)
> +		ftrace_alloc_snapshot();
> +
> +	return ret;
> +}
> +
> +static int
> +snapshot_trigger_print(struct seq_file *m, struct event_trigger_ops *ops,
> +		       struct event_trigger_data *data)
> +{
> +	return event_trigger_print("snapshot", m, (void *)data->count,
> +				   data->filter_str);
> +}
> +
> +static struct event_trigger_ops snapshot_trigger_ops = {
> +	.func			= snapshot_trigger,
> +	.print			= snapshot_trigger_print,
> +	.init			= event_trigger_init,
> +	.free			= event_trigger_free,
> +};
> +
> +static struct event_trigger_ops snapshot_count_trigger_ops = {
> +	.func			= snapshot_count_trigger,
> +	.print			= snapshot_trigger_print,
> +	.init			= event_trigger_init,
> +	.free			= event_trigger_free,
> +};
> +
> +static struct event_trigger_ops *
> +snapshot_get_trigger_ops(char *cmd, char *param)
> +{
> +	return param ? &snapshot_count_trigger_ops : &snapshot_trigger_ops;
> +}
> +
> +static struct event_command trigger_snapshot_cmd = {
> +	.name			= "snapshot",
> +	.trigger_type		= ETT_SNAPSHOT,
> +	.func			= event_trigger_callback,
> +	.reg			= register_snapshot_trigger,
> +	.unreg			= unregister_trigger,
> +	.get_trigger_ops	= snapshot_get_trigger_ops,
> +};
> +
>  static __init void unregister_trigger_traceon_traceoff_cmds(void)
>  {
>  	unregister_event_command(&trigger_traceon_cmd);
> @@ -726,5 +794,11 @@ __init int register_trigger_cmds(void)
>  		return ret;
>  	}
>  
> +	ret = register_event_command(&trigger_snapshot_cmd);
> +	if (WARN_ON(ret < 0)) {
> +		unregister_trigger_traceon_traceoff_cmds();

If the snapshot trigger fails, why remove the traceon_traceoff trigger
if it succeeded? Is there some dependency that we should be worried
about?

Or is this just saying "if once trigger fails, they all fail!"?

-- Steve

> +		return ret;
> +	}
> +
>  	return 0;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/