Re: [PATCH 2/4] tools lib traceevent: Add options to plugins
From: Namhyung Kim
Date: Mon May 19 2014 - 10:29:47 EST
Hi Steve,
2014-05-16 (ê), 10:02 -0400, Steven Rostedt:
> From: "Steven Rostedt (Red Hat)" <rostedt@xxxxxxxxxxx>
>
> The traceevent plugins allows developers to have their events print out
> information that is more advanced than what can be achieved by the
> trace event format files.
>
> As these plugins are used on the userspace side of the tracing tools, it
> is only logical that the tools should be able to produce different types
> of output for the events. The types of events still need to be defined by
> the plugins thus we need a way to pass information from the tool to the
> plugin to specify what type of information to be shown.
>
> Not only does the information need to be passed by the tool to plugin, but
> the plugin also requires a way to notify the tool of what options it can
> provide.
>
> This builds the plugin option infrastructure that is taken from trace-cmd
> that is used to allow plugins to produce different output based on the
> options specified by the tool.
>
> Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> ---
> tools/lib/traceevent/event-parse.h | 16 ++-
> tools/lib/traceevent/event-plugin.c | 197 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 209 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index a68ec3d8289f..d6c610a66006 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -107,8 +107,8 @@ typedef int (*pevent_event_handler_func)(struct trace_seq *s,
> typedef int (*pevent_plugin_load_func)(struct pevent *pevent);
> typedef int (*pevent_plugin_unload_func)(struct pevent *pevent);
>
> -struct plugin_option {
> - struct plugin_option *next;
> +struct pevent_plugin_option {
> + struct pevent_plugin_option *next;
Hmm.. this name change reminds me that it might be better if this plugin
and option list belong to a pevent..
> void *handle;
> char *file;
> char *name;
> @@ -135,7 +135,7 @@ struct plugin_option {
> * PEVENT_PLUGIN_OPTIONS: (optional)
> * Plugin options that can be set before loading
> *
> - * struct plugin_option PEVENT_PLUGIN_OPTIONS[] = {
> + * struct pevent_plugin_option PEVENT_PLUGIN_OPTIONS[] = {
> * {
> * .name = "option-name",
> * .plugin_alias = "overide-file-name", (optional)
> @@ -355,7 +355,7 @@ enum pevent_func_arg_type {
> enum pevent_flag {
> PEVENT_NSEC_OUTPUT = 1, /* output in NSECS */
> PEVENT_DISABLE_SYS_PLUGINS = 1 << 1,
> - PEVENT_DISABLE_PLUGINS = 1 << 2,
> + PEVENT_DISABLE_PLUGINS = 1 << 2
Unnecessary change?
> };
>
> #define PEVENT_ERRORS \
> @@ -415,6 +415,14 @@ struct plugin_list;
> struct plugin_list *traceevent_load_plugins(struct pevent *pevent);
> void traceevent_unload_plugins(struct plugin_list *plugin_list,
> struct pevent *pevent);
> +char **traceevent_plugin_list_options(void);
> +void traceevent_plugin_free_options_list(char **list);
> +int traceevent_plugin_add_options(const char *name,
> + struct pevent_plugin_option *options);
> +void traceevent_plugin_remove_options(struct pevent_plugin_option *options);
> +void traceevent_print_plugins(struct trace_seq *s,
> + const char *prefix, const char *suffix,
> + const struct plugin_list *list);
>
> struct cmdline;
> struct cmdline_list;
> diff --git a/tools/lib/traceevent/event-plugin.c b/tools/lib/traceevent/event-plugin.c
> index 317466bd1a37..a24479426d58 100644
> --- a/tools/lib/traceevent/event-plugin.c
> +++ b/tools/lib/traceevent/event-plugin.c
> @@ -18,6 +18,7 @@
> * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> */
>
> +#include <stdio.h>
> #include <string.h>
> #include <dlfcn.h>
> #include <stdlib.h>
> @@ -30,12 +31,208 @@
>
> #define LOCAL_PLUGIN_DIR ".traceevent/plugins"
>
> +static struct registered_plugin_options {
> + struct registered_plugin_options *next;
> + struct pevent_plugin_option *options;
> +} *registered_options;
> +
> +static struct trace_plugin_options {
> + struct trace_plugin_options *next;
> + char *plugin;
> + char *option;
> + char *value;
> +} *trace_plugin_options;
> +
> struct plugin_list {
> struct plugin_list *next;
> char *name;
> void *handle;
> };
>
> +/**
> + * traceevent_plugin_list_options - get list of plugin options
> + *
> + * Returns an array of char strings that list the currently registered
> + * plugin options in the format of <plugin>:<option>. This list can be
> + * used by toggling the option.
> + *
> + * Returns NULL if there's no options registered. On error it returns
> + * an (char **)-1 (must check for that)
What about making it a macro like INVALID_OPTION_LIST?
> + *
> + * Must be freed with traceevent_plugin_free_options_list().
> + */
> +char **traceevent_plugin_list_options(void)
> +{
> + struct registered_plugin_options *reg;
> + struct pevent_plugin_option *op;
> + char **list = NULL;
> + char *name;
> + int count = 0;
> +
> + for (reg = registered_options; reg; reg = reg->next) {
> + for (op = reg->options; op->name; op++) {
> + char *alias = op->plugin_alias ? op->plugin_alias : op->file;
> +
> + name = malloc(strlen(op->name) + strlen(alias) + 2);
> + if (!name)
> + goto err;
> +
> + sprintf(name, "%s:%s", alias, op->name);
> + list = realloc(list, count + 2);
> + if (!list) {
This will lost the original list pointer. Please use a temp variable.
> + free(name);
> + goto err;
> + }
> + list[count++] = name;
> + list[count] = NULL;
> + }
> + }
> + if (!count)
> + return NULL;
> + return list;
Isn't it enough to simply return the list?
> +
> + err:
> + while (--count > 0)
Shouldn't it be >= instead of > ?
Thanks,
Namhyung
> + free(list[count]);
> + free(list);
> +
> + return (char **)((unsigned long)-1);
> +}
--
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/