[PATCHv2] seq_buf: add seq_buf_do_printk() helper

  Sometimes we use seq_buf to format a string buffer, which
we then pass to printk(). However, in certain situations
the seq_buf string buffer can get too big, exceeding the
PRINTKRB_RECORD_MAX bytes limit, and causing printk() to
truncate the string.

Add a new seq_buf helper. This helper prints the seq_buf
string buffer line by line, using \n as a delimiter,
rather than passing the whole string buffer to printk()
at once.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 include/linux/seq_buf.h |  2 ++
 lib/seq_buf.c           | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)
  

On Tue, 11 Apr 2023 23:38:52 +0900
Sergey Senozhatsky <senozhatsky@chromium.org> wrote:

> Sometimes we use seq_buf to format a string buffer, which
> we then pass to printk(). However, in certain situations
> the seq_buf string buffer can get too big, exceeding the
> PRINTKRB_RECORD_MAX bytes limit, and causing printk() to
> truncate the string.
> 
> Add a new seq_buf helper. This helper prints the seq_buf
> string buffer line by line, using \n as a delimiter,
> rather than passing the whole string buffer to printk()
> at once.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Looks good to me. Want me to pull this in my tree, or do you have
patches dependent on this?

-- Steve
  

On (23/04/11 22:46), Steven Rostedt wrote:
> > Sometimes we use seq_buf to format a string buffer, which
> > we then pass to printk(). However, in certain situations
> > the seq_buf string buffer can get too big, exceeding the
> > PRINTKRB_RECORD_MAX bytes limit, and causing printk() to
> > truncate the string.
> > 
> > Add a new seq_buf helper. This helper prints the seq_buf
> > string buffer line by line, using \n as a delimiter,
> > rather than passing the whole string buffer to printk()
> > at once.
> > 
> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> 
> Looks good to me. Want me to pull this in my tree, or do you have
> patches dependent on this?

Thanks, would be great if you can pull this in your tree. The MM
patches that Yosry is working on will be posted after the merge
window.
  

On Tue, Apr 11, 2023 at 8:08 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (23/04/11 22:46), Steven Rostedt wrote:
> > > Sometimes we use seq_buf to format a string buffer, which
> > > we then pass to printk(). However, in certain situations
> > > the seq_buf string buffer can get too big, exceeding the
> > > PRINTKRB_RECORD_MAX bytes limit, and causing printk() to
> > > truncate the string.
> > >
> > > Add a new seq_buf helper. This helper prints the seq_buf
> > > string buffer line by line, using \n as a delimiter,
> > > rather than passing the whole string buffer to printk()
> > > at once.
> > >
> > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> >
> > Looks good to me. Want me to pull this in my tree, or do you have
> > patches dependent on this?
>
> Thanks, would be great if you can pull this in your tree. The MM
> patches that Yosry is working on will be posted after the merge
> window.

Yes, thanks Sergey and Steven.

The sooner this patch ends up in Linus's tree the better for me so I
can send my MM patches based off of it. I am guessing through your
tree is the easiest way for that.

Thanks again.
  

On Tue 2023-04-11 23:38:52, Sergey Senozhatsky wrote:
> Sometimes we use seq_buf to format a string buffer, which
> we then pass to printk(). However, in certain situations
> the seq_buf string buffer can get too big, exceeding the
> PRINTKRB_RECORD_MAX bytes limit, and causing printk() to
> truncate the string.
> 
> Add a new seq_buf helper. This helper prints the seq_buf
> string buffer line by line, using \n as a delimiter,
> rather than passing the whole string buffer to printk()
> at once.
> 
> --- a/lib/seq_buf.c
> +++ b/lib/seq_buf.c
> @@ -93,6 +93,40 @@ int seq_buf_printf(struct seq_buf *s, const char *fmt, ...)
>  }
>  EXPORT_SYMBOL_GPL(seq_buf_printf);
>  
> +/**
> + * seq_buf_do_printk - printk seq_buf line by line
> + * @s: seq_buf descriptor
> + * @lvl: printk level
> + *
> + * printk()-s a multi-line sequential buffer line by line. The function
> + * makes sure that the buffer in @s is nul terminated and safe to read
> + * as a string.
> + */
> +void seq_buf_do_printk(struct seq_buf *s, const char *lvl)
> +{
> +	const char *start, *lf;
> +	int len;
> +
> +	if (s->size == 0 || s->len == 0)
> +		return;
> +
> +	seq_buf_terminate(s);
> +
> +	start = s->buffer;
> +	while ((lf = strchr(start, '\n'))) {
> +		len = lf - start + 1;
> +		printk("%s%.*s", lvl, len, start);
> +		start = ++lf;
> +	}
> +
> +	/* No trailing LF */
> +	if (start < s->buffer + s->len) {
> +		len = s->buffer + s->len - start;
> +		printk("%s%.*s\n", lvl, len, start);

We know that the string is '\0' terminated, so the last print
might be easier:

	if (start < s->buffer + s->len)
		printk("%s%s\n", lvl, start);

Anyway, it looks good. With or without this change:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr
  

On (23/04/14 11:00), Petr Mladek wrote:
> > +void seq_buf_do_printk(struct seq_buf *s, const char *lvl)
> > +{
> > +	const char *start, *lf;
> > +	int len;
> > +
> > +	if (s->size == 0 || s->len == 0)
> > +		return;
> > +
> > +	seq_buf_terminate(s);
> > +
> > +	start = s->buffer;
> > +	while ((lf = strchr(start, '\n'))) {
> > +		len = lf - start + 1;
> > +		printk("%s%.*s", lvl, len, start);
> > +		start = ++lf;
> > +	}
> > +
> > +	/* No trailing LF */
> > +	if (start < s->buffer + s->len) {
> > +		len = s->buffer + s->len - start;
> > +		printk("%s%.*s\n", lvl, len, start);
> 
> We know that the string is '\0' terminated, so the last print
> might be easier:
> 
> 	if (start < s->buffer + s->len)
> 		printk("%s%s\n", lvl, start);

Indeed. Steven, let me know if you'd prefer a v3.

> Anyway, it looks good. With or without this change:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Thanks Petr!
  

On April 15, 2023 3:04:06 AM EDT, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
>On (23/04/14 11:00), Petr Mladek wrote:
>> > +void seq_buf_do_printk(struct seq_buf *s, const char *lvl)
>> > +{
>> > +	const char *start, *lf;
>> > +	int len;
>> > +
>> > +	if (s->size == 0 || s->len == 0)
>> > +		return;
>> > +
>> > +	seq_buf_terminate(s);
>> > +
>> > +	start = s->buffer;
>> > +	while ((lf = strchr(start, '\n'))) {
>> > +		len = lf - start + 1;
>> > +		printk("%s%.*s", lvl, len, start);
>> > +		start = ++lf;
>> > +	}
>> > +
>> > +	/* No trailing LF */
>> > +	if (start < s->buffer + s->len) {
>> > +		len = s->buffer + s->len - start;
>> > +		printk("%s%.*s\n", lvl, len, start);
>> 
>> We know that the string is '\0' terminated, so the last print
>> might be easier:
>> 
>> 	if (start < s->buffer + s->len)
>> 		printk("%s%s\n", lvl, start);
>
>Indeed. Steven, let me know if you'd prefer a v3.

Sure. Why not.

-- Steve

>
>> Anyway, it looks good. With or without this change:
>> 
>> Reviewed-by: Petr Mladek <pmladek@suse.com>
>
>Thanks Petr!