On Wed, 23 Dec 2020 17:17:27 -0600
Tom Zanussi <zanu...@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 <zanu...@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 <mhira...@kernel.org>
