Re: [PATCH v3 1/4] tracing/user_events: Prepare find/delete for same name events

[Beau Belgrave]

On Wed, Feb 21, 2024 at 10:17:21AM -0500, Steven Rostedt wrote:
> On Wed, 14 Feb 2024 17:50:43 +0000
> Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> So the patches look good, but since I gave you some updates, I'm now going
> to go though "nits". Like grammar and such ;-)
> 

Sure thing.

> > The current code for finding and deleting events assumes that there will
> > never be cases when user_events are registered with the same name, but
> > different formats. In the future this scenario will exist to ensure
> 
> > user programs can be updated or modify their events and run different
> > versions of their programs side-by-side without being blocked.
> 
> Can you change the last sentence above. I read it three times and it's
> still awkward to understand it. Particularly, the "user programs can be
> updated or modify their events". That just doesn't want to compute.
> 

Yeah, I'll clean this up.

> > 
> > This change does not yet allow for multi-format events. If user_events
> > are registered with the same name but different arguments the programs
> > see the same return values as before. This change simply makes it
> > possible to easily accomodate for this in future changes.
> 
> I think you can drop the "in future changes" part.
> 

Agreed.

> > 
> > Update find_user_event() to take in argument parameters and register
> > flags to accomodate future multi-format event scenarios. Have find
> > validate argument matching and return error pointers to cover address
> > in use cases, or allocation errors. Update callers to handle error
> 
>   "to cover address in use cases" ?

Yeah, if the ABI is using a single-format event and it's already in use,
we return -EADDRINUSE. It does not happen in multi-format event cases,
since that is allowed.

I'll try to clarify this a bit.

> 
> > pointer logic.
> > 
> > Move delete_user_event() to use hash walking directly now that find has
> > changed. Delete all events found that match the register name, stop
> 
> "now that find has changed" ?  You mean the "find function"?
> 

Yeah, I'll just use the function name here, find_user_event().

> > if an error occurs and report back to the user.
> > 
> > Update user_fields_match() to cover list_empty() scenarios instead of
> > each callsite doing it now that find_user_event() uses it directly.
> 
> The above is a bit of a run-on sentence.
> 

I'll clean it up a bit.

Thanks,
-Beau

> -- Steve
> 
> > 
> > Signed-off-by: Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx>
> > ---