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

[Tom Zanussi <zanussi@kernel.org>]

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

> 
> > +	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.

Will have to investigate tomorrow..

Tom


> 
> Thanks,
> 
> -- Steve
> 
> >  		kfree(trigger_data);