Re: [PATCH v6 4/6] tracing: Add a backward-compatibility check for synthetic event creation

[Tom Zanussi <zanussi@kernel.org>]

Hi Steve,

On Fri, 2021-01-22 at 16:12 -0500, Steven Rostedt wrote:
> On Thu, 21 Jan 2021 11:01:07 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> > --- a/kernel/trace/trace_events_synth.c
> > +++ b/kernel/trace/trace_events_synth.c
> > @@ -580,11 +580,18 @@ static void free_synth_field(struct
> > synth_field *field)
> >  	kfree(field);
> >  }
> >  
> > -static struct synth_field *parse_synth_field(int argc, char
> > **argv)
> > +static int check_field_version(const char *prefix, const char
> > *field_type,
> > +			       const char *field_name)
> > +{
> 
> This needs a comment:
> 
> 	/*
> 	 * For backward compatibility, the old format did not require
> 	 * semicolons, and not to break user space, that old format
> must
> 	 * still work. If a new feature is added, then the format that
> uses
> 	 * the new feature will be required to have semicolons, as
> nothing
> 	 * that uses the old format would be using the new, yet to be
> 	 * created, feature. When a new feature is added, this will
> detect
> 	 * it, and return a number greater than 1, and require the
> format
> 	 * to use semicolons.
> 	 */
> 
> Or something to that effect.
> 

OK, will add better comments in the code about how this is meant to
work.

Thanks,

Tom

> > +	return 1;
> > +}
> > +
> > +static struct synth_field *parse_synth_field(int argc, char
> > **argv,
> > +					     int *consumed, int
> > *field_version)
> >  {
> >  	const char *prefix = NULL, *field_type = argv[0], *field_name,
> > *array;
> > -	int len, consumed, ret = -ENOMEM;
> >  	struct synth_field *field;
> > +	int len, ret = -ENOMEM;
> >  	struct seq_buf s;
> >  	ssize_t size;
> >  
> > @@ -596,15 +603,10 @@ static struct synth_field
> > *parse_synth_field(int argc, char **argv)
> >  		prefix = "unsigned ";
> >  		field_type = argv[1];
> >  		field_name = argv[2];
> > -		consumed = 3;
> > +		*consumed += 3;
> >  	} else {
> >  		field_name = argv[1];
> > -		consumed = 2;
> > -	}
> > -
> > -	if (consumed < argc) {
> > -		synth_err(SYNTH_ERR_INVALID_FIELD, errpos(field_type));
> > -		return ERR_PTR(-EINVAL);
> > +		*consumed += 2;
> >  	}
> >  
> >  	if (!field_name) {
> > @@ -612,6 +614,8 @@ static struct synth_field
> > *parse_synth_field(int argc, char **argv)
> >  		return ERR_PTR(-EINVAL);
> >  	}
> >  
> > +	*field_version = check_field_version(prefix, field_type,
> > field_name);
> > +
> >  	field = kzalloc(sizeof(*field), GFP_KERNEL);
> >  	if (!field)
> >  		return ERR_PTR(-ENOMEM);
> > @@ -1167,6 +1171,7 @@ 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 consumed, cmd_version = 1, n_fields_this_loop;
> >  	int i, argc, n_fields = 0, ret = 0;
> >  	struct synth_event *event = NULL;
> >  
> > @@ -1214,21 +1219,46 @@ static int __create_synth_event(const char
> > *name, const char *raw_fields)
> >  		if (!argc)
> >  			continue;
> >  
> > -		field = parse_synth_field(argc, argv);
> > -		if (IS_ERR(field)) {
> > -			argv_free(argv);
> > -			ret = PTR_ERR(field);
> > -			goto err;
> > -		}
> > +		n_fields_this_loop = 0;
> > +		consumed = 0;
> > +		while (argc > consumed) {
> > +			int field_version;
> > +
> > +			field = parse_synth_field(argc - consumed,
> > +						  argv + consumed,
> > &consumed,
> > +						  &field_version);
> > +			if (IS_ERR(field)) {
> > +				argv_free(argv);
> > +				ret = PTR_ERR(field);
> > +				goto err;
> > +			}
> >  
> > -		argv_free(argv);
> > +			if (field_version > cmd_version)
> > +				cmd_version = field_version;
> > +
> 
> There needs to be some comments here to explain what the versioning
> means.
> 
> -- Steve
> 
> 
> > +			if (cmd_version > 1 && n_fields_this_loop >= 1)
> > {
> > +				synth_err(SYNTH_ERR_INVALID_CMD,
> > errpos(field_str));
> > +				ret = -EINVAL;
> > +				goto err;
> > +			}
> > +
> > +			fields[n_fields++] = field;
> > +			if (n_fields == SYNTH_FIELDS_MAX) {
> > +				synth_err(SYNTH_ERR_TOO_MANY_FIELDS,
> > 0);
> > +				ret = -EINVAL;
> > +				goto err;
> > +			}
> > +
> > +			n_fields_this_loop++;
> > +		}
> >  
> > -		fields[n_fields++] = field;
> > -		if (n_fields == SYNTH_FIELDS_MAX) {
> > -			synth_err(SYNTH_ERR_TOO_MANY_FIELDS, 0);
> > +		if (consumed < argc) {
> > +			synth_err(SYNTH_ERR_INVALID_CMD, 0);
> >  			ret = -EINVAL;
> >  			goto err;
> >  		}
> > +
> > +		argv_free(argv);
> >  	}
> >  
> >  	if (n_fields == 0) {
> 
>