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

[Steven Rostedt <rostedt@goodmis.org>]

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?

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

Thanks,

-- Steve

>  		kfree(trigger_data);