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

From: Tom Zanussi
Date: Sat Jan 08 2022 - 23:23:14 EST


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

>
> > + 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);