[PATCH 0/3] tracing - adding common method for reading/parsing user input

[PATCH 0/3] tracing - adding common method for reading/parsing user input

[jolsa]

As 3 different places for 4 user input files were using the same way
of reading and parsing user's input, I made one common routine to be
used. 

The mentioned files are:
	set_graph_function
	set_event
	set_ftrace_filter
	set_ftrace_notrace

wbr,
jirka

Signed-off-by: Jiri Olsa <jolsa@redhat.com>

---
 kernel/trace/ftrace.c       |  152 ++++++++++++-------------------------------
 kernel/trace/trace.c        |   83 +++++++++++++++++++++++
 kernel/trace/trace.h        |   48 ++++++++++++++
 kernel/trace/trace_events.c |   58 ++++-------------
 4 files changed, 186 insertions(+), 155 deletions(-)

[PATCH 0/3] tracing - adding common method for reading/parsing user input

[jolsa]

Defined 'trace_biter' structure to carry the properties of the input,
through the multiple writes.

The 'trace_get_user' reads the user incput string separated by space
(matched by isspace(ch). For each string found the 'struct trace_biter' 
iterator is updated, and the function returns.

wbr,
jirka

Signed-off-by: Jiri Olsa <jolsa@redhat.com>

---
 kernel/trace/trace.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/trace/trace.h |   48 +++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+), 0 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index af15008..8cf8f99 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -335,6 +335,89 @@ static struct {
 
 int trace_clock_id;
 
+/*
+ * trace_get_user - reads the user incput string separated by  space
+ * (matched by isspace(ch))
+ *
+ * For each string found the 'struct trace_biter' iterator is updated,
+ * and the function returns.
+ *
+ * Returns number of bytes read.
+ *
+ * see kernel/trace/trace.h for 'struct trace_biter' details.
+ */
+int trace_get_user(struct trace_biter *biter, const char __user *ubuf,
+	size_t cnt, loff_t *ppos)
+{
+	char ch;
+	size_t read = 0;
+	ssize_t ret;
+
+	if (!*ppos)
+		TRACE_BITER_CLEAR(biter);
+
+	ret = get_user(ch, ubuf++);
+	if (ret)
+		goto out;
+
+	read++;
+	cnt--;
+
+	/*
+	 * If the parser haven't finished with the last write,
+	 * continue reading the user input without skipping spaces.
+	 */
+	if (!biter->cont) {
+		/* skip white space */
+		while (cnt && isspace(ch)) {
+			ret = get_user(ch, ubuf++);
+			if (ret)
+				goto out;
+			read++;
+			cnt--;
+		}
+
+		/* only spaces were written */
+		if (isspace(ch)) {
+			*ppos += read;
+			ret = read;
+			goto out;
+		}
+
+		biter->idx = 0;
+	}
+
+	/* read the non-space input */
+	while (cnt && !isspace(ch)) {
+		if (biter->idx < biter->size)
+			biter->buffer[biter->idx++] = ch;
+		else {
+			ret = -EINVAL;
+			goto out;
+		}
+		ret = get_user(ch, ubuf++);
+		if (ret)
+			goto out;
+		read++;
+		cnt--;
+	}
+
+	/* We either got finished input or we have to wait for another call. */
+	if (isspace(ch)) {
+		biter->buffer[biter->idx] = 0;
+		biter->cont = 0;
+	} else {
+		biter->cont = 1;
+		biter->buffer[biter->idx++] = ch;
+	}
+
+	*ppos += read;
+	ret = read;
+
+out:
+	return ret;
+}
+
 ssize_t trace_seq_to_user(struct trace_seq *s, char __user *ubuf, size_t cnt)
 {
 	int len;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index d881015..d4e38e8 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -652,6 +652,54 @@ static inline int ftrace_trace_task(struct task_struct *task)
 #endif
 
 /*
+ * struct trace_biter - servers for reading the user input separated by spaces
+ * @cont: set if the input is not complete - no final space char was found
+ * @buffer: holds the parsed user input
+ * @idx: user input lenght
+ * @size: buffer size
+ */
+struct trace_biter {
+	bool		cont;
+	char		*buffer;
+	unsigned	idx;
+	unsigned	size;
+};
+
+#define TRACE_BITER_LOADED(biter) (biter->idx)
+#define TRACE_BITER_CONT(biter)   (biter->cont)
+#define TRACE_BITER_CLEAR(biter) \
+do { \
+	biter->cont = false; \
+	biter->idx = 0; \
+} while(0)
+
+/*
+ * trace_biter_alloc - allocates the buffer for trace buffer iterator
+ */
+static inline int trace_biter_alloc(struct trace_biter *biter, int size)
+{
+	memset(biter, 0, sizeof(*biter));
+
+	biter->buffer = kmalloc(size, GFP_KERNEL);
+	if (!biter->buffer)
+		return 1;
+
+	biter->size = size;
+	return 0;
+}
+
+/*
+ * trace_biter_free - frees the buffer for trace buffer iterator
+ */
+static inline void trace_biter_free(struct trace_biter *biter)
+{
+	kfree(biter->buffer);
+}
+
+extern int trace_get_user(struct trace_biter *biter, const char __user *ubuf,
+	size_t cnt, loff_t *ppos);
+
+/*
  * trace_iterator_flags is an enumeration that defines bit
  * positions into trace_flags that controls the output.
  *
-- 
1.6.2.5

[PATCH 0/3] tracing - adding common method for reading/parsing user input

[jolsa]

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;
 	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;
+		int set = 1;
 
-	i = 0;
-	while (cnt && !isspace(ch)) {
-		if (!i && ch == '!')
+		if (*buf == '!') {
 			set = 0;
-		else
-			buf[i++] = ch;
+			buf++;
+		}
+		biter.buffer[biter.idx] = 0;
 
-		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;
 }
-- 
1.6.2.5

[PATCH 0/3] tracing - adding common method for reading/parsing user input

[jolsa]

Updated 'set_graph_function', 'set_ftrace_filter', 'set_ftrace_notrace' write methods
to use the 'trace_get_user' function.

Removed FTRACE_ITER_CONT flag, since it's not needed after this change.

Fixed minor in set_graph_function display - g_show function.

wbr,
jirka

Signed-off-by: Jiri Olsa <jolsa@redhat.com>

---
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 8c804e2..04bffba 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1323,11 +1323,10 @@ static int __init ftrace_dyn_table_alloc(unsigned long num_to_init)
 
 enum {
 	FTRACE_ITER_FILTER	= (1 << 0),
-	FTRACE_ITER_CONT	= (1 << 1),
-	FTRACE_ITER_NOTRACE	= (1 << 2),
-	FTRACE_ITER_FAILURES	= (1 << 3),
-	FTRACE_ITER_PRINTALL	= (1 << 4),
-	FTRACE_ITER_HASH	= (1 << 5),
+	FTRACE_ITER_NOTRACE	= (1 << 1),
+	FTRACE_ITER_FAILURES	= (1 << 2),
+	FTRACE_ITER_PRINTALL	= (1 << 3),
+	FTRACE_ITER_HASH	= (1 << 4),
 };
 
 #define FTRACE_BUFF_MAX (KSYM_SYMBOL_LEN+4) /* room for wildcards */
@@ -1337,8 +1336,7 @@ struct ftrace_iterator {
 	int			hidx;
 	int			idx;
 	unsigned		flags;
-	unsigned char		buffer[FTRACE_BUFF_MAX+1];
-	unsigned		buffer_idx;
+	struct trace_biter	biter;
 };
 
 static void *
@@ -1604,6 +1602,11 @@ ftrace_regex_open(struct inode *inode, struct file *file, int enable)
 	if (!iter)
 		return -ENOMEM;
 
+	if (trace_biter_alloc(&iter->biter, FTRACE_BUFF_MAX)) {
+		kfree(iter);
+		return -ENOMEM;
+	}
+
 	mutex_lock(&ftrace_regex_lock);
 	if ((file->f_mode & FMODE_WRITE) &&
 	    (file->f_flags & O_TRUNC))
@@ -2196,9 +2199,8 @@ ftrace_regex_write(struct file *file, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos, int enable)
 {
 	struct ftrace_iterator *iter;
-	char ch;
-	size_t read = 0;
-	ssize_t ret;
+	struct trace_biter *biter;
+	ssize_t ret, read;
 
 	if (!cnt || cnt < 0)
 		return 0;
@@ -2211,72 +2213,23 @@ ftrace_regex_write(struct file *file, const char __user *ubuf,
 	} else
 		iter = file->private_data;
 
-	if (!*ppos) {
-		iter->flags &= ~FTRACE_ITER_CONT;
-		iter->buffer_idx = 0;
-	}
-
-	ret = get_user(ch, ubuf++);
-	if (ret)
-		goto out;
-	read++;
-	cnt--;
+	biter = &iter->biter;
+	read = trace_get_user(biter, ubuf, cnt, ppos);
 
-	/*
-	 * If the parser haven't finished with the last write,
-	 * continue reading the user input without skipping spaces.
-	 */
-	if (!(iter->flags & FTRACE_ITER_CONT)) {
-		/* skip white space */
-		while (cnt && isspace(ch)) {
-			ret = get_user(ch, ubuf++);
-			if (ret)
-				goto out;
-			read++;
-			cnt--;
-		}
-
-		/* only spaces were written */
-		if (isspace(ch)) {
-			*ppos += read;
-			ret = read;
-			goto out;
-		}
-
-		iter->buffer_idx = 0;
-	}
-
-	while (cnt && !isspace(ch)) {
-		if (iter->buffer_idx < FTRACE_BUFF_MAX)
-			iter->buffer[iter->buffer_idx++] = ch;
-		else {
-			ret = -EINVAL;
-			goto out;
-		}
-		ret = get_user(ch, ubuf++);
+	if (TRACE_BITER_LOADED(biter) &&
+	    !TRACE_BITER_CONT(biter)) {
+		ret = ftrace_process_regex(biter->buffer,
+					   biter->idx, enable);
 		if (ret)
 			goto out;
-		read++;
-		cnt--;
-	}
 
-	if (isspace(ch)) {
-		iter->buffer[iter->buffer_idx] = 0;
-		ret = ftrace_process_regex(iter->buffer,
-					   iter->buffer_idx, enable);
-		if (ret)
-			goto out;
-		iter->buffer_idx = 0;
-	} else {
-		iter->flags |= FTRACE_ITER_CONT;
-		iter->buffer[iter->buffer_idx++] = ch;
+		TRACE_BITER_CLEAR(biter);
 	}
 
-	*ppos += read;
 	ret = read;
- out:
-	mutex_unlock(&ftrace_regex_lock);
 
+	mutex_unlock(&ftrace_regex_lock);
+out:
 	return ret;
 }
 
@@ -2381,6 +2334,7 @@ ftrace_regex_release(struct inode *inode, struct file *file, int enable)
 {
 	struct seq_file *m = (struct seq_file *)file->private_data;
 	struct ftrace_iterator *iter;
+	struct trace_biter *biter;
 
 	mutex_lock(&ftrace_regex_lock);
 	if (file->f_mode & FMODE_READ) {
@@ -2390,9 +2344,10 @@ ftrace_regex_release(struct inode *inode, struct file *file, int enable)
 	} else
 		iter = file->private_data;
 
-	if (iter->buffer_idx) {
-		iter->buffer[iter->buffer_idx] = 0;
-		ftrace_match_records(iter->buffer, iter->buffer_idx, enable);
+	biter = &iter->biter;
+	if (TRACE_BITER_LOADED(biter)) {
+		biter->buffer[biter->idx] = 0;
+		ftrace_match_records(biter->buffer, biter->idx, enable);
 	}
 
 	mutex_lock(&ftrace_lock);
@@ -2400,7 +2355,9 @@ ftrace_regex_release(struct inode *inode, struct file *file, int enable)
 		ftrace_run_update_code(FTRACE_ENABLE_CALLS);
 	mutex_unlock(&ftrace_lock);
 
+	trace_biter_free(biter);
 	kfree(iter);
+
 	mutex_unlock(&ftrace_regex_lock);
 	return 0;
 }
@@ -2499,7 +2456,7 @@ static int g_show(struct seq_file *m, void *v)
 		return 0;
 	}
 
-	seq_printf(m, "%pf\n", v);
+	seq_printf(m, "%p\n", (void*) *ptr);
 
 	return 0;
 }
@@ -2602,12 +2559,10 @@ static ssize_t
 ftrace_graph_write(struct file *file, const char __user *ubuf,
 		   size_t cnt, loff_t *ppos)
 {
-	unsigned char buffer[FTRACE_BUFF_MAX+1];
+	struct trace_biter biter;
 	unsigned long *array;
 	size_t read = 0;
 	ssize_t ret;
-	int index = 0;
-	char ch;
 
 	if (!cnt || cnt < 0)
 		return 0;
@@ -2625,51 +2580,27 @@ ftrace_graph_write(struct file *file, const char __user *ubuf,
 	} else
 		array = file->private_data;
 
-	ret = get_user(ch, ubuf++);
-	if (ret)
+	if (trace_biter_alloc(&biter, FTRACE_BUFF_MAX)) {
+		ret = -ENOMEM;
 		goto out;
-	read++;
-	cnt--;
-
-	/* skip white space */
-	while (cnt && isspace(ch)) {
-		ret = get_user(ch, ubuf++);
-		if (ret)
-			goto out;
-		read++;
-		cnt--;
 	}
 
-	if (isspace(ch)) {
-		*ppos += read;
-		ret = read;
-		goto out;
-	}
+	read = trace_get_user(&biter, ubuf, cnt, ppos);
+
+	if (TRACE_BITER_LOADED((&biter))) {
+		biter.buffer[biter.idx] = 0;
+
+		/* we allow only one expression at a time */
+		ret = ftrace_set_func(array, &ftrace_graph_count,
+					biter.buffer);
 
-	while (cnt && !isspace(ch)) {
-		if (index < FTRACE_BUFF_MAX)
-			buffer[index++] = ch;
-		else {
-			ret = -EINVAL;
-			goto out;
-		}
-		ret = get_user(ch, ubuf++);
 		if (ret)
 			goto out;
-		read++;
-		cnt--;
 	}
-	buffer[index] = 0;
-
-	/* we allow only one expression at a time */
-	ret = ftrace_set_func(array, &ftrace_graph_count, buffer);
-	if (ret)
-		goto out;
-
-	file->f_pos += read;
 
 	ret = read;
  out:
+	trace_biter_free(&biter);
 	mutex_unlock(&graph_lock);
 
 	return ret;

Re: [PATCH 0/3] tracing - adding common method for reading/parsing user input

[Steven Rostedt]

On Fri, 2009-09-04 at 15:31 +0200, jolsa@redhat.com wrote:
> As 3 different places for 4 user input files were using the same way
> of reading and parsing user's input, I made one common routine to be
> used. 
> 
> The mentioned files are:
> 	set_graph_function
> 	set_event
> 	set_ftrace_filter
> 	set_ftrace_notrace
> 

Hi Jolsa,

I'll take a look at these. I know there's duplicate code that I even
wrote a "generic" handler for others to use, that basically got a NAK
from Andrew Morton. I probably could have put it into the ftrace code
directly, but never did so.

It will be interesting to look at your patches, especially when those
functions all do things quite differently, with the exception of
set_ftrace_filter and set_ftrace_notrace.

Note, all your patches have [PATCH 0/3] in the subject. I think I can
figure out the order, but you will want to fix that on your next
version.

Currently I'm bug fixing other areas that need a bit of attention. I'll
add this to my queue.

Thanks,

-- Steve

Re: [PATCH 0/3] tracing - adding common method for reading/parsing user input

[Jiri Olsa]

On Fri, Sep 04, 2009 at 09:51:29AM -0400, Steven Rostedt wrote:
> 
> On Fri, 2009-09-04 at 15:31 +0200, jolsa@redhat.com wrote:
> > As 3 different places for 4 user input files were using the same way
> > of reading and parsing user's input, I made one common routine to be
> > used. 
> > 
> > The mentioned files are:
> > 	set_graph_function
> > 	set_event
> > 	set_ftrace_filter
> > 	set_ftrace_notrace
> > 
> 
> Hi Jolsa,
> 
> I'll take a look at these. I know there's duplicate code that I even
> wrote a "generic" handler for others to use, that basically got a NAK
> from Andrew Morton. I probably could have put it into the ftrace code
> directly, but never did so.
> 
> It will be interesting to look at your patches, especially when those
> functions all do things quite differently, with the exception of
> set_ftrace_filter and set_ftrace_notrace.
> 
> Note, all your patches have [PATCH 0/3] in the subject. I think I can
> figure out the order, but you will want to fix that on your next
> version.

oops, script fault ;) I resent v2, plz check

> 
> Currently I'm bug fixing other areas that need a bit of attention. I'll
> add this to my queue.

thanks,
jirka

> 
> Thanks,
> 
> -- Steve
> 
>