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

Steven Rostedt
2009-09-10T20:35:33Z

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;
> }