On 20/06/2022 02.42, Kent Overstreet wrote:
> This patch adds two new features to printbuf for structured formatting:
>
> - Indent level: the indent level, as a number of spaces, may be
> increased with pr_indent_add() and decreased with pr_indent_sub().
>
> Subsequent lines, when started with pr_newline() (not "\n", although
> that may change) will then be intended according to the current
> indent level. This helps with pretty-printers that structure a large
> amonut of data across multiple lines and multiple functions.
>
> - Tabstops: Tabstops may be set by assigning to the printbuf->tabstops
> array.
>
> Then, pr_tab() may be used to advance to the next tabstop, printing
> as many spaces as required - leaving previous output left justified
> to the previous tabstop. pr_tab_rjust() advances to the next tabstop
> but inserts the spaces just after the previous tabstop - right
> justifying the previously-outputted text to the next tabstop.
I am really, really, really not convinced that we want or need this. But
as long as this doesn't add overhead to those not using it (in
particular, as long as it doesn't grow a "scan whatever contents was
just added for maybe a \n so ->last_newline can be updated"), meh.
> + *
> + * Make sure you use prt_newline() instead of \n in the format string for indent
> + * level and tabstops to work corretly.
> */
>
> #include <linux/kernel.h>
> @@ -45,18 +62,29 @@ struct printbuf {
> char *buf;
> unsigned size;
> unsigned pos;
> + unsigned last_newline;
> + unsigned last_field;
> + unsigned indent;
> /*
> * If nonzero, allocations will be done with GFP_ATOMIC:
> */
> u8 atomic;
> bool allocation_failure:1;
> bool heap_allocated:1;
> + u8 tabstop;
> + u8 tabstops[4];
> };
>
> int printbuf_make_room(struct printbuf *, unsigned);
> const char *printbuf_str(const struct printbuf *);
> void printbuf_exit(struct printbuf *);
>
> +void prt_newline(struct printbuf *);
> +void printbuf_indent_add(struct printbuf *, unsigned);
> +void printbuf_indent_sub(struct printbuf *, unsigned);
> +void prt_tab(struct printbuf *);
> +void prt_tab_rjust(struct printbuf *);
> +
> /* Initializer for a heap allocated printbuf: */
> #define PRINTBUF ((struct printbuf) { .heap_allocated = true })
>
> @@ -187,6 +215,8 @@ static inline void printbuf_reset(struct printbuf *buf)
> {
> buf->pos = 0;
> buf->allocation_failure = 0;
> + buf->indent = 0;
> + buf->tabstop = 0;
> }
>
> /**
> diff --git a/lib/printbuf.c b/lib/printbuf.c
> index 8c70128e31..a7f80f63ca 100644
> --- a/lib/printbuf.c
> +++ b/lib/printbuf.c
> @@ -12,6 +12,11 @@
> #include <linux/slab.h>
> #include <linux/printbuf.h>
>
> +static inline size_t printbuf_linelen(struct printbuf *buf)
> +{
> + return buf->pos - buf->last_newline;
> +}
> +
> int printbuf_make_room(struct printbuf *out, unsigned extra)
> {
> unsigned new_size;
> @@ -69,3 +74,123 @@ void printbuf_exit(struct printbuf *buf)
> }
> }
> EXPORT_SYMBOL(printbuf_exit);
> +
> +void prt_newline(struct printbuf *buf)
> +{
> + unsigned i;
> +
> + printbuf_make_room(buf, 1 + buf->indent);
> +
> + __prt_char(buf, '\n');
> +
> + buf->last_newline = buf->pos;
> +
> + for (i = 0; i < buf->indent; i++)
> + __prt_char(buf, ' ');
Why the loop? Don't you have a _chars variant?
> +void printbuf_indent_add(struct printbuf *buf, unsigned spaces)
> +{
> + if (WARN_ON_ONCE(buf->indent + spaces < buf->indent))
> + spaces = 0;
> +
> + buf->indent += spaces;
> + while (spaces--)
> + prt_char(buf, ' ');
> +}
> +EXPORT_SYMBOL(printbuf_indent_add);
> +
> +/**
> + * printbuf_indent_sub - subtract from the current indent level
> + *
> + * @buf: printbuf to control
> + * @spaces: number of spaces to subtract from the current indent level
> + *
> + * Subsequent lines, and the current line if the output position is at the start
> + * of the current line, will be indented by @spaces less spaces.
> + */
> +void printbuf_indent_sub(struct printbuf *buf, unsigned spaces)
> +{
> + if (WARN_ON_ONCE(spaces > buf->indent))
> + spaces = buf->indent;
> +
> + if (buf->last_newline + buf->indent == buf->pos) {
> + buf->pos -= spaces;
> + printbuf_nul_terminate(buf);
> + }
> + buf->indent -= spaces;
> +}
> +EXPORT_SYMBOL(printbuf_indent_sub);
> +
> +/**
> + * prt_tab - Advance printbuf to the next tabstop
> + *
> + * @buf: printbuf to control
> + *
> + * Advance output to the next tabstop by printing spaces.
> + */
> +void prt_tab(struct printbuf *out)
> +{
> + int spaces = max_t(int, 0, out->tabstops[out->tabstop] - printbuf_linelen(out));
> +
> + BUG_ON(out->tabstop > ARRAY_SIZE(out->tabstops));
So this accesses out->tabstops first, then does a (buggy) bounds check.
And kills the machine if somebody managed to corrupt ->tabstop. Instead
of limping along and living with less-pretty-printed output.
I don't think you'll get this code accepted by the Great Penguin.
Rasmus
