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