Re: [PATCH v3 01/33] lib/printbuf: New data structure for printing strings

From: Petr Mladek
Date: Thu Jun 09 2022 - 09:55:54 EST


On Sat 2022-06-04 15:30:10, 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.
>
> Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> ---
> include/linux/printbuf.h | 118 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 118 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..8b3797dc4b
> --- /dev/null
> +++ b/include/linux/printbuf.h
> @@ -0,0 +1,118 @@
> +/* 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

It might be worth to mention:

@pos is the position that would be used to write into the buffer.
The string is truncated when @pos is greater or equal to
the buffer @size.

@buf might be NULL and @size zero when the buffer is used just
to count the needed buffer size.

It is visible in the code. But it would be nice to explicitly
mention it because it is one important feature of this buffer.

> + */
> +
> +struct printbuf {
> + char *buf;
> + unsigned size;
> + unsigned pos;
> +};
> +
> +static inline void __prt_chars(struct printbuf *out, char c, unsigned n)
> +{
> + memset(out->buf + out->pos,
> + c,
> + min(n, printbuf_remaining(out)));
> + out->pos += n;
> +}

Thanks for using "prt_" prefix. It is better than "pr_", definitely.

Though, I wonder if you considered using:

pb_ aka "Print to Buffer"
ppb_ aka "Print to PrintBuffer"
bpr_ aka "Buffer Print"

to make it more obvious that it is printing into some buffer.

I promise that this is my last try to bikeshed about the prefix.
I just want to get it "right" before you extend the patchset even
more.

Otherwise, the code looks good to me.

Best Regards,
Petr