Re: [PATCH 4/5] test_printf: Append '|' more efficiently

[Petr Mladek <pmladek@suse.com>]

On Wed 2021-10-13 11:59:38, Rasmus Villemoes wrote:
> On 13/10/2021 07.22, Yafang Shao wrote:
> > On Wed, Oct 13, 2021 at 2:33 AM Matthew Wilcox (Oracle)
> > <willy@infradead.org> wrote:
> >>
> >> Instead of calling snprintf(), just append '|' by hand.
> >>
> >> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> >> ---
> >>  lib/test_printf.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/test_printf.c b/lib/test_printf.c
> >> index 60cdf4ba991e..662c3785aa57 100644
> >> --- a/lib/test_printf.c
> >> +++ b/lib/test_printf.c
> >> @@ -623,9 +623,9 @@ page_flags_test(int section, int node, int zone, int last_cpupid,
> >>                 if (!pft[i].width)
> >>                         continue;
> >>
> >> -               if (append) {
> >> -                       snprintf(cmp_buf + size, BUF_SIZE - size, "|");
> >> -                       size = strlen(cmp_buf);
> >> +               if (append && size < BUF_SIZE) {
> > 
> > Should it be:
> >                     if (append && size < BUF_SIZE - 1)
>
> Perhaps, but the whole thing screams "don't do it like this". We have
> seq_buf to abstract a "buffer with known length you can do a bunch of
> snprintfs to". That's what should be used. Then the test can also error
> out if seq_buf_has_overflowed(), because that's a bug in the test.

Interesting idea.

> Alternatively, the right pattern is "size += scnprintf(cmp_buf + size,
> BUF_SIZE - size)", since that will eventually saturate size at BUF_SIZE-1.

Yup, this is well known pattern so that it is much easier to make
it correctly and review.

The performance is not important here.

Best Regards,
Petr