Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <48d21108d635e707ef135cac7fa0eaabc1cf5e9a.camel@kernel.org>
Subject: Re: [PATCH v5 2/5] tracing: Rework synthetic event command parsing
From:   Tom Zanussi <zanussi@kernel.org>
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     rostedt@goodmis.org, axelrasmussen@google.com,
        dan.carpenter@oracle.com, linux-kernel@vger.kernel.org
Date:   Wed, 23 Dec 2020 17:17:27 -0600
In-Reply-To: <20201222214259.464311df07a343de821db568@kernel.org>
References: <cover.1608586464.git.zanussi@kernel.org>
         <cc39d3737ffa8b330c3d3754b062709dbc1c50f6.1608586464.git.zanussi@kernel.org>
         <20201222214259.464311df07a343de821db568@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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. 
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.

> 
> > +
> >  		fields[n_fields++] = field;
> > -		i += consumed - 1;
> >  	}
> >  
> > -	if (i < argc && strcmp(argv[i], ";") != 0) {
> > -		synth_err(SYNTH_ERR_INVALID_FIELD, errpos(argv[i]));
> > +	if (n_fields == 0) {
> > +		synth_err(SYNTH_ERR_CMD_INCOMPLETE, 0);
> >  		ret = -EINVAL;
> >  		goto err;
> >  	}
> > @@ -1266,6 +1248,8 @@ static int __create_synth_event(int argc,
> > const char *name, const char **argv)
> >   out:
> >  	mutex_unlock(&event_mutex);
> >  
> > +	kfree(saved_fields);
> > +
> >  	return ret;
> >   err:
> >  	for (i = 0; i < n_fields; i++)
> > @@ -1385,29 +1369,35 @@ EXPORT_SYMBOL_GPL(synth_event_delete);
> >  
> >  static int create_or_delete_synth_event(const char *raw_command)
> >  {
> > -	char **argv, *name = NULL;
> > -	int argc = 0, ret = 0;
> > +	char *name = NULL, *fields, *p;
> > +	int ret = 0;
> >  
> > -	argv = argv_split(GFP_KERNEL, raw_command, &argc);
> > -	if (!argv)
> > -		return -ENOMEM;
> > +	raw_command = skip_spaces(raw_command);
> > +	if (raw_command[0] == '\0')
> > +		return ret;
> >  
> > -	if (!argc)
> > -		goto free;
> > +	last_cmd_set(raw_command);
> >  
> > -	name = argv[0];
> > +	p = strpbrk(raw_command, " \t");
> > +	if (!p)
> > +		return -EINVAL;
> 
> Hmm, this may drop the ability to delete an event with "!name",
> it always requires some spaces after the name.
> 

Yes, good point, will fix that and also add a test case for just !name.

Thanks,

Tom

> Thank you,
> 
>