linux-kernel - Re: [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <cfa03e4c70ff706b5373bf5314f453930019e1a1.camel@kernel.org>
Date: Tue, 25 Nov 2025 07:01:43 -0600
From: Tom Zanussi <zanussi@...nel.org>
To: Steven Rostedt <rostedt@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org
Cc: Masami Hiramatsu <mhiramat@...nel.org>, Mark Rutland
 <mark.rutland@....com>,  Mathieu Desnoyers
 <mathieu.desnoyers@...icios.com>, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct
 event_command

Hi Steve,

On Tue, 2025-11-18 at 22:10 -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@...dmis.org>
> 
> Now that there's pretty much a one to one mapping between the struct
> event_trigger_ops and struct event_command, there's no reason to have two
> different structures. Merge the function pointers of event_trigger_ops
> into event_command.
> 
> There's one exception in trace_events_hist.c for the
> event_hist_trigger_named_ops. This has special logic for the init and free
> function pointers for "named histograms". In this case, allocate the
> cmd_ops of the event_trigger_data and set it to the proper init and free
> functions, which are used to initialize and free the event_trigger_data
> respectively. Have the free function and the init function (on failure)
> free the cmd_ops of the data element.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>

Very nice as well, just one tiny note below..

Reviewed-by: Tom Zanussi <zanussi@...nel.org>


> ---
>  kernel/trace/trace.h                | 126 +++++++++++-----------------
>  kernel/trace/trace_eprobe.c         |  13 +--
>  kernel/trace/trace_events_hist.c    |  93 ++++++++++----------
>  kernel/trace/trace_events_trigger.c | 121 +++++++++++---------------
>  4 files changed, 152 insertions(+), 201 deletions(-)
> 

[...]

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index f9cc8d6a215b..1e03398b9e91 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -5694,7 +5694,7 @@ static void hist_trigger_show(struct seq_file *m,
>  		seq_puts(m, "\n\n");
>  
>  	seq_puts(m, "# event histogram\n#\n# trigger info: ");
> -	data->ops->print(m, data);
> +	data->cmd_ops->print(m, data);
>  	seq_puts(m, "#\n\n");
>  
>  	hist_data = data->private_data;
> @@ -6016,7 +6016,7 @@ static void hist_trigger_debug_show(struct seq_file *m,
>  		seq_puts(m, "\n\n");
>  
>  	seq_puts(m, "# event histogram\n#\n# trigger info: ");
> -	data->ops->print(m, data);
> +	data->cmd_ops->print(m, data);
>  	seq_puts(m, "#\n\n");
>  
>  	hist_data = data->private_data;
> @@ -6326,20 +6326,23 @@ static void event_hist_trigger_free(struct event_trigger_data *data)
>  	free_hist_pad();
>  }
>  
> -static const struct event_trigger_ops event_hist_trigger_ops = {
> -	.trigger		= event_hist_trigger,
> -	.print			= event_hist_trigger_print,
> -	.init			= event_hist_trigger_init,
> -	.free			= event_hist_trigger_free,
> -};
> +static struct event_command trigger_hist_cmd;
>  

I don't think this is needed, since the same declaration already
appears just above the event_hist_trigger_parse() declaration.

Thanks,

Tom