Re: [PATCH v5 2/5] tracing: Rework synthetic event command parsing

[Masami Hiramatsu <mhiramat@kernel.org>]

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>