Re: [PATCH 01/22] tools lib traceevent: Add plugin support

[Steven Rostedt]

On Fri, 22 Nov 2013 23:17:06 +0900
Namhyung Kim <namhyung@xxxxxxxxxx> wrote:

> > 
> [SNIP[
> > +static void
> > +load_plugin(struct pevent *pevent, const char *path,
> > +	    const char *file, void *data)
> > +{
> > +	struct plugin_list **plugin_list = data;
> > +	pevent_plugin_load_func func;
> > +	struct plugin_list *list;
> > +	const char *alias;
> > +	char *plugin;
> > +	void *handle;
> > +
> > +	plugin = malloc_or_die(strlen(path) + strlen(file) + 2);
> 
> I'd like not to see this malloc_or_die() anymore in a new code.  Just
> returning after showing a warning looks enough here.

Yeah I agree. This is a relic from my code. I think it's OK to add
here, as it is pretty much direct port of my code, and then we can just
add a patch against it to remove it.

> 
> > +
> > +	strcpy(plugin, path);
> > +	strcat(plugin, "/");
> > +	strcat(plugin, file);
> > +
> > +	handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
> 
> Why RTLD_NOW and RTLD_GLOBAL?  Hmm.. maybe using _NOW is needed to
> prevent a runtime error, but not sure why _GLOBAL is needed.

Yes, we want to make sure all symbols defined are available at time of
load, otherwise bail out.

> 
> IIUC _GLOBAL is for exporting symbols to *other libraries*.  Is it
> intended for this plugin support?

That was the plan. To have one plugin supply a set of functions that
other plugins may use. That is what GLOBAL is for, right?  I don't
recall if I every did this, but it was something I wanted for future
work.

Now if we don't need it, we could remove it, but is it bad to have?


> 
> 
> > +	if (!handle) {
> > +		warning("could not load plugin '%s'\n%s\n",
> > +			plugin, dlerror());
> > +		goto out_free;
> > +	}
> > +
> > +	alias = dlsym(handle, PEVENT_PLUGIN_ALIAS_NAME);
> > +	if (!alias)
> > +		alias = file;
> 
> So this 'alias' is not used anywhere in the current code, right?
> Do you plan to add the option processing soon?

I believe he is, and hopefully he will, because I want to forward port
this code to have trace-cmd use it. The end result should be a separate
library that we all can use.

It may be best to add the above alias when we add option support. But
it doesn't hurt to include it now, as long as we plan to add that in
the near future.

> 
> > +
> > +	func = dlsym(handle, PEVENT_PLUGIN_LOADER_NAME);
> > +	if (!func) {
> > +		warning("could not find func '%s' in plugin '%s'\n%s\n",
> > +			PEVENT_PLUGIN_LOADER_NAME, plugin, dlerror());
> > +		goto out_free;
> > +	}
> > +
> > +	list = malloc_or_die(sizeof(*list));
> 
> Ditto.  Please check return value and handle error properly.

I agree, but lets make that a separate patch, such that we see the
changes between the backport and updates.

> 
> 
> > +	list->next = *plugin_list;
> > +	list->handle = handle;
> > +	list->name = plugin;
> > +	*plugin_list = list;
> > +
> > +	pr_stat("registering plugin: %s", plugin);
> > +	func(pevent);
> > +	return;
> > +
> > + out_free:
> > +	free(plugin);
> > +}
> > +
> > +static void
> > +load_plugins_dir(struct pevent *pevent, const char *suffix,
> > +		 const char *path,
> > +		 void (*load_plugin)(struct pevent *pevent,
> > +				     const char *path,
> > +				     const char *name,
> > +				     void *data),
> 
> Doesn't gcc complain about the name shadows other function?  Anyway I
> think we should avoid using same name - probably using typedef for the
> function might be helpful.

I agree here too, but again, lets keep the backport and fixes as
separate patches.

-- Steve

> 
> 
> > +		 void *data)
> [SNIP]
> > +	/*
> > +	 * Now let the home directory override the environment
> > +	 * or system defaults.
> > +	 */
> > +	home = getenv("HOME");
> > +	if (!home)
> > +		return;
> > +
> > +	path = malloc_or_die(strlen(home) + strlen(LOCAL_PLUGIN_DIR) + 2);
> 
> Ditto.
> 
> Thanks,
> Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/