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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YqH72gJpajLCLCUc@alley>
Date:   Thu, 9 Jun 2022 15:55:38 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Kent Overstreet <kent.overstreet@...il.com>
Cc:     linux-kernel@...r.kernel.org, rostedt@...dmis.org,
        Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH v3 01/33] lib/printbuf: New data structure for printing
 strings

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@...il.com>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@...radead.org>
> ---
>  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