| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 22 Nov 2013 23:17:06 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: Jiri Olsa <jolsa@...hat.com>
Cc: linux-kernel@...r.kernel.org,
Corey Ashford <cjashfor@...ux.vnet.ibm.com>,
Frederic Weisbecker <fweisbec@...il.com>,
Ingo Molnar <mingo@...e.hu>, Paul Mackerras <paulus@...ba.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
Steven Rostedt <rostedt@...dmis.org>,
David Ahern <dsahern@...il.com>
Subject: Re: [PATCH 01/22] tools lib traceevent: Add plugin support
Hi Jiri,
2013-11-21 (목), 12:00 +0100, Jiri Olsa:
> Backporting plugin support for traceevent lib.
>
> Backported from Steven Rostedt's trace-cmd repo (HEAD 0f2c2fb):
> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/trace-cmd.git
>
> It's now possible to use following interface to load plugins
> (shared objects) to enahnce pevent object functionality.
>
> The plugin interface/hooks are as follows:
> (taken from event-parse.h comments)
>
> - 'pevent_plugin_loader' (required)
> The function name to initialized the plugin.
>
> int pevent_plugin_loader(struct pevent *pevent)
>
> - 'pevent_plugin_unloader' (optional)
> The function called just before unloading
>
> int pevent_plugin_unloader(void)
>
> - 'pevent_plugin_options' (optional)
> Plugin options that can be set before loading
>
> struct plugin_option pevent_plugin_options[] = {
> {
> .name = "option-name",
> .plugin_alias = "overide-file-name", (optional)
> .description = "description of option to show users",
> },
> {
> .name = NULL,
> },
> };
>
> Array must end with .name = NULL;
>
> The plugin_alias (below) can be used to give a shorter
> name to access the vairable. Useful if a plugin handles
> more than one event.
>
> NOTE options support is not backported yet.
>
> - 'pevent_plugin_alias' (optional)
> The name to use for finding options (uses filename if not defined)
>
> New traceevent functions are added to search and load
> available plugins:
>
> struct plugin_list*
> traceevent_load_plugins(struct pevent *pevent)
> - loads plusing for 'struct pevent' object and returns
> loaded plugins list
>
> void traceevent_unload_plugins(struct plugin_list *plugin_list);
> - unload plugin list
First of all, thank you for your nice work! :)
>
[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.
> +
> + 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.
IIUC _GLOBAL is for exporting symbols to *other libraries*. Is it
intended for this plugin support?
> + 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?
> +
> + 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.
> + 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.
> + 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
> +
> + strcpy(path, home);
> + strcat(path, "/");
> + strcat(path, LOCAL_PLUGIN_DIR);
> +
> + load_plugins_dir(pevent, suffix, path, load_plugin, data);
> +
> + free(path);
> +}
> +
> +struct plugin_list*
> +traceevent_load_plugins(struct pevent *pevent)
> +{
> + struct plugin_list *list = NULL;
> +
> + load_plugins(pevent, ".so", load_plugin, &list);
> + return list;
> +}
> +
> +void
> +traceevent_unload_plugins(struct plugin_list *plugin_list)
> +{
> + pevent_plugin_unload_func func;
> + struct plugin_list *list;
> +
> + while (plugin_list) {
> + list = plugin_list;
> + plugin_list = list->next;
> + func = dlsym(list->handle, PEVENT_PLUGIN_UNLOADER_NAME);
> + if (func)
> + func();
> + dlclose(list->handle);
> + free(list->name);
> + free(list);
> + }
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists