Re: [PATCH] lib/vsprintf.c: increase the size of the field_width variable

[Rasmus Villemoes]

On Thu, Sep 10 2015, Joe Perches <joe@xxxxxxxxxxx> wrote:

> On Thu, 2015-09-10 at 09:04 +0200, Maurizio Lombardi wrote:
>> Hi Rasmus,
>> 
>> On 09/09/2015 08:51 PM, Rasmus Villemoes wrote:
>> > I'm also a little confused; I don't see what printk has to do with the
>> > reported problem (I'd expect the /sys/... file to be generated by
>> > something like seq_printf).
>> 
>> In the scsi-debug case scnprintf is used, but it doesn't really matter
>> because the change I made would influence printk and all its friends as
>> well... everything that will parse "%*pb[l]".
>> 
>> > 
>> >> %*pb is meant for smallish bitmaps, not big ones.
>> > 
>> > Yup. And that leads to my other confusion: Given that the expected
>> > output is given as "0-15", does the bitmap really consist of > S16_MAX
>> > bits with only the first 16 set?
>> > 
>> 
>> Yes. To be precise, in the example I mentioned in the commit message, a
>> bitmap of size = 524288 bits is created.
>> If you assign this number to a s16 variable the result will be zero and
>> nothing will be printed.
>
> Maurizio, did you try the patch I posted?
> I think it'll work, but it doesn't fix the
> fundamental issue of %*pbl with large bitmaps.

It also won't work for the case at hand if/when the actual bitmap ever
gets a bit set beyond S16_MAX.

>> scsi-debug used the bitmap_scnlistprintf() function but since commit
>> dbc760bcc150cc27160f0131b15db76350df4334 this function is just a wrapper
>> around scnprintf("%*pbl");
>> as a consequence, the scsi-debug map_show() function stopped working
>> correctly.
>
> Perhaps the thin wrapper conversions in lib/bitmap.c
> in that commit for bitmap_scnprintf, bscnl_emit, and 
> bitmap_scnlistprintf should be reverted.

Yes, that's the obvious short-term solution, though I think it's a bit
sad having to have library functions needed only by very few users
(after all, it took 6 months for this to be reported).

A (somewhat ugly?) solution might be to teach %pb another flag, say h (for
huge), meaning that the pointer is actually (struct printf_bitmap*),
with 

struct printf_bitmap { unsigned long *bits; unsigned long nbits; }

Then callers with potentially huge bitmaps would do

struct printf_bitmap tmp = { my_bitmap, my_size };
snprintf("%pbhl", &tmp)

and I think we'd only need a tiny refactorization in lib/vsprintf.c to
support this. (When using this form, the field width would just be
ignored.) Basically, move the simplistic flag handling from the generic
dispatch function pointer() to its own little helper, let that helper
compute/fetch the (unsigned long*) and size we need, and pass that to
either bitmap_list_string or bitmap_string - basically, they'd lose the
spec argument and grow an explicit nbits argument, and then get to
declare their own struct printf_spec.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/