Re: [PATCH v2 03/12] libtracefs: New kprobes APIs

[Yordan Karadzhov <y.karadz@gmail.com>]

On 2.11.21 г. 6:58, Tzvetomir Stoyanov wrote:
> On Mon, Nov 1, 2021 at 7:22 PM Yordan Karadzhov <y.karadz@gmail.com> wrote:
>>
>>
> [ ... ]
>>> +/**
>>> + * tracefs_kprobe_create - Create a kprobe or kretprobe in the system
>>> + * @kprobe: Pointer to a kprobe context, describing the probe
>>> + *
>>> + * Return 0 on success, or -1 on error.
>>> + */
>>> +int tracefs_kprobe_create(struct tracefs_dynevent *kprobe)
>>> +{
>>> +     return dynevent_create(kprobe);
>>> +}
>>> +
>>
>> hmm, what will happen if if you have a code like this:
>>
>>
>> struct tracefs_dynevent *synth = tracefs_synth_alloc(...)
>>
>> int ret = tracefs_kprobe_create(synth);
>>
>>
>> Are you just giving several additional fake names to the same function ('dynevent_create()')?
>>
>>
> 
> Yes, that will work. All dynamic events - kprobes, uprobes, eprobes
> and synthetic events have almost the same configuration logic, which
> is handled by these dynevent_... internal APIs. That makes the code
> more consistent and reusable.
> I see three possible approaches to handle that:
>   1. Create structures like that:
>        struct tracefs_kprobe {
>           struct tracefs_dynevent dynevent;
>        };
>     and use only that type in krpobe specific APIs:
>       int tracefs_kprobe_create(struct tracefs_kprobe *kprobe)
>       {
>           return dynevent_create(&kprobe->dynevent);
>       }
> 
> 2. Expose the dynevent_... APIs directly:
>      struct tracefs_dynevent *synth =
> tracefs_dynevent_alloc(TRACE_DYNEVENT_SYNTH, ...);

I would prefer taking this approach. It is OK to also have several helper constructors like tracefs_kprobe_alloc(), 
tracefs_synth_alloc(), etc. that will call internally dynevent_alloc().

In principle having multiple constructors is OK.


>      struct tracefs_dynevent *kprobe =
> tracefs_dynevent_alloc(TRACE_DYNEVENT_KPROBE, ...);
> 
>      ret = tracefs_dynevent_create(synth);
>      ret = tracefs_dynevent_create(kprobe);
> 

Yes, this is OK as well.

We only have a single type (struct tracefs_dynevent) so there must be a single create. And what is even more important: 
there must be ONLY ONE destructor.

Think about the usecase of the APIs. You create an object using one of the constructors. Later at some point you want to 
use the objects and after this finally you want to destroy it. If we have multiple/specialized "create" and "destroy" 
versions for the same object type, the user must somehow keep its own record of which constructor had been used trough 
the entire live of the object in order to know which is the proper destructor to call at the end.

Thanks!
Yordan


>   The problem here is that there are some small differences between
> dynamic events - some parameters are mandatory for one event and
> optional for another. That can be handled in the
> tracefs_dynevent_alloc() API, but could be confusing for the library
> users.
> 
> 3. Mixed between 1) and 2), the current approach.
> 
> The first one adds a lot of overhead, a different set of APIs for each
> type of dynamic event - but maybe it is more clear for the library
> users. The second is straightforward for implementation, less APIs -
> but that could be confusing for the users.
> 
>>> +/**
>>> + * tracefs_kprobe_free - Free a kprobe context
>>> + * @kprobe: Pointer to a kprobe context
>>> + *
>>> + * The kprobe/kretprobe, described by this context, is not
>>> + * removed from the system by this API. It only frees the memory.
>>> + */
>>> +void tracefs_kprobe_free(struct tracefs_dynevent *kprobe)
>>> +{
>>> +     dynevent_free(kprobe);
>>> +}
>>> +
>>
>> The same argument applys here.
>>
>> Thanks!
>> Yordan
>>
> 
> Thanks for the review!
>