Re: [PATCH v5 4/4] tracing: Have existing event_command.parse() implementations use helpers

[Tom Zanussi]

On Sat, 2022-01-08 at 22:23 -0600, Tom Zanussi wrote:
> Hi Steve,
> 
> On Sat, 2022-01-08 at 19:54 -0500, Steven Rostedt wrote:
> > On Tue, 14 Dec 2021 12:57:32 -0600
> > Tom Zanussi <zanussi@xxxxxxxxxx> wrote:
> > 
> > > index da103826f27e..e6a48e8c79eb 100644
> > > --- a/kernel/trace/trace_events_trigger.c
> > > +++ b/kernel/trace/trace_events_trigger.c
> > > @@ -973,7 +973,7 @@ int event_trigger_register(struct
> > > event_command
> > > *cmd_ops,
> > >   * @file: The trace_event_file associated with the event
> > >   * @glob: The raw string used to register the trigger
> > >   * @cmd: The cmd portion of the string used to register the
> > > trigger
> > > - * @param: The params portion of the string used to register the
> > > trigger
> > > + * @param_and_filter: The param and filter portion of the string
> > > used to register the trigger
> > >   *
> > >   * Common implementation for event command parsing and trigger
> > >   * instantiation.
> > > @@ -986,94 +986,53 @@ int event_trigger_register(struct
> > > event_command *cmd_ops,
> > >  static int
> > >  event_trigger_parse(struct event_command *cmd_ops,
> > >  		    struct trace_event_file *file,
> > > -		    char *glob, char *cmd, char *param)
> > > +		    char *glob, char *cmd, char *param_and_filter)
> > >  {
> > >  	struct event_trigger_data *trigger_data;
> > >  	struct event_trigger_ops *trigger_ops;
> > > -	char *trigger = NULL;
> > > -	char *number;
> > > +	char *param, *filter;
> > > +	bool remove;
> > >  	int ret;
> > >  
> > > -	/* separate the trigger from the filter (t:n [if filter]) */
> > > -	if (param && isdigit(param[0])) {
> > > -		trigger = strsep(&param, " \t");
> > > -		if (param) {
> > > -			param = skip_spaces(param);
> > > -			if (!*param)
> > > -				param = NULL;
> > > -		}
> > > -	}
> > > +	remove = event_trigger_check_remove(glob);
> > >  
> > > -	trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
> > 
> > Did you mean to remove the assignment of trigger_ops here?
> 
> Hmm, yeah, that shouldn't have been removed, but...

Actually, it should be removed, because get_trigger_ops() is now called
in event_trigger_alloc() in patch 3/4.  And trigger_ops isn't actually
used by unreg(), so the trigger_ops local can just be removed.  For
that matter, trigger_ops should probably be removed from the
reg()/unreg() callbacks altogether.  Will add a separate pach to do
that.

> 
> > 
> > > +	ret = event_trigger_separate_filter(param_and_filter, &param,
> > > &filter, false);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	ret = -ENOMEM;
> > > -	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
> > > +	trigger_data = event_trigger_alloc(cmd_ops, cmd, param, file);
> > >  	if (!trigger_data)
> > >  		goto out;
> > >  
> > > -	trigger_data->count = -1;
> > > -	trigger_data->ops = trigger_ops;
> > > -	trigger_data->cmd_ops = cmd_ops;
> > > -	trigger_data->private_data = file;
> > > -	INIT_LIST_HEAD(&trigger_data->list);
> > > -	INIT_LIST_HEAD(&trigger_data->named_list);
> > > -
> > > -	if (glob[0] == '!') {
> > > +	if (remove) {
> > >  		cmd_ops->unreg(glob+1, trigger_ops, trigger_data,
> > > file);
> > 
> > It's still used here and below.
> > 
> > I get a warning on this.
> 
> I'm not getting a warning, and remove should have crashed the
> testcases, but I'm not seeing that either.

I'm not getting a warning because of -Wno-maybe-ininitialized.  I had
to build with W=2 to see the warning.

Also, the testcases aren't crashing because trigger_ops is never used
in unreg().

I'll respin and resend later today.

Tom

> 
> Will have to investigate tomorrow..
> 
> Tom
> 
> 
> > 
> > Thanks,
> > 
> > -- Steve
> > 
> > >  		kfree(trigger_data);
> 
>