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

[Steven Rostedt]

On Tue, 23 Nov 2021 14:53:54 -0600
Tom Zanussi <zanussi@xxxxxxxxxx> 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?

-- 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;
>  }
>