Hi Masami,
Thanks for taking the time to look at those changes.
On Thu, Jan 25, 2024 at 12:11:49AM +0900, Masami Hiramatsu wrote:
> On Tue, 23 Jan 2024 11:07:54 +0000
> Vincent Donnefort <vdonnef...@google.com> wrote:
>
> [...]
> > @@ -6592,8 +6641,11 @@ int tracing_set_tracer(struct trace_array *tr, const
> > char *buf)
> >
> > if (t->init) {
> > ret = tracer_init(t, tr);
> > - if (ret)
> > + if (ret) {
> > + if (t->use_max_tr)
> > + tracing_disarm_snapshot_locked(tr);
>
> This part is out of CONFIG_TRACER_MAX_TRACE, so it may cause a compile error
> if CONFIG_TRACER_MAX_TRACE is not set.
Duh, yes it must depends on TRACER_MAX_TRACE :-\
>
> > goto out;
> > + }
> > }
> >
> > tr->current_trace = t;
> [...]
> > diff --git a/kernel/trace/trace_events_trigger.c
> > b/kernel/trace/trace_events_trigger.c
> > index 46439e3bcec4..d41bf64741e2 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -597,20 +597,9 @@ static int register_trigger(char *glob,
> > return ret;
> > }
> >
> > -/**
> > - * unregister_trigger - Generic event_command @unreg implementation
> > - * @glob: The raw string used to register the trigger
> > - * @test: Trigger-specific data used to find the trigger to remove
> > - * @file: The trace_event_file associated with the event
> > - *
> > - * Common implementation for event trigger unregistration.
> > - *
> > - * Usually used directly as the @unreg method in event command
> > - * implementations.
> > - */
> > -static void unregister_trigger(char *glob,
> > - struct event_trigger_data *test,
> > - struct trace_event_file *file)
>
> OK, so __unregister_trigger returns true if data exists, but
> unregister_trigger() ignores results. (I want some comment here)
Will add something for the __unregister_trigger flavour.
>
> > +static bool __unregister_trigger(char *glob,
> > + struct event_trigger_data *test,
> > + struct trace_event_file *file)
> > {
> > struct event_trigger_data *data = NULL, *iter;
> >
> > @@ -626,8 +615,32 @@ static void unregister_trigger(char *glob,
> > }
> > }
> >
> > - if (data && data->ops->free)
> > - data->ops->free(data);
> > + if (data) {
> > + if (data->ops->free)
> > + data->ops->free(data);
> > +
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +/**
> > + * unregister_trigger - Generic event_command @unreg implementation
> > + * @glob: The raw string used to register the trigger
> > + * @test: Trigger-specific data used to find the trigger to remove
> > + * @file: The trace_event_file associated with the event
> > + *
> > + * Common implementation for event trigger unregistration.
> > + *
> > + * Usually used directly as the @unreg method in event command
> > + * implementations.
> > + */
> > +static void unregister_trigger(char *glob,
> > + struct event_trigger_data *test,
> > + struct trace_event_file *file)
> > +{
> > + __unregister_trigger(glob, test, file);
> > }
> >
> > /*
> > @@ -1470,12 +1483,20 @@ register_snapshot_trigger(char *glob,
> > struct event_trigger_data *data,
> > struct trace_event_file *file)
> > {
> > - if (tracing_alloc_snapshot_instance(file->tr) != 0)
> > + if (tracing_arm_snapshot(file->tr))
> > return 0;
>
> BTW, is this return value correct? It seems that the register_*_trigger()
> will return error code when it fails.
It should indeed be
ret = tracing_arm_snapshot()
if (ret)
return ret;
>
> Thanks,
>
> >
> > return register_trigger(glob, data, file);
> > }
> >
> > +static void unregister_snapshot_trigger(char *glob,
> > + struct event_trigger_data *data,
> > + struct trace_event_file *file)
> > +{
> > + if (__unregister_trigger(glob, data, file))
> > + tracing_disarm_snapshot(file->tr);
> > +}
> > +
> > static int
> > snapshot_trigger_print(struct seq_file *m, struct event_trigger_data *data)
> > {
> > @@ -1508,7 +1529,7 @@ static struct event_command trigger_snapshot_cmd = {
> > .trigger_type = ETT_SNAPSHOT,
> > .parse = event_trigger_parse,
> > .reg = register_snapshot_trigger,
> > - .unreg = unregister_trigger,
> > + .unreg = unregister_snapshot_trigger,
> > .get_trigger_ops = snapshot_get_trigger_ops,
> > .set_filter = set_trigger_filter,
> > };
> > --
> > 2.43.0.429.g432eaa2c6b-goog
> >
>
>
> --
> Masami Hiramatsu (Google) <mhira...@kernel.org>
