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.
Link: http://lkml.kernel.org/r/[email protected]
Tested-by: Jiri Kosina <[email protected]>
Acked-by: Jiri Kosina <[email protected]>
Reviewed-by: Petr Mladek <[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 581c1fc733c3..3e550d214187 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -64,6 +64,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)
+{
+ WARN_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 9d3bb64dca31..4f35a783b82f 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 Thu 2014-11-13 20:13:02, 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.
>
> Link: http://lkml.kernel.org/r/[email protected]
>
> Tested-by: Jiri Kosina <[email protected]>
> Acked-by: Jiri Kosina <[email protected]>
> Reviewed-by: Petr Mladek <[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 581c1fc733c3..3e550d214187 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -64,6 +64,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)
> +{
> + WARN_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);
I thought about it more and we should probably do
BUG_ON(s->len + num > s->size);
"size + 1" signalizes that there was an overflow. It is a valid
value for the "len" variable. But it is not valid to commit "size + 1"
bytes into the buffer. It would mean access outside of the buffer.
What do you think, please?
Best Regards,
Petr
PS: Sigh, I wish, I saw all this problems during the first review and not
triggering another patchset version.
> + 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 9d3bb64dca31..4f35a783b82f 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, 14 Nov 2014 18:18:28 +0100
Petr Mladek <[email protected]> wrote:
> On Thu 2014-11-13 20:13:02, 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.
> >
> > Link: http://lkml.kernel.org/r/[email protected]
> >
> > Tested-by: Jiri Kosina <[email protected]>
> > Acked-by: Jiri Kosina <[email protected]>
> > Reviewed-by: Petr Mladek <[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 581c1fc733c3..3e550d214187 100644
> > --- a/include/linux/seq_buf.h
> > +++ b/include/linux/seq_buf.h
> > @@ -64,6 +64,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)
> > +{
> > + WARN_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);
>
> I thought about it more and we should probably do
>
> BUG_ON(s->len + num > s->size);
>
> "size + 1" signalizes that there was an overflow. It is a valid
> value for the "len" variable. But it is not valid to commit "size + 1"
> bytes into the buffer. It would mean access outside of the buffer.
>
> What do you think, please?
I don't see anything wrong with committing an overflowed state.
Note, seq_commit() allows an overflow number to go in too. But of
course that's where overflow is still on the buffer.
Hmm, I guess I can still make this change. The seq_commit() comment
states that @num needs to be < 0 if the data did not fit, and if it is
in overflow state, then it should be -1 not size + 1 (or size for the
old way).
-- Steve
>
> Best Regards,
> Petr
>
> PS: Sigh, I wish, I saw all this problems during the first review and not
> triggering another patchset version.
>
>
> > + 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 9d3bb64dca31..4f35a783b82f 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, 14 Nov 2014 15:31:23 -0500
Steven Rostedt <[email protected]> wrote:
> Hmm, I guess I can still make this change. The seq_commit() comment
> states that @num needs to be < 0 if the data did not fit, and if it is
> in overflow state, then it should be -1 not size + 1 (or size for the
> old way).
I made the change, and here it is:
-- Steve
>From 69488bad76c4af9f9b5948c8990b5ed1196b2bc1 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]
Link: http://lkml.kernel.org/r/[email protected]
Tested-by: Jiri Kosina <[email protected]>
Acked-by: Jiri Kosina <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/seq_buf.h | 41 +++++++++++++++++++++++++++++++++++++++++
kernel/trace/seq_buf.c | 7 +++----
2 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index 0800a24b4348..12c64282aa98 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -70,6 +70,47 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
return min(s->len, s->size);
}
+/**
+ * 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)
+{
+ WARN_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 {
+ /* num must be negative on overflow */
+ BUG_ON(s->len + num > s->size);
+ 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 86e50f47894b..3657af4b5f53 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_buffer_left(s);
+ char *buf;
+ size_t size = seq_buf_get_buf(s, &buf);
int res = -1;
WARN_ON(s->size == 0);
@@ -294,8 +294,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;
}
--
1.8.1.4