seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_cstr()
Commit Message
Solve two ergonomic issues with struct seq_buf:
1) Too much boilerplate is required to initialize:
struct seq_buf s;
char buf[32];
seq_buf_init(s, buf, sizeof(buf));
Instead, we can build this directly on the stack. Provide
DECLARE_SEQ_BUF() macro to do this:
DECLARE_SEQ_BUF(s, 32);
2) %NUL termination is fragile and requires 2 steps to get a valid
C String (and is a layering violation exposing the "internals" of
seq_buf):
seq_buf_terminate(s);
do_something(s->buffer);
Instead, we can just return s->buffer direction after terminating it
in refactored seq_buf_terminate(), now known as seq_buf_cstr():
do_soemthing(seq_buf_cstr(s));
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Justin Stitt <justinstitt@google.com>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Yun Zhou <yun.zhou@windriver.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Cc: Zhen Lei <thunder.leizhen@huawei.com>
Cc: linux-trace-kernel@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/seq_buf.h | 19 +++++++++++++++----
kernel/trace/trace.c | 11 +----------
lib/seq_buf.c | 4 +---
3 files changed, 17 insertions(+), 17 deletions(-)
Comments
On Thu, 26 Oct 2023 10:07:28 -0700
Kees Cook <keescook@chromium.org> wrote:
> Solve two ergonomic issues with struct seq_buf:
"ergonomic"? Does it cause carpal tunnel? ;-)
>
> 1) Too much boilerplate is required to initialize:
>
> struct seq_buf s;
> char buf[32];
>
> seq_buf_init(s, buf, sizeof(buf));
>
> Instead, we can build this directly on the stack. Provide
> DECLARE_SEQ_BUF() macro to do this:
>
> DECLARE_SEQ_BUF(s, 32);
>
> 2) %NUL termination is fragile and requires 2 steps to get a valid
> C String (and is a layering violation exposing the "internals" of
> seq_buf):
>
> seq_buf_terminate(s);
> do_something(s->buffer);
>
> Instead, we can just return s->buffer direction after terminating it
> in refactored seq_buf_terminate(), now known as seq_buf_cstr():
>
> do_soemthing(seq_buf_cstr(s));
Do we really need to call it _cstr? Why not just have seq_buf_str() ?
I mean, this is C, do we need to state that in the name too?
BTW, I'm perfectly fine with this change, just the naming I have issues
with.
-- Steve
On Thu, Oct 26, 2023 at 01:38:50PM -0400, Steven Rostedt wrote:
> On Thu, 26 Oct 2023 10:07:28 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
> > Solve two ergonomic issues with struct seq_buf:
>
> "ergonomic"? Does it cause carpal tunnel? ;-)
>
> >
> > 1) Too much boilerplate is required to initialize:
> >
> > struct seq_buf s;
> > char buf[32];
> >
> > seq_buf_init(s, buf, sizeof(buf));
> >
> > Instead, we can build this directly on the stack. Provide
> > DECLARE_SEQ_BUF() macro to do this:
> >
> > DECLARE_SEQ_BUF(s, 32);
> >
> > 2) %NUL termination is fragile and requires 2 steps to get a valid
> > C String (and is a layering violation exposing the "internals" of
> > seq_buf):
> >
> > seq_buf_terminate(s);
> > do_something(s->buffer);
> >
> > Instead, we can just return s->buffer direction after terminating it
> > in refactored seq_buf_terminate(), now known as seq_buf_cstr():
> >
> > do_soemthing(seq_buf_cstr(s));
>
> Do we really need to call it _cstr? Why not just have seq_buf_str() ?
>
> I mean, this is C, do we need to state that in the name too?
I'm fine either way. I did that just to make the distinction between our
length-managed string of characters interface (seq_buf), and the
%NUL-terminated string of characters (traditionally called "C String" in
other languages). And it was still shorter than "seq_buf_terminate(s);
s->buffer" ;)
> BTW, I'm perfectly fine with this change, just the naming I have issues
> with.
Cool; thanks for looking at it!
On Thu, 26 Oct 2023 10:54:26 -0700
Kees Cook <keescook@chromium.org> wrote:
> > Do we really need to call it _cstr? Why not just have seq_buf_str() ?
> >
> > I mean, this is C, do we need to state that in the name too?
>
> I'm fine either way. I did that just to make the distinction between our
> length-managed string of characters interface (seq_buf), and the
> %NUL-terminated string of characters (traditionally called "C String" in
> other languages). And it was still shorter than "seq_buf_terminate(s);
> s->buffer" ;)
Do you believe that people might get confused with it as seq_buf_str()?
Can you envision that we would want a seq_buf_str() and seq_buf_cstr() that
do something different?
-- Steve
On Thu, Oct 26, 2023 at 02:02:47PM -0400, Steven Rostedt wrote:
> On Thu, 26 Oct 2023 10:54:26 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
> > > Do we really need to call it _cstr? Why not just have seq_buf_str() ?
> > >
> > > I mean, this is C, do we need to state that in the name too?
> >
> > I'm fine either way. I did that just to make the distinction between our
> > length-managed string of characters interface (seq_buf), and the
> > %NUL-terminated string of characters (traditionally called "C String" in
> > other languages). And it was still shorter than "seq_buf_terminate(s);
> > s->buffer" ;)
>
> Do you believe that people might get confused with it as seq_buf_str()?
>
> Can you envision that we would want a seq_buf_str() and seq_buf_cstr() that
> do something different?
No, I see your point. Like I said, I don't care either way. I was just
explaining why I did it that way. "string" means a lot of things to
different people. "C String" is unambiguous, and I try to be unambiguous
whenever possible. :)
I'll send a v2 as seq_buf_str()...
On Thu, 26 Oct 2023 12:35:43 -0700
Kees Cook <keescook@chromium.org> wrote:
> I'll send a v2 as seq_buf_str()...
Thanks.
-- Steve
On Thu, Oct 26, 2023 at 10:54:26AM -0700, Kees Cook wrote:
> > > do_soemthing(seq_buf_cstr(s));
> >
> > Do we really need to call it _cstr? Why not just have seq_buf_str() ?
> >
> > I mean, this is C, do we need to state that in the name too?
>
> I'm fine either way. I did that just to make the distinction between our
> length-managed string of characters interface (seq_buf), and the
> %NUL-terminated string of characters (traditionally called "C String" in
> other languages). And it was still shorter than "seq_buf_terminate(s);
> s->buffer" ;)
'cstr' might be short for 'counted string' ...
@@ -21,9 +21,16 @@ struct seq_buf {
size_t len;
};
+#define DECLARE_SEQ_BUF(NAME, SIZE) \
+ char __ ## NAME ## _buffer[SIZE] = ""; \
+ struct seq_buf NAME = { .buffer = &__ ## NAME ## _buffer, \
+ .size = SIZE }
+
static inline void seq_buf_clear(struct seq_buf *s)
{
s->len = 0;
+ if (s->size)
+ s->buffer[0] = '\0';
}
static inline void
@@ -69,8 +76,8 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
}
/**
- * seq_buf_terminate - Make sure buffer is nul terminated
- * @s: the seq_buf descriptor to terminate.
+ * seq_buf_cstr - get %NUL-terminated C string from seq_buf
+ * @s: the seq_buf handle
*
* This makes sure that the buffer in @s is nul terminated and
* safe to read as a string.
@@ -81,16 +88,20 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
*
* After this function is called, s->buffer is safe to use
* in string operations.
+ *
+ * Returns @s->buf after making sure it is terminated.
*/
-static inline void seq_buf_terminate(struct seq_buf *s)
+static inline char *seq_buf_cstr(struct seq_buf *s)
{
if (WARN_ON(s->size == 0))
- return;
+ return "";
if (seq_buf_buffer_left(s))
s->buffer[s->len] = 0;
else
s->buffer[s->size - 1] = 0;
+
+ return s->buffer;
}
/**
@@ -3828,15 +3828,6 @@ static bool trace_safe_str(struct trace_iterator *iter, const char *str,
return false;
}
-static const char *show_buffer(struct trace_seq *s)
-{
- struct seq_buf *seq = &s->seq;
-
- seq_buf_terminate(seq);
-
- return seq->buffer;
-}
-
static DEFINE_STATIC_KEY_FALSE(trace_no_verify);
static int test_can_verify_check(const char *fmt, ...)
@@ -3976,7 +3967,7 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt,
*/
if (WARN_ONCE(!trace_safe_str(iter, str, star, len),
"fmt: '%s' current_buffer: '%s'",
- fmt, show_buffer(&iter->seq))) {
+ fmt, seq_buf_cstr(&iter->seq.seq))) {
int ret;
/* Try to safely read the string */
@@ -109,9 +109,7 @@ void seq_buf_do_printk(struct seq_buf *s, const char *lvl)
if (s->size == 0 || s->len == 0)
return;
- seq_buf_terminate(s);
-
- start = s->buffer;
+ start = seq_buf_cstr(s);
while ((lf = strchr(start, '\n'))) {
int len = lf - start + 1;