| Message ID | 20211012182647.1605095-5-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improvements to %pGp | expand |
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) ? > + cmp_buf[size++] = '|'; > + cmp_buf[size] = '\0'; > } > > page_flags |= (values[i] & pft[i].mask) << pft[i].shift; > -- > 2.32.0 >
On Wed, Oct 13, 2021 at 01:22:10PM +0800, 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) > ? Yeah, looks like off-by-one to me.
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. Alternatively, the right pattern is "size += scnprintf(cmp_buf + size, BUF_SIZE - size)", since that will eventually saturate size at BUF_SIZE-1. Rasmus
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
On 10/13/21 2:57 PM, Kirill A. Shutemov wrote: > On Wed, Oct 13, 2021 at 01:22:10PM +0800, 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) >> ? > > Yeah, looks like off-by-one to me. Agreed.
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) { + cmp_buf[size++] = '|'; + cmp_buf[size] = '\0'; } page_flags |= (values[i] & pft[i].mask) << pft[i].shift;
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(-)