Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 24 Dec 2020 17:21:47 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v5 2/5] tracing: Rework synthetic event command parsing |
| |
On Wed, 23 Dec 2020 17:17:27 -0600 Tom Zanussi <zanussi@kernel.org> wrote:
> Hi Masami, > > On Tue, 2020-12-22 at 21:42 +0900, Masami Hiramatsu wrote: > > Hi Tom, > > > > On Mon, 21 Dec 2020 15:44:28 -0600 > > Tom Zanussi <zanussi@kernel.org> wrote: > > > > > > > @@ -656,7 +651,6 @@ static struct synth_field > > > *parse_synth_field(int argc, const char **argv, > > > > > > size = synth_field_size(field->type); > > > if (size < 0) { > > > - synth_err(SYNTH_ERR_INVALID_TYPE, errpos(field_type)); > > > > Why did you remove this error message? > > It wasn't actually removed - it was just moved into the next patch, so > is still there. I'll move it back here to avoid confusion. > > > > > [..] > > > @@ -1228,26 +1189,47 @@ 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; > > > + tmp_fields = saved_fields = kstrdup(raw_fields, GFP_KERNEL); > > > + if (!tmp_fields) { > > > + ret = -ENOMEM; > > > + goto out; > > > + } > > > + > > > + while ((field_str = strsep(&tmp_fields, ";")) != NULL) { > > > if (n_fields == SYNTH_FIELDS_MAX) { > > > synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0); > > > ret = -EINVAL; > > > goto err; > > > } > > > > > > - field = parse_synth_field(argc - i, &argv[i], > > > &consumed); > > > + argv = argv_split(GFP_KERNEL, field_str, &argc); > > > + if (!argv) { > > > + ret = -ENOMEM; > > > + goto err; > > > + } > > > + > > > + if (!argc) > > > + continue; > > > + > > > + field = parse_synth_field(argc, argv, &consumed); > > > if (IS_ERR(field)) { > > > + argv_free(argv); > > > ret = PTR_ERR(field); > > > goto err; > > > } > > > + > > > + argv_free(argv); > > > + > > > + if (consumed < argc) { > > > + ret = -EINVAL; > > > + goto err; > > > + } > > > > You can check the consumed < argc in parse_synth_field(), unless > > you keep the backward compatibility - I think you can add an > > inner loop for it, something like > > > > while ((field_str = strsep(&tmp_fields, ";")) != NULL) { > > argv = argv_split(...); > > consumed = 0; > > while (argc > consumed) { > > // increment consumed in parse_synth_field() > > field = parse_synth_field(argc - consumed, argv + consumed, > > &consumed); > > if (IS_ERR(field)) {...} > > > > fields[n_fields++] = field; > > if (n_fields == SYNTH_FIELDS_MAX) {...} > > } > > > > argv_free(argv); > > } > > > > what would you think? > > Hmm, not sure this helps - there's only supposed to be one field per > field_str and consumed returns either 2 or 3 depending on the field.
I meant that this can be reused to support older syntax like "myevent int foo unsigned long bar" without inserting semicolons.
> consumed is only used to detect whether there were unused words and if > so flag an error, rather than loop around to try to get another field.
If you drop the backward compatibility from *this* patch, we can just check the "consumed < argc" in parse_synth_field() and remove "consumed" from this function.
Thank you,
-- Masami Hiramatsu <mhiramat@kernel.org>
| ![\](/images/icornerr.gif) |