Hi Tom,
On Thu, 21 Jan 2021 11:01:06 -0600
Tom Zanussi <zanu...@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 <zanu...@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 <mhira...@kernel.org>
