Re: [PATCH 2/4] tools lib traceevent: Add options to plugins

Steven Rostedt
2014-06-03T03:12:16Z

On Mon, 19 May 2014 23:29:35 +0900
Namhyung Kim <namhyung@kernel.org> wrote:

> > - * 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?

Hmm, no idea why I changed that.

> > +/**
> > + * 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>: . 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?

Yeah, I could do this.

>
> > + *
> > + * 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.

Will fix.

>
>
> > + 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?

Yep, will do.

>
> > +
> > + err:
> > + while (--count > 0)
>
> Shouldn't it be >= instead of > ?
>

Fixed.

Thanks,

-- Steve

> Thanks,
> Namhyung
>
>
> > + free(list[count]);
> > + free(list);
> > +
> > + return (char **)((unsigned long)-1);
> > +}
>