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

[Steven Rostedt]

On Mon, 19 May 2014 23:29:35 +0900
Namhyung Kim <namhyung@xxxxxxxxxx> 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>:<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?

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);
> > +}
> 

--
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/