This adds printbufs: a printbuf points to a char * buffer and knows the
size of the output buffer as well as the current output position.
Future patches will be adding more features to printbuf, but initially
printbufs are targeted at refactoring and improving our existing code in
lib/vsprintf.c - so this initial printbuf patch has the features
required for that.
Signed-off-by: Kent Overstreet <[email protected]>
---
include/linux/printbuf.h | 94 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 94 insertions(+)
create mode 100644 include/linux/printbuf.h
diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
new file mode 100644
index 0000000000..40dc07040d
--- /dev/null
+++ b/include/linux/printbuf.h
@@ -0,0 +1,94 @@
+/* SPDX-License-Identifier: LGPL-2.1+ */
+/* Copyright (C) 2022 Kent Overstreet */
+
+#ifndef _LINUX_PRINTBUF_H
+#define _LINUX_PRINTBUF_H
+
+#include <linux/string.h>
+
+/*
+ * Printbufs: String buffer for outputting (printing) to, for vsnprintf
+ */
+
+struct printbuf {
+ char *buf;
+ unsigned size;
+ unsigned pos;
+};
+
+static inline unsigned printbuf_remaining(struct printbuf *out)
+{
+ return out->pos < out->size ? out->size - out->pos : 0;
+}
+
+static inline unsigned printbuf_written(struct printbuf *out)
+{
+ return min(out->pos, out->size);
+}
+
+static inline void printbuf_nul_terminate(struct printbuf *out)
+{
+ if (out->pos < out->size)
+ out->buf[out->pos] = 0;
+ else if (out->size)
+ out->buf[out->size - 1] = 0;
+}
+
+static inline void pr_chars(struct printbuf *out, char c, unsigned n)
+{
+ memset(out->buf + out->pos,
+ c,
+ min(n, printbuf_remaining(out)));
+ out->pos += n;
+ printbuf_nul_terminate(out);
+}
+
+static inline void __pr_char(struct printbuf *out, char c)
+{
+ if (printbuf_remaining(out))
+ out->buf[out->pos] = c;
+ out->pos++;
+}
+
+static inline void pr_char(struct printbuf *out, char c)
+{
+ __pr_char(out, c);
+ printbuf_nul_terminate(out);
+}
+
+static inline void pr_bytes(struct printbuf *out, const void *b, unsigned n)
+{
+ memcpy(out->buf + out->pos,
+ b,
+ min(n, printbuf_remaining(out)));
+ out->pos += n;
+ printbuf_nul_terminate(out);
+}
+
+static inline void pr_str(struct printbuf *out, const char *str)
+{
+ pr_bytes(out, str, strlen(str));
+}
+
+static inline void pr_hex_byte(struct printbuf *out, u8 byte)
+{
+ __pr_char(out, hex_asc_hi(byte));
+ __pr_char(out, hex_asc_lo(byte));
+ printbuf_nul_terminate(out);
+}
+
+static inline void pr_hex_byte_upper(struct printbuf *out, u8 byte)
+{
+ __pr_char(out, hex_asc_upper_hi(byte));
+ __pr_char(out, hex_asc_upper_lo(byte));
+ printbuf_nul_terminate(out);
+}
+
+#define PRINTBUF ((struct printbuf) { .si_units = PRINTBUF_UNITS_10 })
+#define PRINTBUF_EXTERN(_buf, _size) \
+((struct printbuf) { \
+ .buf = _buf, \
+ .size = _size, \
+})
+
+#endif /* _LINUX_PRINTBUF_H */
--
2.36.0
On Thu 2022-05-19 13:23:54, Kent Overstreet wrote:
> This adds printbufs: a printbuf points to a char * buffer and knows the
> size of the output buffer as well as the current output position.
>
> Future patches will be adding more features to printbuf, but initially
> printbufs are targeted at refactoring and improving our existing code in
> lib/vsprintf.c - so this initial printbuf patch has the features
> required for that.
> diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
> new file mode 100644
> index 0000000000..40dc07040d
> --- /dev/null
> +++ b/include/linux/printbuf.h
> +static inline void pr_chars(struct printbuf *out, char c, unsigned n)
> +{
> + memset(out->buf + out->pos,
> + c,
> + min(n, printbuf_remaining(out)));
> + out->pos += n;
> + printbuf_nul_terminate(out);
> +}
This function is not later used. Please, do not add API
that will not have users in the same patchset.
There are several other cases. I am not going to comment
all of them.
> +static inline void __pr_char(struct printbuf *out, char c)
> +{
> + if (printbuf_remaining(out))
> + out->buf[out->pos] = c;
> + out->pos++;
> +}
> +
> +static inline void pr_char(struct printbuf *out, char c)
> +{
> + __pr_char(out, c);
> + printbuf_nul_terminate(out);
> +}
The "pr_" prefix is a nightmare for me because the same prefix
is used also for printk() API ;-)
Could we please use "pb_" instead?
Note that "prb_" prefix is already used by the lockless printk
ringbuffer, see kernel/printk/printk_ringbuffer.h.
Best Regards,
Petr
On Thu, 2022-05-26 at 11:21 -0400, Kent Overstreet wrote:
> On Thu, May 26, 2022 at 05:06:15PM +0200, Petr Mladek wrote:
> > The "pr_" prefix is a nightmare for me because the same prefix
> > is used also for printk() API ;-)
> >
> > Could we please use "pb_" instead?
>
> I'm not entirely against that, but I see printbufs as already in this patchset
> tightly coupled to vsprintf.c and thus quite related to printk, as well - and
> there aren't that many different pr_ things. So I think the shared prefix makes
> some sense, I'd like to hear what others think before making that change.
I think the reused prefix is not good.
bufs are not printks.
On Thu 2022-05-26 11:21:27, Kent Overstreet wrote:
> On Thu, May 26, 2022 at 05:06:15PM +0200, Petr Mladek wrote:
> > On Thu 2022-05-19 13:23:54, Kent Overstreet wrote:
> > > This adds printbufs: a printbuf points to a char * buffer and knows the
> > > size of the output buffer as well as the current output position.
> > >
> > > Future patches will be adding more features to printbuf, but initially
> > > printbufs are targeted at refactoring and improving our existing code in
> > > lib/vsprintf.c - so this initial printbuf patch has the features
> > > required for that.
> >
> > > diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
> > > new file mode 100644
> > > index 0000000000..40dc07040d
> > > --- /dev/null
> > > +++ b/include/linux/printbuf.h
> > > +static inline void pr_chars(struct printbuf *out, char c, unsigned n)
> > > +{
> > > + memset(out->buf + out->pos,
> > > + c,
> > > + min(n, printbuf_remaining(out)));
> > > + out->pos += n;
> > > + printbuf_nul_terminate(out);
> > > +}
> >
> > This function is not later used. Please, do not add API
> > that will not have users in the same patchset.
> >
> > There are several other cases. I am not going to comment
> > all of them.
>
> It is used in this patchset, in lib/vsnprintf.c. You sure about the other cases?
Ah, I used outdated cscope. This was bad example.
> > > +static inline void __pr_char(struct printbuf *out, char c)
> > > +{
> > > + if (printbuf_remaining(out))
> > > + out->buf[out->pos] = c;
> > > + out->pos++;
> > > +}
> > > +
> > > +static inline void pr_char(struct printbuf *out, char c)
> > > +{
> > > + __pr_char(out, c);
> > > + printbuf_nul_terminate(out);
> > > +}
> >
> > The "pr_" prefix is a nightmare for me because the same prefix
> > is used also for printk() API ;-)
> >
> > Could we please use "pb_" instead?
>
> I'm not entirely against that, but I see printbufs as already in this patchset
> tightly coupled to vsprintf.c and thus quite related to printk, as well - and
> there aren't that many different pr_ things. So I think the shared prefix makes
> some sense, I'd like to hear what others think before making that change.
I would really like to keep the three APIs separated and easy to
distinguish. They are principally different:
1. pr_*() API:
+ wrapper to printk(). They makes the messages available on
console and for user-space log daemons while printf()
+ the various pr_*() variants are used to define kernel
specific features and behavior, for example:
loglevel, ratelimit, only once. deferred console handling.
+ uses implicit (system) buffer
+ The message format is defined by the 1st parameter. It
is the same way as printf() in user-space.
+ It is inspired by printf() from user-space that prints
the messages to the standard output.
2. *s*printf() APIs:
+ basically duplicate the same user-space API. It supports
some extra %p modifiers. There might be few more
incompatibilities.
+ use simple "char *" buffer provided as the 1st parameter
+ the messages format is defined the same way as in
the user-space counterparts.
3. printbuf API:
+ append messages into the given printbuf by small pieces
+ format defined by the suffix, for example, _char(),
bytes(), units_64(), _tab(), indent()
+ allows to do special operations on the buffer,
for example, _reset(), make_room(), atomic_inc()
+ it will be used as low-level API for vscnprinf()
implementation, pretty printing API, or
stand alone uses.
+ I wonder if there will be variant that will allow
to pass the format in the printf() way, e.g.
int pb_printf(printbuf *buf, const char *fmt, ...);
+ is there any user space counter part?
Now, it is clear that printfbuf API must be distinguished by another
prefix:
+ it must be clear that it stores the output into printbuf.
It is similar to dprintf(), fprintf(), sprintf().
+ It can't be done by the suffix because it is already used
to define format of the appended string or extra operation.
+ It must be clear what is low-level API used to implement
vsprintf() and high-level API that uses vsprintf().
I mean pb_char() vs. pb_printf().
Best Regards,
Petr
PS: I probably won't find time to write more comments on this patchset
today. I'll continue the following week. It seems that it will
be a long journey.
On Thu, May 26, 2022 at 05:06:15PM +0200, Petr Mladek wrote:
> On Thu 2022-05-19 13:23:54, Kent Overstreet wrote:
> > This adds printbufs: a printbuf points to a char * buffer and knows the
> > size of the output buffer as well as the current output position.
> >
> > Future patches will be adding more features to printbuf, but initially
> > printbufs are targeted at refactoring and improving our existing code in
> > lib/vsprintf.c - so this initial printbuf patch has the features
> > required for that.
>
> > diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h
> > new file mode 100644
> > index 0000000000..40dc07040d
> > --- /dev/null
> > +++ b/include/linux/printbuf.h
> > +static inline void pr_chars(struct printbuf *out, char c, unsigned n)
> > +{
> > + memset(out->buf + out->pos,
> > + c,
> > + min(n, printbuf_remaining(out)));
> > + out->pos += n;
> > + printbuf_nul_terminate(out);
> > +}
>
> This function is not later used. Please, do not add API
> that will not have users in the same patchset.
>
> There are several other cases. I am not going to comment
> all of them.
It is used in this patchset, in lib/vsnprintf.c. You sure about the other cases?
> > +static inline void __pr_char(struct printbuf *out, char c)
> > +{
> > + if (printbuf_remaining(out))
> > + out->buf[out->pos] = c;
> > + out->pos++;
> > +}
> > +
> > +static inline void pr_char(struct printbuf *out, char c)
> > +{
> > + __pr_char(out, c);
> > + printbuf_nul_terminate(out);
> > +}
>
> The "pr_" prefix is a nightmare for me because the same prefix
> is used also for printk() API ;-)
>
> Could we please use "pb_" instead?
I'm not entirely against that, but I see printbufs as already in this patchset
tightly coupled to vsprintf.c and thus quite related to printk, as well - and
there aren't that many different pr_ things. So I think the shared prefix makes
some sense, I'd like to hear what others think before making that change.
On Fri, May 27, 2022 at 12:29:20PM +0200, Petr Mladek wrote:
> I would really like to keep the three APIs separated and easy to
> distinguish. They are principally different:
>
> 1. pr_*() API:
>
> + wrapper to printk(). They makes the messages available on
> console and for user-space log daemons while printf()
>
> + the various pr_*() variants are used to define kernel
> specific features and behavior, for example:
> loglevel, ratelimit, only once. deferred console handling.
>
> + uses implicit (system) buffer
>
> + The message format is defined by the 1st parameter. It
> is the same way as printf() in user-space.
>
> + It is inspired by printf() from user-space that prints
> the messages to the standard output.
>
>
> 2. *s*printf() APIs:
>
> + basically duplicate the same user-space API. It supports
> some extra %p modifiers. There might be few more
> incompatibilities.
>
> + use simple "char *" buffer provided as the 1st parameter
>
> + the messages format is defined the same way as in
> the user-space counterparts.
After printbufs are merged, I think we should consider formally deprecating
sprintf/snprintf, certainly for new code. As you saw from the vsprintf.c
cleanup, printbufs are _much_ nicer than passing around char */length - it's
2022, we shouldn't be doing that anymore!
>
>
> 3. printbuf API:
>
> + append messages into the given printbuf by small pieces
>
> + format defined by the suffix, for example, _char(),
> bytes(), units_64(), _tab(), indent()
>
> + allows to do special operations on the buffer,
> for example, _reset(), make_room(), atomic_inc()
atomic_inc() should not exist in the long term - we _really_ need
memalloc_nowait_(save|restore), that's the correct way to do this.
> + it will be used as low-level API for vscnprinf()
> implementation, pretty printing API, or
> stand alone uses.
>
> + I wonder if there will be variant that will allow
> to pass the format in the printf() way, e.g.
> int pb_printf(printbuf *buf, const char *fmt, ...);
That's pr_buf()/vpr_buf(), and I heavily use pr_buf() in my own code.
snprintf() is just a wrapper around pr_buf() now.
> + is there any user space counter part?
I've been using the previous version of this code in userspace that was part of
bcachefs, and my intention is very much for this code to also be used in
userspace as well.
Bringing the base printbuf API to userspace is trivial - i.e. doing it as a
wrapper around snprintf(), which is how printbufs started. However, the %(%p)
format string extension for calling pretty-printers directly - which I badly
want and think is far superior to what glibc has [1], will also require patching
glibc (and gcc, to get the format string that we want).
So that'll be a little ways off.
> Now, it is clear that printfbuf API must be distinguished by another
> prefix:
>
> + it must be clear that it stores the output into printbuf.
> It is similar to dprintf(), fprintf(), sprintf().
>
> + It can't be done by the suffix because it is already used
> to define format of the appended string or extra operation.
>
> + It must be clear what is low-level API used to implement
> vsprintf() and high-level API that uses vsprintf().
> I mean pb_char() vs. pb_printf().
I'm coming around to the pb_* naming idea. pbprintf() doesn't roll off the
tongue in the same way that pr_buf() does, but I guess I can live with that.
1: https://www.gnu.org/software/libc/manual/html_node/Printf-Extension-Example.html
On Fri, May 27, 2022 at 12:29:20PM +0200, Petr Mladek wrote:
> I would really like to keep the three APIs separated and easy to
> distinguish. They are principally different:
>
> 1. pr_*() API:
>
> + wrapper to printk(). They makes the messages available on
> console and for user-space log daemons while printf()
>
> + the various pr_*() variants are used to define kernel
> specific features and behavior, for example:
> loglevel, ratelimit, only once. deferred console handling.
>
> + uses implicit (system) buffer
>
> + The message format is defined by the 1st parameter. It
> is the same way as printf() in user-space.
>
> + It is inspired by printf() from user-space that prints
> the messages to the standard output.
>
>
> 2. *s*printf() APIs:
>
> + basically duplicate the same user-space API. It supports
> some extra %p modifiers. There might be few more
> incompatibilities.
>
> + use simple "char *" buffer provided as the 1st parameter
>
> + the messages format is defined the same way as in
> the user-space counterparts.
I'd like to get sprintf() style functions - anything that outputs to raw char *
pointers - deprecated. That's going to mean a _lot_ of refactoring (so I don't
know that I'll be the one to do it), but it's mostly easy refactoring.
> 3. printbuf API:
>
> + append messages into the given printbuf by small pieces
>
> + format defined by the suffix, for example, _char(),
> bytes(), units_64(), _tab(), indent()
>
> + allows to do special operations on the buffer,
> for example, _reset(), make_room(), atomic_inc()
>
> + it will be used as low-level API for vscnprinf()
> implementation, pretty printing API, or
> stand alone uses.
>
> + I wonder if there will be variant that will allow
> to pass the format in the printf() way, e.g.
> int pb_printf(printbuf *buf, const char *fmt, ...);
Right now this is called pr_buf(). I suppose pr_printf()/pb_printf() makes sense
:)
>
> + is there any user space counter part?
>
>
> Now, it is clear that printfbuf API must be distinguished by another
> prefix:
>
> + it must be clear that it stores the output into printbuf.
> It is similar to dprintf(), fprintf(), sprintf().
>
> + It can't be done by the suffix because it is already used
> to define format of the appended string or extra operation.
>
> + It must be clear what is low-level API used to implement
> vsprintf() and high-level API that uses vsprintf().
> I mean pb_char() vs. pb_printf().
So there's more in the pr_* namespace than I realized - I guess you've convinced
me on not reusing that. Which is a shame, because it rolls off the tongue so
much easier than pb_* and I think otherwise makes more sense here - pr_foo for
"print foo".
However, I'm not going to put special operations on printbufs under the pb_
prefix: I want that naming (whether pb_* or pr_*) to _just_ be for "print foo";
this "print this" prefix should be the common prefix for _any_ pretty printer,
unless it has another subsystem prefix - that means there's going to be a lot of
functions with these prefix. So I'm going to keep "printbuf special operations"
on the printbuf_ prefix.
Also, how about prt_* instead of pb_*? I want something that sounds more like
print, and prt_ isn't taken.