On Fri, 2021-01-22 at 22:16 +0900, Masami Hiramatsu wrote:
> On Thu, 21 Jan 2021 11:01:05 -0600
> Tom Zanussi <zanu...@kernel.org> wrote:
>
> > Now that command parsing has been delegated to the create functions
> > and we're no longer constrained by argv_split(), we can modify the
> > synthetic event command parser to better match the higher-level
> > structure of the synthetic event commands, which is basically an
> > event
> > name followed by a set of semicolon-separated fields.
> >
> > Since we're also now passed the raw command, we can also save it
> > directly and can get rid of save_cmdstr().
> >
>
> This looks good to me.
>
> Reviewed-by: Masami Hiramatsu <mhira...@kernel.org>
>
> Thank you,
>
Thanks, Masami.
Tom
> > Signed-off-by: Tom Zanussi <zanu...@kernel.org>
> > ---
> > kernel/trace/trace_events_synth.c | 198 +++++++++++++++-----------
> > ----
> > 1 file changed, 98 insertions(+), 100 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_synth.c
> > b/kernel/trace/trace_events_synth.c
> > index b2588a5650c9..a79c17b97add 100644
> > --- a/kernel/trace/trace_events_synth.c
> > +++ b/kernel/trace/trace_events_synth.c
> > @@ -48,7 +48,7 @@ static int errpos(const char *str)
> > return err_pos(last_cmd, str);
> > }
> >
> > -static void last_cmd_set(char *str)
> > +static void last_cmd_set(const char *str)
> > {
> > if (!str)
> > return;
> > @@ -579,18 +579,14 @@ static void free_synth_field(struct
> > synth_field *field)
> > kfree(field);
> > }
> >
> > -static struct synth_field *parse_synth_field(int argc, const char
> > **argv,
> > - int *consumed)
> > +static struct synth_field *parse_synth_field(int argc, char
> > **argv)
> > {
> > - struct synth_field *field;
> > const char *prefix = NULL, *field_type = argv[0], *field_name,
> > *array;
> > - int len, ret = -ENOMEM;
> > + int len, consumed, ret = -ENOMEM;
> > + struct synth_field *field;
> > struct seq_buf s;
> > ssize_t size;
> >
> > - if (field_type[0] == ';')
> > - field_type++;
> > -
> > if (!strcmp(field_type, "unsigned")) {
> > if (argc < 3) {
> > synth_err(SYNTH_ERR_INCOMPLETE_TYPE,
> > errpos(field_type));
> > @@ -599,10 +595,20 @@ static struct synth_field
> > *parse_synth_field(int argc, const char **argv,
> > prefix = "unsigned ";
> > field_type = argv[1];
> > field_name = argv[2];
> > - *consumed = 3;
> > + consumed = 3;
> > } else {
> > field_name = argv[1];
> > - *consumed = 2;
> > + consumed = 2;
> > + }
> > +
> > + if (consumed < argc) {
> > + synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + if (!field_name) {
> > + synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
> > + return ERR_PTR(-EINVAL);
> > }
> >
> > field = kzalloc(sizeof(*field), GFP_KERNEL);
> > @@ -613,8 +619,6 @@ static struct synth_field
> > *parse_synth_field(int argc, const char **argv,
> > array = strchr(field_name, '[');
> > if (array)
> > len -= strlen(array);
> > - else if (field_name[len - 1] == ';')
> > - len--;
> >
> > field->name = kmemdup_nul(field_name, len, GFP_KERNEL);
> > if (!field->name)
> > @@ -626,8 +630,6 @@ static struct synth_field
> > *parse_synth_field(int argc, const char **argv,
> > goto free;
> > }
> >
> > - if (field_type[0] == ';')
> > - field_type++;
> > len = strlen(field_type) + 1;
> >
> > if (array)
> > @@ -644,11 +646,8 @@ static struct synth_field
> > *parse_synth_field(int argc, const char **argv,
> > if (prefix)
> > seq_buf_puts(&s, prefix);
> > seq_buf_puts(&s, field_type);
> > - if (array) {
> > + if (array)
> > seq_buf_puts(&s, array);
> > - if (s.buffer[s.len - 1] == ';')
> > - s.len--;
> > - }
> > if (WARN_ON_ONCE(!seq_buf_buffer_left(&s)))
> > goto free;
> >
> > @@ -1160,46 +1159,12 @@ int synth_event_gen_cmd_array_start(struct
> > dynevent_cmd *cmd, const char *name,
> > }
> > EXPORT_SYMBOL_GPL(synth_event_gen_cmd_array_start);
> >
> > -static int save_cmdstr(int argc, const char *name, const char
> > **argv)
> > -{
> > - struct seq_buf s;
> > - char *buf;
> > - int i;
> > -
> > - buf = kzalloc(MAX_DYNEVENT_CMD_LEN, GFP_KERNEL);
> > - if (!buf)
> > - return -ENOMEM;
> > -
> > - seq_buf_init(&s, buf, MAX_DYNEVENT_CMD_LEN);
> > -
> > - seq_buf_puts(&s, name);
> > -
> > - for (i = 0; i < argc; i++) {
> > - seq_buf_putc(&s, ' ');
> > - seq_buf_puts(&s, argv[i]);
> > - }
> > -
> > - if (!seq_buf_buffer_left(&s)) {
> > - synth_err(SYNTH_ERR_CMD_TOO_LONG, 0);
> > - kfree(buf);
> > - return -EINVAL;
> > - }
> > - buf[s.len] = 0;
> > - last_cmd_set(buf);
> > -
> > - kfree(buf);
> > - return 0;
> > -}
> > -
> > -static int __create_synth_event(int argc, const char *name, const
> > char **argv)
> > +static int __create_synth_event(const char *name, const char
> > *raw_fields)
> > {
> > + char **argv, *field_str, *tmp_fields, *saved_fields = NULL;
> > struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
> > + int i, argc, n_fields = 0, ret = 0;
> > struct synth_event *event = NULL;
> > - int i, consumed = 0, n_fields = 0, ret = 0;
> > -
> > - ret = save_cmdstr(argc, name, argv);
> > - if (ret)
> > - return ret;
> >
> > /*
> > * Argument syntax:
> > @@ -1208,13 +1173,14 @@ static int __create_synth_event(int argc,
> > const char *name, const char **argv)
> > * where 'field' = type field_name
> > */
> >
> > - if (name[0] == '\0' || argc < 1) {
> > + mutex_lock(&event_mutex);
> > +
> > + if (name[0] == '\0') {
> > synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> >
> > - mutex_lock(&event_mutex);
> > -
> > if (!is_good_name(name)) {
> > synth_err(SYNTH_ERR_BAD_NAME, errpos(name));
> > ret = -EINVAL;
> > @@ -1228,26 +1194,41 @@ static int __create_synth_event(int argc,
> > const char *name, const char **argv)
> > goto out;
> > }
> >
> > - for (i = 0; i < argc - 1; i++) {
> > - if (strcmp(argv[i], ";") == 0)
> > - continue;
> > - if (n_fields == SYNTH_FIELDS_MAX) {
> > - synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
> > - ret = -EINVAL;
> > + tmp_fields = saved_fields = kstrdup(raw_fields, GFP_KERNEL);
> > + if (!tmp_fields) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + while ((field_str = strsep(&tmp_fields, ";")) != NULL) {
> > + argv = argv_split(GFP_KERNEL, field_str, &argc);
> > + if (!argv) {
> > + ret = -ENOMEM;
> > goto err;
> > }
> >
> > - field = parse_synth_field(argc - i, &argv[i],
> > &consumed);
> > + if (!argc)
> > + continue;
> > +
> > + field = parse_synth_field(argc, argv);
> > if (IS_ERR(field)) {
> > + argv_free(argv);
> > ret = PTR_ERR(field);
> > goto err;
> > }
> > +
> > + argv_free(argv);
> > +
> > fields[n_fields++] = field;
> > - i += consumed - 1;
> > + if (n_fields == SYNTH_FIELDS_MAX) {
> > + synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > }
> >
> > - if (i < argc && strcmp(argv[i], ";") != 0) {
> > - synth_err(SYNTH_ERR_INVALID_FIELD, errpos(argv[i]));
> > + if (n_fields == 0) {
> > + synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
> > ret = -EINVAL;
> > goto err;
> > }
> > @@ -1266,6 +1247,8 @@ static int __create_synth_event(int argc,
> > const char *name, const char **argv)
> > out:
> > mutex_unlock(&event_mutex);
> >
> > + kfree(saved_fields);
> > +
> > return ret;
> > err:
> > for (i = 0; i < n_fields; i++)
> > @@ -1385,29 +1368,38 @@ EXPORT_SYMBOL_GPL(synth_event_delete);
> >
> > static int create_or_delete_synth_event(const char *raw_command)
> > {
> > - char **argv, *name = NULL;
> > - int argc = 0, ret = 0;
> > + char *name = NULL, *fields, *p;
> > + int ret = 0;
> >
> > - argv = argv_split(GFP_KERNEL, raw_command, &argc);
> > - if (!argv)
> > - return -ENOMEM;
> > + raw_command = skip_spaces(raw_command);
> > + if (raw_command[0] == '\0')
> > + return ret;
> > +
> > + last_cmd_set(raw_command);
> >
> > - if (!argc)
> > + p = strpbrk(raw_command, " \t");
> > + if (!p && raw_command[0] != '!') {
> > + synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
> > + ret = -EINVAL;
> > goto free;
> > + }
> >
> > - name = argv[0];
> > + name = kmemdup_nul(raw_command, p ? p - raw_command :
> > strlen(raw_command), GFP_KERNEL);
> > + if (!name)
> > + return -ENOMEM;
> >
> > - /* trace_run_command() ensures argc != 0 */
> > if (name[0] == '!') {
> > ret = synth_event_delete(name + 1);
> > goto free;
> > }
> >
> > - ret = __create_synth_event(argc - 1, name, (const char **)argv
> > + 1);
> > + fields = skip_spaces(p);
> > +
> > + ret = __create_synth_event(name, fields);
> > free:
> > - argv_free(argv);
> > + kfree(name);
> >
> > - return ret == -ECANCELED ? -EINVAL : ret;
> > + return ret;
> > }
> >
> > static int synth_event_run_command(struct dynevent_cmd *cmd)
> > @@ -1953,39 +1945,45 @@ EXPORT_SYMBOL_GPL(synth_event_trace_end);
> >
> > static int create_synth_event(const char *raw_command)
> > {
> > - char **argv, *name;
> > - int len, argc = 0, ret = 0;
> > + char *fields, *p;
> > + const char *name;
> > + int len, ret = 0;
> >
> > - argv = argv_split(GFP_KERNEL, raw_command, &argc);
> > - if (!argv) {
> > - ret = -ENOMEM;
> > + raw_command = skip_spaces(raw_command);
> > + if (raw_command[0] == '\0')
> > return ret;
> > - }
> >
> > - if (!argc)
> > - goto free;
> > + last_cmd_set(raw_command);
> >
> > - name = argv[0];
> > + p = strpbrk(raw_command, " \t");
> > + if (!p)
> > + return -EINVAL;
> >
> > - if (name[0] != 's' || name[1] != ':') {
> > - ret = -ECANCELED;
> > - goto free;
> > - }
> > + fields = skip_spaces(p);
> > +
> > + name = raw_command;
> > +
> > + if (name[0] != 's' || name[1] != ':')
> > + return -ECANCELED;
> > name += 2;
> >
> > /* This interface accepts group name prefix */
> > if (strchr(name, '/')) {
> > len = str_has_prefix(name, SYNTH_SYSTEM "/");
> > - if (len == 0) {
> > - ret = -EINVAL;
> > - goto free;
> > - }
> > + if (len == 0)
> > + return -EINVAL;
> > name += len;
> > }
> >
> > - ret = __create_synth_event(argc - 1, name, (const char **)argv
> > + 1);
> > -free:
> > - argv_free(argv);
> > + len = name - raw_command;
> > +
> > + name = kmemdup_nul(raw_command + len, p - raw_command - len,
> > GFP_KERNEL);
> > + if (!name)
> > + return -ENOMEM;
> > +
> > + ret = __create_synth_event(name, fields);
> > +
> > + kfree(name);
> >
> > return ret;
> > }
> > --
> > 2.17.1
> >
>
>
