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

[Masami Hiramatsu <mhiramat@kernel.org>]

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?

Thank you,



-- 
Masami Hiramatsu <mhiramat@kernel.org>