Re: [PATCH v3 2/2] tracing: Have existing event_command implementations use helpers

[Tom Zanussi <zanussi@kernel.org>]

Hi Steve,

On Tue, 2021-11-30 at 16:31 -0500, Steven Rostedt wrote:
> On Tue, 23 Nov 2021 14:53:54 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> 
> > index a873f4e8be04..1d1716d5bee2 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -931,89 +931,47 @@ event_trigger_callback(struct event_command
> > *cmd_ops,
> >  	struct event_trigger_data *trigger_data;
> >  	struct event_trigger_ops *trigger_ops;
> >  	char *trigger = NULL;
> > -	char *number;
> > +	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);
> > +	ret = event_trigger_separate_filter(&trigger, &param, false);
> > +	if (ret)
> > +		return ret;
> >  
> >  	ret = -ENOMEM;
> > -	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
> > +	trigger_data = event_trigger_alloc(cmd_ops, cmd, trigger,
> > 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);
> > -		kfree(trigger_data);
> 
> How is trigger_data freed on error here?

You're right, that kfree() shouldn't have been removed, will add it
back.

Thanks for picking that up,

Tom

> 
> -- Steve
> 
> >  		ret = 0;
> >  		goto out;
> >  	}
> >  
> > -	if (trigger) {
> > -		number = strsep(&trigger, ":");
> > -
> > -		ret = -EINVAL;
> > -		if (!strlen(number))
> > -			goto out_free;
> > -
> > -		/*
> > -		 * We use the callback data field (which is a pointer)
> > -		 * as our counter.
> > -		 */
> > -		ret = kstrtoul(number, 0, &trigger_data->count);
> > -		if (ret)
> > -			goto out_free;
> > -	}
> > -
> > -	if (!param) /* if param is non-empty, it's supposed to be a
> > filter */
> > -		goto out_reg;
> > -
> > -	if (!cmd_ops->set_filter)
> > -		goto out_reg;
> > +	ret = event_trigger_parse_num(trigger, trigger_data);
> > +	if (ret)
> > +		goto out_free;
> >  
> > -	ret = cmd_ops->set_filter(param, trigger_data, file);
> > +	ret = event_trigger_set_filter(cmd_ops, file, param,
> > trigger_data);
> >  	if (ret < 0)
> >  		goto out_free;
> >  
> > - out_reg:
> >  	/* Up the trigger_data count to make sure reg doesn't free it
> > on failure */
> >  	event_trigger_init(trigger_ops, trigger_data);
> > -	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> > -	/*
> > -	 * The above returns on success the # of functions enabled,
> > -	 * but if it didn't find any functions it returns zero.
> > -	 * Consider no functions a failure too.
> > -	 */
> > -	if (!ret) {
> > -		cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
> > -		ret = -ENOENT;
> > -	} else if (ret > 0)
> > -		ret = 0;
> > +
> > +	ret = event_trigger_register(cmd_ops, file, glob, cmd, trigger,
> > trigger_data, NULL);
> > +	if (ret)
> > +		goto out_free;
> >  
> >  	/* Down the counter of trigger_data or free it if not used
> > anymore */
> >  	event_trigger_free(trigger_ops, trigger_data);
> >   out:
> >  	return ret;
> > -
> >   out_free:
> > -	if (cmd_ops->set_filter)
> > -		cmd_ops->set_filter(NULL, trigger_data, NULL);
> > +	event_trigger_reset_filter(cmd_ops, trigger_data);
> >  	kfree(trigger_data);
> >  	goto out;
> >  }
> >