Re: [PATCH v2 8/9] eventfs: moving tracing/events to eventfs

[Ajay Kaher <akaher@vmware.com>]

> On 09-May-2023, at 10:15 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> !! External Email
> 
> On Tue, 9 May 2023 12:29:23 +0000
> Ajay Kaher <akaher@vmware.com> wrote:
> 
>> > On 02/05/23, 11:42 PM, "kernel test robot" <lkp@intel.com> wrote:
>>>>> kernel/trace/trace_events.c:2424:17: warning: variable 'd_events' set but not used [-Wunused-but-set-variable]
>>>          struct dentry *d_events;
>>>                      ^
>>>   1 warning generated.
>>> 
>> 
>> Steve, with-in event_create_dir(), do we have any scenario when file->event_call->class->system
>> doesn't have TRACE_SYSTEM? And need to execute following:
>> 
>> ae63b31e4d0e2e Steven Rostedt           2012-05-03  2437                d_events = parent;
>> 
>> looking for your input if we could remove d_events from event_create_dir().
>> 
> 
> I have hit this in the beginning, but I don't think it's an issue
> anymore. Perhaps just have it be:
> 
>        if (WARN_ON_ONCE(strcmp(call->class->system, TRACE_SYSTEM) == 0))
>                return -ENODEV;
> 
>        ef_subsystem = event_subsystem_dir(tr, call->class->system, file, parent);
> 
> Hmm, how about just add this patch before your patch set:
> 

Sounds good. Thanks Steve :)
Once you will merge below patch, I will rebase this patch in v3.

- Ajay

> 
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> Subject: [PATCH] tracing: Require all trace events to have a TRACE_SYSTEM
> 
> The creation of the trace event directory requires that a TRACE_SYSTEM is
> defined that the trace event directory is added within the system it was
> defined in.
> 
> The code handled the case where a TRACE_SYSTEM was not added, and would
> then add the event at the events directory. But nothing should be doing
> this. This code also prevents the implementation of creating dynamic
> dentrys for the eventfs system.
> 
> As this path has never been hit on correct code, remove it. If it does get
> hit, issues a WARN_ON_ONCE() and return ENODEV.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> 
>  (lightly tested)
> 
> kernel/trace/trace_events.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 654ffa40457a..16bc5ba45507 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2424,14 +2424,15 @@ event_create_dir(struct dentry *parent, struct trace_event_file *file)
> 
>        /*
>         * If the trace point header did not define TRACE_SYSTEM
> -        * then the system would be called "TRACE_SYSTEM".
> +        * then the system would be called "TRACE_SYSTEM". This should
> +        * never happen.
>         */
> -       if (strcmp(call->class->system, TRACE_SYSTEM) != 0) {
> -               d_events = event_subsystem_dir(tr, call->class->system, file, parent);
> -               if (!d_events)
> -                       return -ENOMEM;
> -       } else
> -               d_events = parent;
> +       if (WARN_ON_ONCE(strcmp(call->class->system, TRACE_SYSTEM) == 0))
> +               return -ENODEV;
> +
> +       d_events = event_subsystem_dir(tr, call->class->system, file, parent);
> +       if (!d_events)
> +               return -ENOMEM;
> 
>        name = trace_event_name(call);
>        file->dir = tracefs_create_dir(name, d_events);
> --
> 2.39.2
> 
> 
> !! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.