Re: [PATCH v7 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases

[Yury Norov]

On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <yury.norov@xxxxxxxxx> wrote:
> > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > The added test items cover both cases where bitmap buf of the printed
> > > > result is greater than and less than 4KB.
> > > > And it also covers the case where offset for bitmap_print_to_buf is
> > > > non-zero which will happen when printed buf is larger than one page
> > > > in sysfs bin_attribute.
> > >
> > > More test cases is always a good thing, thanks!
> >
> > Generally yes. But in this case... I believe, Barry didn't write that
> > huge line below by himself. Most probably he copy-pasted the output of
> > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > and just enforces current behavior of snprintf.
> 
> I'm not sure I got what you are telling me. The big line is to test
> strings that are bigger than 4k.

I'm trying to say that human are not able to verify correctness of
this line. The test is supposed to check bitmap_print_to_buf(), but
reference output itself is generated by bitmap_print_to_buf(). This
test will always pass by design, even if there's an error somewhere
in the middle, isn't it?

> 
> ...
> 
> > > > +static const char large_list[] __initconst = /* more than 4KB */
> > > > +   "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > +   "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > > > +   "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
> >
> > I don't like this behavior of the code: each individual line is not a
> > valid bitmap_list. I would prefer to split original bitmap and print
> > list representation of parts in a compatible format; considering a
> > receiving part of this splitting machinery.
> 
> I agree that split is not the best here, but after all it's only 1
> line and this is on purpose.

What I see is that bitmap_print_to_buf() is called many times, and
each time it returns something that is not a valid bitmap list string.
If the caller was be able to concatenate all the lines returned by
bitmap_print_to_buf(), he'd probably get correct result. But in such
case, why don't he use scnprintf("pbl") directly?

If there exists a real case where scnprintf("pbl") output is too long
(or execution time is too slow) that we need to split, the right
approach is to split the original bitmap, not an output of
scnprintf("pbl").

And I still don't understand, what prevents the higher levels from
calling the scnprintf() directly in this specific case?