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 <[email protected]>
---
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(-)
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 <[email protected]>
---
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
Updated 'set_event' write method to use the 'trace_get_user' function.
wbr,
jirka
Signed-off-by: Jiri Olsa <[email protected]>
---
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
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 <[email protected]>
---
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;
On Sat, 2009-09-05 at 03:12 +0200, [email protected] 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.
>
Patches 1 and 3 have a couple of minor checkpatch errors..
Daniel
On Fri, Sep 04, 2009 at 07:41:26PM -0700, Daniel Walker wrote:
> On Sat, 2009-09-05 at 03:12 +0200, [email protected] 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.
> >
>
> Patches 1 and 3 have a couple of minor checkpatch errors..
>
> Daniel
>
thanks, let me send out those changes later after I get other comments
wbr,
jirka
On Sat, 2009-09-05 at 08:40 +0200, Jiri Olsa wrote:
> On Fri, Sep 04, 2009 at 07:41:26PM -0700, Daniel Walker wrote:
> > On Sat, 2009-09-05 at 03:12 +0200, [email protected] 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.
> > >
> >
> > Patches 1 and 3 have a couple of minor checkpatch errors..
> >
> > Daniel
> >
>
> thanks, let me send out those changes later after I get other comments
I wont be able to look at these until next week.
-- Steve
On Sat, 2009-09-05 at 03:12 +0200, [email protected] wrote:
> 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
incput?
> (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 <[email protected]>
>
> ---
> 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
incput again?
> + * (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
length
> + * @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;
> +}
I don't see any reason that trace_biter_alloc is as static inline in a
header file. It should be a normal function in trace.c
> +
> +/*
> + * trace_biter_free - frees the buffer for trace buffer iterator
> + */
> +static inline void trace_biter_free(struct trace_biter *biter)
> +{
> + kfree(biter->buffer);
> +}
trace_biter_free too.
But I also hate the name "biter", looks too much like "bitter". How
about something like "trace_parser", or "trace_string_reader"?
-- Steve
> +
> +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.
> *
On Sat, 2009-09-05 at 03:12 +0200, [email protected] 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
>
> wbr,
> jirka
>
> Signed-off-by: Jiri Olsa <[email protected]>
Looking back, this is pretty much exactly what I did back in February:
http://lkml.org/lkml/2009/2/25/385
The difference is that I wanted this to be a generic feature for things
outside of tracing to use. I may just take my old patch and put the code
in trace.c like you did.
Thanks,
-- Steve
On Tue, Sep 08, 2009 at 10:01:39AM -0400, Steven Rostedt wrote:
> On Sat, 2009-09-05 at 03:12 +0200, [email protected] 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
> >
> > wbr,
> > jirka
> >
> > Signed-off-by: Jiri Olsa <[email protected]>
>
> Looking back, this is pretty much exactly what I did back in February:
>
> http://lkml.org/lkml/2009/2/25/385
>
> The difference is that I wanted this to be a generic feature for things
> outside of tracing to use. I may just take my old patch and put the code
> in trace.c like you did.
>
> Thanks,
>
> -- Steve
>
>
ok, so are you going to take your patch, or should I do the changes you
proposed and send it out again?
wbr,
jirka
On Tue, 2009-09-08 at 16:37 +0200, Jiri Olsa wrote:
> On Tue, Sep 08, 2009 at 10:01:39AM -0400, Steven Rostedt wrote:
>
> ok, so are you going to take your patch, or should I do the changes you
> proposed and send it out again?
Yeah, I may forward port my patch. But I may take some things from your
patch and apply them too. I'll put your name in the change log too ;-)
Thanks,
-- Steve
On Tue, Sep 08, 2009 at 10:44:30AM -0400, Steven Rostedt wrote:
> On Tue, 2009-09-08 at 16:37 +0200, Jiri Olsa wrote:
> > On Tue, Sep 08, 2009 at 10:01:39AM -0400, Steven Rostedt wrote:
>
> >
> > ok, so are you going to take your patch, or should I do the changes you
> > proposed and send it out again?
>
> Yeah, I may forward port my patch. But I may take some things from your
> patch and apply them too. I'll put your name in the change log too ;-)
>
> Thanks,
>
> -- Steve
>
>
ok, cool :)
thanks,
jirka
Ah, after working with my old patches, I think I like yours better ;-)
But I still have some issues. I'll comment on them, and if you can send
out a v3 with the updates, I'll apply them.
On Sat, 2009-09-05 at 03:12 +0200, [email protected] wrote:
> 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 <[email protected]>
>
> ---
> 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
typo - incput
> + * (matched by isspace(ch))
> + *
> + * For each string found the 'struct trace_biter' iterator is updated,
Call it trace_parser, I hate the sound of trace_biter (I keep
pronouncing it as bitter).
> + * 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,
s/biter/parser/g
> + size_t cnt, loff_t *ppos)
> +{
> + char ch;
> + size_t read = 0;
> + ssize_t ret;
> +
> + if (!*ppos)
> + TRACE_BITER_CLEAR(biter);
Don't do this with macros (explained below)
> +
> + ret = get_user(ch, ubuf++);
> + if (ret)
> + goto out;
> +
> + read++;
> + cnt--;
> +
> + /*
> + * If the parser haven't finished with the last write,
grammar nit - "If the parser is not finished ..."
> + * 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
Again, rename to trace_parser.
> + * @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)
Ug, just use parser->idx directly. No need for macros. Or if you want to
encapsulate this, then use static inline functions. Avoid MACROS, they
just make the code ugly.
> +
> +/*
> + * trace_biter_alloc - allocates the buffer for trace buffer iterator
> + */
> +static inline int trace_biter_alloc(struct trace_biter *biter, int size)
Using alloc as the name here is confusing because you are passing in a
struct trace_parser already, so you are not allocating one. Rename it to
trace_parser_get_init().
> +{
> + 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)
And again, you are not freeing the trace_parser. Lets rename this to
trace_parser_put(). This way we have a get/put pair that developers can
relate to. Maybe another name pair may be more appropriate, but when you
call something x_alloc/x_free, it refers to allocating and freeing x,
not a part of x.
-- Steve
> +{
> + 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.
> *
On Sat, 2009-09-05 at 03:13 +0200, [email protected] wrote:
> Updated 'set_event' write method to use the 'trace_get_user' function.
>
> wbr,
> jirka
>
> Signed-off-by: Jiri Olsa <[email protected]>
>
> ---
> 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;
> }
On Sat, 2009-09-05 at 03:13 +0200, [email protected] wrote:
> 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.
>
This patch seems fine as long as it gets updated with the same comments
of the other patches.
-- Steve
> wbr,
> jirka
>
> Signed-off-by: Jiri Olsa <[email protected]>
>
> ---
> 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;