Messages in this thread | | | Subject | Re: [PATCH v6 4/6] tracing: Add a backward-compatibility check for synthetic event creation | From | Tom Zanussi <> | Date | Mon, 25 Jan 2021 10:25:02 -0600 |
| |
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) { > >
| |