Re: [PATCH v2 03/15] tools lib traceevent: Add tep_load_plugins_hook() API

From: Steven Rostedt
Date: Tue Jul 07 2020 - 11:35:47 EST


On Tue, 7 Jul 2020 23:55:34 +0900
Namhyung Kim <namhyung@xxxxxxxxxx> wrote:

> > -static void
> > -load_plugins(struct tep_handle *tep, const char *suffix,
> > - void (*load_plugin)(struct tep_handle *tep,
> > - const char *path,
> > - const char *name,
> > - void *data),
> > - void *data)
> > +void tep_load_plugins_hook(struct tep_handle *tep, const char *suffix,
> > + void (*load_plugin)(struct tep_handle *tep,
> > + const char *path,
> > + const char *name,
> > + void *data),
> > + void *data)
>
> Can we have a comment (or a doc) for this API? I'm not sure how it's used..

Actually, this requires a man page.

>
>
> > {
> > char *home;
> > char *path;
> > char *envdir;
> > int ret;
> >
> > - if (tep->flags & TEP_DISABLE_PLUGINS)
> > + if (tep && tep->flags & TEP_DISABLE_PLUGINS)
> > return;
>
> Is it ok to call with a NULL tep handle?

Hmm, it looks like we could possibly pass this in without a tep handle,
if the plugins don't need a it. I'm not sure we have a use case for
that. I'll need to look deeper at this.

Thanks for the review!

-- Steve

> >
> > /*
> > @@ -386,7 +385,7 @@ load_plugins(struct tep_handle *tep, const char
> > *suffix,
> > * check that first.
> > */
> > #ifdef PLUGIN_DIR
> > - if (!(tep->flags & TEP_DISABLE_SYS_PLUGINS))
> > + if (!tep || !(tep->flags & TEP_DISABLE_SYS_PLUGINS))
> > load_plugins_dir(tep, suffix, PLUGIN_DIR,
> > load_plugin, data);
> > #endif
> > @@ -423,7 +422,7 @@ tep_load_plugins(struct tep_handle *tep)
> > {
> > struct tep_plugin_list *list = NULL;
> >
> > - load_plugins(tep, ".so", load_plugin, &list);
> > + tep_load_plugins_hook(tep, ".so", load_plugin, &list);
> > return list;
> > }