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

[Steven Rostedt]

On Thu, 21 Jan 2021 11:01:07 -0600
Tom Zanussi <zanussi@xxxxxxxxxx> 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.

> +	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) {