Re: [PATCHv2 2/3] tracing - buf iterator support for set_event

[Steven Rostedt <rostedt@goodmis.org>]

On Sat, 2009-09-05 at 03:13 +0200, jolsa@redhat.com wrote:
> Updated 'set_event' write method to use the 'trace_get_user' function.
> 
> wbr,
> jirka
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> 
> ---
>  kernel/trace/trace_events.c |   58 +++++++++---------------------------------
>  1 files changed, 13 insertions(+), 45 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index d33bcde..7e5703c 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -230,11 +230,9 @@ static ssize_t
>  ftrace_event_write(struct file *file, const char __user *ubuf,
>  		   size_t cnt, loff_t *ppos)
>  {
> +	struct trace_biter biter;

s/biter/parser/g

>  	size_t read = 0;
> -	int i, set = 1;
>  	ssize_t ret;
> -	char *buf;
> -	char ch;
>  
>  	if (!cnt || cnt < 0)
>  		return 0;
> @@ -243,60 +241,30 @@ ftrace_event_write(struct file *file, const char __user *ubuf,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = get_user(ch, ubuf++);
> -	if (ret)
> -		return ret;
> -	read++;
> -	cnt--;
> -
> -	/* skip white space */
> -	while (cnt && isspace(ch)) {
> -		ret = get_user(ch, ubuf++);
> -		if (ret)
> -			return ret;
> -		read++;
> -		cnt--;
> -	}
> -
> -	/* Only white space found? */
> -	if (isspace(ch)) {
> -		file->f_pos += read;
> -		ret = read;
> -		return ret;
> -	}
> -
> -	buf = kmalloc(EVENT_BUF_SIZE+1, GFP_KERNEL);
> -	if (!buf)
> +	if (trace_biter_alloc(&biter, EVENT_BUF_SIZE + 1))
>  		return -ENOMEM;
>  
> -	if (cnt > EVENT_BUF_SIZE)
> -		cnt = EVENT_BUF_SIZE;
> +	read = trace_get_user(&biter, ubuf, cnt, ppos);
> +
> +	if (TRACE_BITER_LOADED((&biter))) {
> +		char *buf = biter.buffer;

If you have encapsulated the access to parser with trace_parser_loaded,
might as well do the same for getting the buffer:

static inline char *trace_parser_buffer(struct trace_parser *parser)
{
	return parser->buffer;
}


> +		int set = 1;
>  
> -	i = 0;
> -	while (cnt && !isspace(ch)) {
> -		if (!i && ch == '!')
> +		if (*buf == '!') {
>  			set = 0;
> -		else
> -			buf[i++] = ch;
> +			buf++;

I had a funny trick in my patches to instead of incrementing buf here, I
passed in below a:

	ftrace_set_clr_event(buf + !!set, set);

But whatever ;-)

> +		}
> +		biter.buffer[biter.idx] = 0;

here you are accessing both the buffer and idx directly. Might as well
do it everywhere like this and don't use the helper functions.

-- Steve

>  
> -		ret = get_user(ch, ubuf++);
> +		ret = ftrace_set_clr_event(buf, set);
>  		if (ret)
>  			goto out_free;
> -		read++;
> -		cnt--;
>  	}
> -	buf[i] = 0;
> -
> -	file->f_pos += read;
> -
> -	ret = ftrace_set_clr_event(buf, set);
> -	if (ret)
> -		goto out_free;
>  
>  	ret = read;
>  
>   out_free:
> -	kfree(buf);
> +	trace_biter_free(&biter);
>  
>  	return ret;
>  }