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

[Steven Rostedt]

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;
> >  }