From: Namhyung Kim <namhyung@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
Linux Trace Devel <linux-trace-devel@vger.kernel.org>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Ingo Molnar <mingo@kernel.org>, Jiri Olsa <jolsa@redhat.com>,
Andrew Morton <akpm@linux-foundation.org>,
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
Subject: Re: [PATCH v2 04/15] tools lib traceevent: Add interface for options to plugins
Date: Wed, 8 Jul 2020 00:03:44 +0900 [thread overview]
Message-ID: <CAM9d7cizjF+fbK7YzmsBDgrx__4YAOsmEq67D3sWET8FF+YdFA@mail.gmail.com> (raw)
In-Reply-To: <20200702185704.092654084@goodmis.org>
On Fri, Jul 3, 2020 at 3:57 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> From: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
>
> Add tep_plugin_add_option() and tep_plugin_print_options() to lib traceevent
> library that allows plugins to have their own options. For example, the
> function plugin by default does not print the parent, as it uses the parent
> to do the indenting. The "parent" option is created by the function plugin
> that will print the parent of the function like it does in the trace file.
>
> The tep_plugin_print_options() will print out the list of options that a
> plugin has defined.
>
> Link: http://lore.kernel.org/linux-trace-devel/20190802110101.14759-3-tz.stoyanov@gmail.com
> Link: http://lore.kernel.org/linux-trace-devel/20200625100516.365338-5-tz.stoyanov@gmail.com
>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> tools/lib/traceevent/event-parse.h | 2 +
> tools/lib/traceevent/event-plugin.c | 172 ++++++++++++++++++++++++++++
> 2 files changed, 174 insertions(+)
>
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index 776c7c24ee79..02c0438527de 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -406,7 +406,9 @@ char **tep_plugin_list_options(void);
> void tep_plugin_free_options_list(char **list);
> int tep_plugin_add_options(const char *name,
> struct tep_plugin_option *options);
> +int tep_plugin_add_option(const char *name, const char *val);
> void tep_plugin_remove_options(struct tep_plugin_option *options);
> +void tep_plugin_print_options(struct trace_seq *s);
> void tep_print_plugins(struct trace_seq *s,
> const char *prefix, const char *suffix,
> const struct tep_plugin_list *list);
> diff --git a/tools/lib/traceevent/event-plugin.c b/tools/lib/traceevent/event-plugin.c
> index b53d9a53bcf9..e8f4329ba8e0 100644
> --- a/tools/lib/traceevent/event-plugin.c
> +++ b/tools/lib/traceevent/event-plugin.c
> @@ -13,6 +13,7 @@
> #include <sys/stat.h>
> #include <unistd.h>
> #include <dirent.h>
> +#include <errno.h>
> #include "event-parse.h"
> #include "event-parse-local.h"
> #include "event-utils.h"
> @@ -247,6 +248,166 @@ void tep_plugin_remove_options(struct tep_plugin_option *options)
> }
> }
>
> +static void parse_option_name(char **option, char **plugin)
> +{
> + char *p;
> +
> + *plugin = NULL;
> +
> + if ((p = strstr(*option, ":"))) {
> + *plugin = *option;
> + *p = '\0';
> + *option = strdup(p + 1);
> + if (!*option)
> + return;
It needs to pass the error somehow..
> + }
> +}
> +
> +static struct tep_plugin_option *
> +find_registered_option(const char *plugin, const char *option)
> +{
> + struct registered_plugin_options *reg;
> + struct tep_plugin_option *op;
> + const char *op_plugin;
> +
> + for (reg = registered_options; reg; reg = reg->next) {
> + for (op = reg->options; op->name; op++) {
> + if (op->plugin_alias)
> + op_plugin = op->plugin_alias;
> + else
> + op_plugin = op->file;
> +
> + if (plugin && strcmp(plugin, op_plugin) != 0)
> + continue;
> + if (strcmp(option, op->name) != 0)
> + continue;
> +
> + return op;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +static int process_option(const char *plugin, const char *option, const char *val)
> +{
> + struct tep_plugin_option *op;
> +
> + op = find_registered_option(plugin, option);
> + if (!op)
> + return 0;
> +
> + return update_option_value(op, val);
> +}
> +
> +/**
> + * tep_plugin_add_option - add an option/val pair to set plugin options
> + * @name: The name of the option (format: <plugin>:<option> or just <option>)
> + * @val: (optiona) the value for the option
Typo.
> + *
> + * Modify a plugin option. If @val is given than the value of the option
> + * is set (note, some options just take a boolean, so @val must be either
> + * "1" or "0" or "true" or "false").
> + */
> +int tep_plugin_add_option(const char *name, const char *val)
> +{
> + struct trace_plugin_options *op;
> + char *option_str;
> + char *plugin;
> +
> + option_str = strdup(name);
> + if (!option_str)
> + return -ENOMEM;
> +
> + parse_option_name(&option_str, &plugin);
> +
> + /* If the option exists, update the val */
> + for (op = trace_plugin_options; op; op = op->next) {
> + /* Both must be NULL or not NULL */
> + if ((!plugin || !op->plugin) && plugin != op->plugin)
> + continue;
> + if (plugin && strcmp(plugin, op->plugin) != 0)
> + continue;
> + if (strcmp(op->option, option_str) != 0)
> + continue;
> +
> + /* update option */
> + free(op->value);
> + if (val) {
> + op->value = strdup(val);
> + if (!op->value)
> + goto out_free;
> + } else
> + op->value = NULL;
> +
> + /* plugin and option_str don't get freed at the end */
> + free(plugin);
> + free(option_str);
> +
> + plugin = op->plugin;
> + option_str = op->option;
> + break;
> + }
> +
> + /* If not found, create */
> + if (!op) {
> + op = malloc(sizeof(*op));
> + if (!op)
> + return -ENOMEM;
goto out_free ?
> + memset(op, 0, sizeof(*op));
> + op->next = trace_plugin_options;
> + trace_plugin_options = op;
I think it's better doing this after setting the value below..
> +
> + op->plugin = plugin;
> + op->option = option_str;
> +
> + if (val) {
> + op->value = strdup(val);
> + if (!op->value)
> + goto out_free;
The option_str will be freed and op will have a dangling pointer..
> + }
> + }
> +
> + return process_option(plugin, option_str, val);
> + out_free:
> + free(option_str);
Why not free plugin too?
Thanks
Namhyung
> + return -ENOMEM;
> +}
> +
> +static void print_op_data(struct trace_seq *s, const char *name,
> + const char *op)
> +{
> + if (op)
> + trace_seq_printf(s, "%8s:\t%s\n", name, op);
> +}
> +
> +/**
> + * tep_plugin_print_options - print out the registered plugin options
> + * @s: The trace_seq descriptor to write the plugin options into
> + *
> + * Writes a list of options into trace_seq @s.
> + */
> +void tep_plugin_print_options(struct trace_seq *s)
> +{
> + struct registered_plugin_options *reg;
> + struct tep_plugin_option *op;
> +
> + for (reg = registered_options; reg; reg = reg->next) {
> + if (reg != registered_options)
> + trace_seq_printf(s, "============\n");
> + for (op = reg->options; op->name; op++) {
> + if (op != reg->options)
> + trace_seq_printf(s, "------------\n");
> + print_op_data(s, "file", op->file);
> + print_op_data(s, "plugin", op->plugin_alias);
> + print_op_data(s, "option", op->name);
> + print_op_data(s, "desc", op->description);
> + print_op_data(s, "value", op->value);
> + trace_seq_printf(s, "%8s:\t%d\n", "set", op->set);
> + }
> + }
> +}
> +
> /**
> * tep_print_plugins - print out the list of plugins loaded
> * @s: the trace_seq descripter to write to
> @@ -273,6 +434,7 @@ load_plugin(struct tep_handle *tep, const char *path,
> const char *file, void *data)
> {
> struct tep_plugin_list **plugin_list = data;
> + struct tep_plugin_option *options;
> tep_plugin_load_func func;
> struct tep_plugin_list *list;
> const char *alias;
> @@ -297,6 +459,16 @@ load_plugin(struct tep_handle *tep, const char *path,
> if (!alias)
> alias = file;
>
> + options = dlsym(handle, TEP_PLUGIN_OPTIONS_NAME);
> + if (options) {
> + while (options->name) {
> + ret = update_option(alias, options);
> + if (ret < 0)
> + goto out_free;
> + options++;
> + }
> + }
> +
> func = dlsym(handle, TEP_PLUGIN_LOADER_NAME);
> if (!func) {
> warning("could not find func '%s' in plugin '%s'\n%s\n",
> --
> 2.26.2
>
>
next prev parent reply other threads:[~2020-07-07 15:04 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-02 18:53 [PATCH v2 00/15] tools lib traceevent: Patches from the trace-cmd repo Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 01/15] tools lib traceevent: Add API to read time information from kbuffer Steven Rostedt
2020-07-03 11:31 ` Arnaldo Carvalho de Melo
2020-07-03 16:18 ` Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 02/15] tools lib traceevent: Add proper KBUFFER_TYPE_TIME_STAMP handling Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 03/15] tools lib traceevent: Add tep_load_plugins_hook() API Steven Rostedt
2020-07-07 14:55 ` Namhyung Kim
2020-07-07 15:35 ` Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 04/15] tools lib traceevent: Add interface for options to plugins Steven Rostedt
2020-07-07 15:03 ` Namhyung Kim [this message]
2020-07-02 18:53 ` [PATCH v2 05/15] tools lib traceevent: Introduced new traceevent API, for adding new plugins directories Steven Rostedt
2020-07-07 15:06 ` Namhyung Kim
2020-07-07 15:24 ` Steven Rostedt
2020-07-08 0:02 ` Namhyung Kim
2020-07-02 18:53 ` [PATCH v2 06/15] tools lib traceevent: Add support for more printk format specifiers Steven Rostedt
2020-07-07 15:09 ` Namhyung Kim
2020-07-02 18:53 ` [PATCH v2 07/15] tools lib traceevent: Optimize pretty_print() function Steven Rostedt
2020-07-07 15:11 ` Namhyung Kim
2020-07-07 15:31 ` Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 08/15] tools lib traceevent: Add plugin for tlb_flush Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 09/15] tools lib traceevent: Add more SVM exit reasons Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 10/15] tools lib traceevent: Add offset option for function plugin Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 11/15] tools lib traceevent: Add plugin for decoding syscalls/sys_enter_futex Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 12/15] tools lib traceevent: Move kernel_stack event handler to "function" plugin Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 13/15] tools lib traceevent: Add builtin handler for trace_marker_raw Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 14/15] tools lib traceevent: Change to SPDX License format Steven Rostedt
2020-07-02 18:53 ` [PATCH v2 15/15] tools lib traceevent: Fix reporting of unknown SVM exit reasons Steven Rostedt
2020-07-08 2:02 ` [PATCH v2 00/15] tools lib traceevent: Patches from the trace-cmd repo Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAM9d7cizjF+fbK7YzmsBDgrx__4YAOsmEq67D3sWET8FF+YdFA@mail.gmail.com \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=jolsa@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-trace-devel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=rostedt@goodmis.org \
--cc=tz.stoyanov@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.