From: "Steven Rostedt (Red Hat)" <[email protected]>
Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that
are used by seq_buf_path(). This makes the code similar to the
seq_file: seq_path() function, and will help to be able to consolidate
the functions between seq_file and trace_seq.
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/seq_buf.h | 40 ++++++++++++++++++++++++++++++++++++++++
kernel/trace/seq_buf.c | 7 +++----
2 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index 3cd25038cb5e..10c17c61c7a4 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -55,6 +55,46 @@ seq_buf_set_overflow(struct seq_buf *s)
s->len = s->size + 1;
}
+/**
+ * seq_buf_get_buf - get buffer to write arbitrary data to
+ * @s: the seq_buf handle
+ * @bufp: the beginning of the buffer is stored here
+ *
+ * Return the number of bytes available in the buffer, or zero if
+ * there's no space.
+ */
+static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
+{
+ BUG_ON(s->len > s->size + 1);
+
+ if (s->len < s->size) {
+ *bufp = s->buffer + s->len;
+ return s->size - s->len;
+ }
+
+ *bufp = NULL;
+ return 0;
+}
+
+/**
+ * seq_buf_commit - commit data to the buffer
+ * @s: the seq_buf handle
+ * @num: the number of bytes to commit
+ *
+ * Commit @num bytes of data written to a buffer previously acquired
+ * by seq_buf_get. To signal an error condition, or that the data
+ * didn't fit in the available space, pass a negative @num value.
+ */
+static inline void seq_buf_commit(struct seq_buf *s, int num)
+{
+ if (num < 0) {
+ s->len = s->size;
+ } else {
+ BUG_ON(s->len + num > s->size + 1);
+ s->len += num;
+ }
+}
+
extern __printf(2, 3)
int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
extern __printf(2, 0)
diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
index 06fd1833e692..d118d0d6c956 100644
--- a/kernel/trace/seq_buf.c
+++ b/kernel/trace/seq_buf.c
@@ -280,8 +280,8 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
*/
int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
{
- char *buf = s->buffer + s->len;
- size_t size = SEQ_BUF_LEFT(s);
+ char *buf;
+ size_t size = seq_buf_get_buf(s, &buf);
int res = -1;
if (size) {
@@ -292,8 +292,7 @@ int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
res = end - buf;
}
}
- if (res > 0)
- s->len += res;
+ seq_buf_commit(s, res);
return res;
}
--
2.1.1
On Tue 2014-11-04 10:52:45, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <[email protected]>
>
> Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that
> are used by seq_buf_path(). This makes the code similar to the
> seq_file: seq_path() function, and will help to be able to consolidate
> the functions between seq_file and trace_seq.
>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> include/linux/seq_buf.h | 40 ++++++++++++++++++++++++++++++++++++++++
> kernel/trace/seq_buf.c | 7 +++----
> 2 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index 3cd25038cb5e..10c17c61c7a4 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -55,6 +55,46 @@ seq_buf_set_overflow(struct seq_buf *s)
> s->len = s->size + 1;
> }
>
> +/**
> + * seq_buf_get_buf - get buffer to write arbitrary data to
> + * @s: the seq_buf handle
> + * @bufp: the beginning of the buffer is stored here
> + *
> + * Return the number of bytes available in the buffer, or zero if
> + * there's no space.
> + */
> +static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
> +{
> + BUG_ON(s->len > s->size + 1);
> + if (s->len < s->size) {
> + *bufp = s->buffer + s->len;
> + return s->size - s->len;
> + }
> + *bufp = NULL;
> + return 0;
> +}
> +
> +/**
> + * seq_buf_commit - commit data to the buffer
> + * @s: the seq_buf handle
> + * @num: the number of bytes to commit
> + *
> + * Commit @num bytes of data written to a buffer previously acquired
> + * by seq_buf_get. To signal an error condition, or that the data
> + * didn't fit in the available space, pass a negative @num value.
> + */
> +static inline void seq_buf_commit(struct seq_buf *s, int num)
> +{
> + if (num < 0) {
> + s->len = s->size;
I guess that you want to get the buffer into an overflow state. If
yes, it should be:
s->len = s->size + 1;
or even better
seq_buf_set_overflow(s);
Note that this whole patch depends on the decision about the 7th patch
("tracing: Have seq_buf use full buffer").
It might make sense to move the SEQ_BUF_LEFT, SEQ_BUF_USED macros to
this header and use them in these two function. Or move the functions
to seq_file.c.
Best Regards,
Petr
On Wed, 5 Nov 2014 17:51:25 +0100
Petr Mladek <[email protected]> wrote:
> > +
> > +/**
> > + * seq_buf_commit - commit data to the buffer
> > + * @s: the seq_buf handle
> > + * @num: the number of bytes to commit
> > + *
> > + * Commit @num bytes of data written to a buffer previously acquired
> > + * by seq_buf_get. To signal an error condition, or that the data
> > + * didn't fit in the available space, pass a negative @num value.
> > + */
> > +static inline void seq_buf_commit(struct seq_buf *s, int num)
> > +{
> > + if (num < 0) {
> > + s->len = s->size;
>
> I guess that you want to get the buffer into an overflow state. If
> yes, it should be:
>
> s->len = s->size + 1;
>
> or even better
>
> seq_buf_set_overflow(s);
Yeah, I agree with using the seq_buf_set_overflow().
>
>
> Note that this whole patch depends on the decision about the 7th patch
> ("tracing: Have seq_buf use full buffer").
Which I have decided to do.
>
> It might make sense to move the SEQ_BUF_LEFT, SEQ_BUF_USED macros to
> this header and use them in these two function. Or move the functions
> to seq_file.c.
>
I moved SEQ_BUF_LEFT as a static inline seq_buf_buffer_left() and have
used it in trace_seq.c in the TRACE_SEQ_BUF_LEFT macro.
-- Steve
More updates. Hmm, maybe I should have posted the full series ;-)
-- Steve
>From 41a3f3f5e772ca26ef4441a0312d3f108693d7dc Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 29 Oct 2014 17:30:50 -0400
Subject: [PATCH] tracing: Add seq_buf_get_buf() and seq_buf_commit() helper
functions
Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that
are used by seq_buf_path(). This makes the code similar to the
seq_file: seq_path() function, and will help to be able to consolidate
the functions between seq_file and trace_seq.
Link: http://lkml.kernel.org/r/[email protected]
Tested-by: Jiri Kosina <[email protected]>
Acked-by: Jiri Kosina <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/seq_buf.h | 40 ++++++++++++++++++++++++++++++++++++++++
kernel/trace/seq_buf.c | 7 +++----
2 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index 4aab47d10760..7dacdc791225 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -61,6 +61,46 @@ seq_buf_buffer_left(struct seq_buf *s)
return s->size - s->len;
}
+/**
+ * seq_buf_get_buf - get buffer to write arbitrary data to
+ * @s: the seq_buf handle
+ * @bufp: the beginning of the buffer is stored here
+ *
+ * Return the number of bytes available in the buffer, or zero if
+ * there's no space.
+ */
+static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
+{
+ BUG_ON(s->len > s->size + 1);
+
+ if (s->len < s->size) {
+ *bufp = s->buffer + s->len;
+ return s->size - s->len;
+ }
+
+ *bufp = NULL;
+ return 0;
+}
+
+/**
+ * seq_buf_commit - commit data to the buffer
+ * @s: the seq_buf handle
+ * @num: the number of bytes to commit
+ *
+ * Commit @num bytes of data written to a buffer previously acquired
+ * by seq_buf_get. To signal an error condition, or that the data
+ * didn't fit in the available space, pass a negative @num value.
+ */
+static inline void seq_buf_commit(struct seq_buf *s, int num)
+{
+ if (num < 0) {
+ seq_buf_set_overflow(s);
+ } else {
+ BUG_ON(s->len + num > s->size + 1);
+ s->len += num;
+ }
+}
+
extern __printf(2, 3)
int seq_buf_printf(struct seq_buf *s, const char *fmt, ...);
extern __printf(2, 0)
diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
index 8e60b5c1b9f3..25090f81ea8a 100644
--- a/kernel/trace/seq_buf.c
+++ b/kernel/trace/seq_buf.c
@@ -283,8 +283,8 @@ int seq_buf_putmem_hex(struct seq_buf *s, const void *mem,
*/
int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
{
- char *buf = s->buffer + s->len;
- size_t size = seq_buf_buffer_left(s);
+ char *buf;
+ size_t size = seq_buf_get_buf(s, &buf);
int res = -1;
WARN_ON(s->size == 0);
@@ -297,8 +297,7 @@ int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
res = end - buf;
}
}
- if (res > 0)
- s->len += res;
+ seq_buf_commit(s, res);
return res;
}
--
2.1.1
On Fri 2014-11-07 13:39:29, Steven Rostedt wrote:
> More updates. Hmm, maybe I should have posted the full series ;-)
>
> -- Steve
>
> From 41a3f3f5e772ca26ef4441a0312d3f108693d7dc Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Red Hat)" <[email protected]>
> Date: Wed, 29 Oct 2014 17:30:50 -0400
> Subject: [PATCH] tracing: Add seq_buf_get_buf() and seq_buf_commit() helper
> functions
>
> Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that
> are used by seq_buf_path(). This makes the code similar to the
> seq_file: seq_path() function, and will help to be able to consolidate
> the functions between seq_file and trace_seq.
>
> Link: http://lkml.kernel.org/r/[email protected]
>
> Tested-by: Jiri Kosina <[email protected]>
> Acked-by: Jiri Kosina <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Well, I am curious about the BUG_ONs, see below.
> ---
> include/linux/seq_buf.h | 40 ++++++++++++++++++++++++++++++++++++++++
> kernel/trace/seq_buf.c | 7 +++----
> 2 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index 4aab47d10760..7dacdc791225 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -61,6 +61,46 @@ seq_buf_buffer_left(struct seq_buf *s)
> return s->size - s->len;
> }
>
> +/**
> + * seq_buf_get_buf - get buffer to write arbitrary data to
> + * @s: the seq_buf handle
> + * @bufp: the beginning of the buffer is stored here
> + *
> + * Return the number of bytes available in the buffer, or zero if
> + * there's no space.
> + */
> +static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
> +{
> + BUG_ON(s->len > s->size + 1);
I just wonder if the BUG_ON() is appropriate here. There is used
WARN_ON() for the other similar checks.
On one hand. This function will be used by a code that manipulates
the buffer its own way. Therefore the BUG() would help to debug
potential problems.
On the other hand, this function is used just to get the buffer.
Therefore the BUG() might come too late. The buffer was broken
somewhere else.
> +
> + if (s->len < s->size) {
> + *bufp = s->buffer + s->len;
> + return s->size - s->len;
> + }
> +
> + *bufp = NULL;
> + return 0;
> +}
> +
> +/**
> + * seq_buf_commit - commit data to the buffer
> + * @s: the seq_buf handle
> + * @num: the number of bytes to commit
> + *
> + * Commit @num bytes of data written to a buffer previously acquired
> + * by seq_buf_get. To signal an error condition, or that the data
> + * didn't fit in the available space, pass a negative @num value.
> + */
> +static inline void seq_buf_commit(struct seq_buf *s, int num)
> +{
> + if (num < 0) {
> + seq_buf_set_overflow(s);
> + } else {
> + BUG_ON(s->len + num > s->size + 1);
I agree that the BUG_ON makes sense here. If someone passed too big
"num", she probably also wrote too many bytes and the memory is
corrupted at this point.
> + s->len += num;
> + }
> +}
> +
Best Regards,
Petr
On Mon, 10 Nov 2014 19:33:29 +0100
Petr Mladek <[email protected]> wrote:
> On Fri 2014-11-07 13:39:29, Steven Rostedt wrote:
> > More updates. Hmm, maybe I should have posted the full series ;-)
> >
> > -- Steve
> >
> > From 41a3f3f5e772ca26ef4441a0312d3f108693d7dc Mon Sep 17 00:00:00 2001
> > From: "Steven Rostedt (Red Hat)" <[email protected]>
> > Date: Wed, 29 Oct 2014 17:30:50 -0400
> > Subject: [PATCH] tracing: Add seq_buf_get_buf() and seq_buf_commit() helper
> > functions
> >
> > Add two helper functions; seq_buf_get_buf() and seq_buf_commit() that
> > are used by seq_buf_path(). This makes the code similar to the
> > seq_file: seq_path() function, and will help to be able to consolidate
> > the functions between seq_file and trace_seq.
> >
> > Link: http://lkml.kernel.org/r/[email protected]
> >
> > Tested-by: Jiri Kosina <[email protected]>
> > Acked-by: Jiri Kosina <[email protected]>
> > Signed-off-by: Steven Rostedt <[email protected]>
>
> Reviewed-by: Petr Mladek <[email protected]>
>
> Well, I am curious about the BUG_ONs, see below.
>
> > ---
> > include/linux/seq_buf.h | 40 ++++++++++++++++++++++++++++++++++++++++
> > kernel/trace/seq_buf.c | 7 +++----
> > 2 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> > index 4aab47d10760..7dacdc791225 100644
> > --- a/include/linux/seq_buf.h
> > +++ b/include/linux/seq_buf.h
> > @@ -61,6 +61,46 @@ seq_buf_buffer_left(struct seq_buf *s)
> > return s->size - s->len;
> > }
> >
> > +/**
> > + * seq_buf_get_buf - get buffer to write arbitrary data to
> > + * @s: the seq_buf handle
> > + * @bufp: the beginning of the buffer is stored here
> > + *
> > + * Return the number of bytes available in the buffer, or zero if
> > + * there's no space.
> > + */
> > +static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
> > +{
> > + BUG_ON(s->len > s->size + 1);
>
> I just wonder if the BUG_ON() is appropriate here. There is used
> WARN_ON() for the other similar checks.
That should be a WARN_ON(). Thanks.
It's probably a BUG_ON as that was code that was used internally for
earlier iterations, and the BUG_ON() was for me to see it quickly.
Could have also been that previous versions could access memory that it
should not.
But as it's intended to be used by others, it should be a warning
instead of a bug.
>
> On one hand. This function will be used by a code that manipulates
> the buffer its own way. Therefore the BUG() would help to debug
> potential problems.
>
> On the other hand, this function is used just to get the buffer.
> Therefore the BUG() might come too late. The buffer was broken
> somewhere else.
>
> > +
> > + if (s->len < s->size) {
> > + *bufp = s->buffer + s->len;
> > + return s->size - s->len;
> > + }
> > +
> > + *bufp = NULL;
> > + return 0;
> > +}
> > +
> > +/**
> > + * seq_buf_commit - commit data to the buffer
> > + * @s: the seq_buf handle
> > + * @num: the number of bytes to commit
> > + *
> > + * Commit @num bytes of data written to a buffer previously acquired
> > + * by seq_buf_get. To signal an error condition, or that the data
> > + * didn't fit in the available space, pass a negative @num value.
> > + */
> > +static inline void seq_buf_commit(struct seq_buf *s, int num)
> > +{
> > + if (num < 0) {
> > + seq_buf_set_overflow(s);
> > + } else {
> > + BUG_ON(s->len + num > s->size + 1);
>
> I agree that the BUG_ON makes sense here. If someone passed too big
> "num", she probably also wrote too many bytes and the memory is
> corrupted at this point.
Yeah, this one is worse than the other one and should bug to prevent
memory corruption.
Thanks for reviewing this,
-- Steve
>
> > + s->len += num;
> > + }
> > +}
> > +
>
> Best Regards,
> Petr