Re: [PATCH v6 3/6] tracing: Update synth command errors

[Tom Zanussi <zanussi@kernel.org>]

On Fri, 2021-01-22 at 22:23 +0900, Masami Hiramatsu wrote:
> Hi Tom,
> 
> On Thu, 21 Jan 2021 11:01:06 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > Since array types are handled differently, errors referencing them
> > also need to be handled differently.  Add and use a new
> > INVALID_ARRAY_SPEC error.  Also add INVALID_CMD and INVALID_DYN_CMD
> > to
> > catch and display the correct form for badly-formed commands, which
> > can also be used in place of CMD_INCOMPLETE, which is removed, and
> > remove CMD_TOO_LONG, since it's no longer used.
> 
> OK, so this will add new errors for precise error logging.
> 
> > 
> > Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> > ---
> >  kernel/trace/trace_events_synth.c | 72
> > +++++++++++++++++++++++++++----
> >  1 file changed, 63 insertions(+), 9 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_synth.c
> > b/kernel/trace/trace_events_synth.c
> > index a79c17b97add..dd141ee6b3fc 100644
> > --- a/kernel/trace/trace_events_synth.c
> > +++ b/kernel/trace/trace_events_synth.c
> > @@ -23,13 +23,14 @@
> >  #undef ERRORS
> >  #define ERRORS	\
> >  	C(BAD_NAME,		"Illegal name"),		\
> > -	C(CMD_INCOMPLETE,	"Incomplete command"),		\
> > +	C(INVALID_CMD,		"Command must be of the form:
> > <name> field[;field] ..."),\
> > +	C(INVALID_DYN_CMD,	"Command must be of the form: s or
> > -:[synthetic/]<name> field[;field] ..."),\
> >  	C(EVENT_EXISTS,		"Event already exists"),	\
> >  	C(TOO_MANY_FIELDS,	"Too many fields"),		\
> >  	C(INCOMPLETE_TYPE,	"Incomplete type"),		\
> >  	C(INVALID_TYPE,		"Invalid type"),		\
> > -	C(INVALID_FIELD,	"Invalid field"),		\
> > -	C(CMD_TOO_LONG,		"Command too long"),
> > +	C(INVALID_FIELD,        "Invalid field"),		\
> > +	C(INVALID_ARRAY_SPEC,	"Invalid array specification"),
> >  
> >  #undef C
> >  #define C(a, b)		SYNTH_ERR_##a
> > @@ -655,7 +656,10 @@ static struct synth_field
> > *parse_synth_field(int argc, char **argv)
> >  
> >  	size = synth_field_size(field->type);
> >  	if (size < 0) {
> > -		synth_err(SYNTH_ERR_INVALID_TYPE, errpos(field_type));
> > +		if (array)
> > +			synth_err(SYNTH_ERR_INVALID_ARRAY_SPEC,
> > errpos(field_name));
> > +		else
> > +			synth_err(SYNTH_ERR_INVALID_TYPE,
> > errpos(field_type));
> >  		ret = -EINVAL;
> >  		goto free;
> >  	} else if (size == 0) {
> > @@ -1176,7 +1180,7 @@ static int __create_synth_event(const char
> > *name, const char *raw_fields)
> >  	mutex_lock(&event_mutex);
> >  
> >  	if (name[0] == '\0') {
> > -		synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
> > +		synth_err(SYNTH_ERR_INVALID_CMD, 0);
> >  		ret = -EINVAL;
> >  		goto out;
> >  	}
> > @@ -1228,7 +1232,7 @@ static int __create_synth_event(const char
> > *name, const char *raw_fields)
> >  	}
> >  
> >  	if (n_fields == 0) {
> > -		synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
> > +		synth_err(SYNTH_ERR_INVALID_CMD, 0);
> >  		ret = -EINVAL;
> >  		goto err;
> >  	}
> > @@ -1366,6 +1370,40 @@ int synth_event_delete(const char
> > *event_name)
> >  }
> >  EXPORT_SYMBOL_GPL(synth_event_delete);
> >  
> > +static int check_command(const char *raw_command)
> > +{
> > +	char **argv = NULL, *cmd, *saved_cmd, *name_and_field;
> > +	int argc, ret = 0;
> > +
> > +	cmd = saved_cmd = kstrdup(raw_command, GFP_KERNEL);
> > +	if (!cmd)
> > +		return -ENOMEM;
> > +
> > +	name_and_field = strsep(&cmd, ";");
> > +	if (!name_and_field) {
> > +		ret = -EINVAL;
> > +		goto free;
> > +	}
> > +
> > +	if (name_and_field[0] == '!')
> > +		goto free;
> > +
> > +	argv = argv_split(GFP_KERNEL, name_and_field, &argc);
> > +	if (!argv) {
> > +		ret = -ENOMEM;
> > +		goto free;
> > +	}
> > +
> > +	if (argc < 3)
> > +		ret = -EINVAL;
> > +free:
> > +	kfree(saved_cmd);
> > +	if (argv)
> > +		argv_free(argv);
> > +
> > +	return ret;
> > +}
> 
> But I'm not sure why this (yet another parser) is needed. What you
> are expecting
> for this check_command()? Could you tell me some examples?
> 

This is just a common function to check we even have the possibility of
a valid command.  It will catch things right off the top like

  # echo 'myevent' >> synthetic_events

or

  # echo 'myevent int'  >> synthetic events

You're right, it's confusing to have it in this patch, since it really
belongs in the rework patch - I'll move it there.

Thanks,

Tom


> Thank you,
> 
> 
>