On Mon, Nov 1, 2021 at 7:22 PM Yordan Karadzhov <y.karadz@xxxxxxxxx> 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, ...);
struct tracefs_dynevent *kprobe =
tracefs_dynevent_alloc(TRACE_DYNEVENT_KPROBE, ...);
ret = tracefs_dynevent_create(synth);
ret = tracefs_dynevent_create(kprobe);
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!
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|