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

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

[Maurizio Lombardi]

When printing a bitmap using the "%*pb[l]" printk format
a 16 bit variable (field_width) is used to store the size of the bitmap.
In some cases 16 bits are not sufficient, the variable overflows and
printk does not work as expected.

This patch fixes the problem by changing the type of field_width to s32.

How to reproduce the bug:

1.load scsi_debug
# modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1

2.create VG
# vgcreate tsvg /dev/sdb
  Physical volume "/dev/sdb" successfully created
  Volume group "tsvg" successfully created

3. Bitmap should be set, but still empty
# cat /sys/bus/pseudo/drivers/scsi_debug/map

Expected results:
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-15

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 lib/vsprintf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 95cd63b..20f91c4 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -384,8 +384,8 @@ struct printf_spec {
 	u8	flags;		/* flags to number() */
 	u8	base;		/* number base, 8, 10 or 16 only */
 	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
-	s16	field_width;	/* width of output field */
 	s16	precision;	/* # of digits/chars */
+	s32	field_width;	/* width of output field */
 };
 
 static noinline_for_stack
-- 
Maurizio Lombardi

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

[Tejun Heo]

On Wed, Sep 09, 2015 at 12:13:10PM +0200, Maurizio Lombardi wrote:
> When printing a bitmap using the "%*pb[l]" printk format
> a 16 bit variable (field_width) is used to store the size of the bitmap.
> In some cases 16 bits are not sufficient, the variable overflows and
> printk does not work as expected.
> 
> This patch fixes the problem by changing the type of field_width to s32.
> 
> How to reproduce the bug:
> 
> 1.load scsi_debug
> # modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1
> 
> 2.create VG
> # vgcreate tsvg /dev/sdb
>   Physical volume "/dev/sdb" successfully created
>   Volume group "tsvg" successfully created
> 
> 3. Bitmap should be set, but still empty
> # cat /sys/bus/pseudo/drivers/scsi_debug/map
> 
> Expected results:
> # cat /sys/bus/pseudo/drivers/scsi_debug/map
> 0-15
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[Joe Perches]

On Wed, 2015-09-09 at 12:13 +0200, Maurizio Lombardi wrote:
> When printing a bitmap using the "%*pb[l]" printk format
> a 16 bit variable (field_width) is used to store the size of the bitmap.
> In some cases 16 bits are not sufficient, the variable overflows and
> printk does not work as expected.

If more than 16 bits are necessary, it couldn't work
as a single printk is limited to 1024 bytes.

> 3. Bitmap should be set, but still empty
> # cat /sys/bus/pseudo/drivers/scsi_debug/map
[]
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -384,8 +384,8 @@ struct printf_spec {
>  	u8	flags;		/* flags to number() */
>  	u8	base;		/* number base, 8, 10 or 16 only */
>  	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
> -	s16	field_width;	/* width of output field */
>  	s16	precision;	/* # of digits/chars */
> +	s32	field_width;	/* width of output field */
>  };
>  
>  static noinline_for_stack

And this makes the sizeof struct printf_spec more than
8 bytes which isn't desireable on x86-32.

%*pb is meant for smallish bitmaps, not big ones.

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

[Tejun Heo]

Hello,

On Wed, Sep 09, 2015 at 09:33:52AM -0700, Joe Perches wrote:
> On Wed, 2015-09-09 at 12:13 +0200, Maurizio Lombardi wrote:
> > When printing a bitmap using the "%*pb[l]" printk format
> > a 16 bit variable (field_width) is used to store the size of the bitmap.
> > In some cases 16 bits are not sufficient, the variable overflows and
> > printk does not work as expected.
> 
> If more than 16 bits are necessary, it couldn't work
> as a single printk is limited to 1024 bytes.

It's weird to fail silently tho.  What we can do is just capping it at
16bit max and append something to indicate that the bitmap has been
truncated if truncation actually happened.

> > 3. Bitmap should be set, but still empty
> > # cat /sys/bus/pseudo/drivers/scsi_debug/map
> []
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > @@ -384,8 +384,8 @@ struct printf_spec {
> >  	u8	flags;		/* flags to number() */
> >  	u8	base;		/* number base, 8, 10 or 16 only */
> >  	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
> > -	s16	field_width;	/* width of output field */
> >  	s16	precision;	/* # of digits/chars */
> > +	s32	field_width;	/* width of output field */
> >  };
> >  
> >  static noinline_for_stack
> 
> And this makes the sizeof struct printf_spec more than
> 8 bytes which isn't desireable on x86-32.

Ah, right, we pass this around on stack.

Thanks.

-- 
tejun

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

[Joe Perches]

On Wed, 2015-09-09 at 12:36 -0400, Tejun Heo wrote:
> On Wed, Sep 09, 2015 at 09:33:52AM -0700, Joe Perches wrote:
> > On Wed, 2015-09-09 at 12:13 +0200, Maurizio Lombardi wrote:
> > > When printing a bitmap using the "%*pb[l]" printk format
> > > a 16 bit variable (field_width) is used to store the size of the bitmap.
> > > In some cases 16 bits are not sufficient, the variable overflows and
> > > printk does not work as expected.
> > 
> > If more than 16 bits are necessary, it couldn't work
> > as a single printk is limited to 1024 bytes.
> 
> It's weird to fail silently tho.  What we can do is just capping it at
> 16bit max and append something to indicate that the bitmap has been
> truncated if truncation actually happened.

It might be reasonable to output ... after the last
comma or dash of the bitmap output when buf >= end.

It also might be better to not reuse spec in
bitmap_string and bitmap_list_string.

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

[Rasmus Villemoes]

On Wed, Sep 09 2015, Joe Perches <joe@perches.com> wrote:

> On Wed, 2015-09-09 at 12:13 +0200, Maurizio Lombardi wrote:
>> When printing a bitmap using the "%*pb[l]" printk format
>> a 16 bit variable (field_width) is used to store the size of the bitmap.
>> In some cases 16 bits are not sufficient, the variable overflows and
>> printk does not work as expected.
>
> If more than 16 bits are necessary, it couldn't work
> as a single printk is limited to 1024 bytes.

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).

>> 3. Bitmap should be set, but still empty
>> # cat /sys/bus/pseudo/drivers/scsi_debug/map
> []
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
>> @@ -384,8 +384,8 @@ struct printf_spec {
>>  	u8	flags;		/* flags to number() */
>>  	u8	base;		/* number base, 8, 10 or 16 only */
>>  	u8	qualifier;	/* number qualifier, one of 'hHlLtzZ' */
>> -	s16	field_width;	/* width of output field */
>>  	s16	precision;	/* # of digits/chars */
>> +	s32	field_width;	/* width of output field */
>>  };
>>  
>>  static noinline_for_stack
>
> And this makes the sizeof struct printf_spec more than
> 8 bytes which isn't desireable on x86-32.

Or other architectures, I imagine. I'm pretty sure struct printf_spec
purposely has sizeof==8 to allow it to be (relatively cheaply) passed
around by value.

> %*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?

Rasmus

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

[Joe Perches]

On Wed, 2015-09-09 at 20:51 +0200, Rasmus Villemoes wrote:
> On Wed, Sep 09 2015, Joe Perches <joe@perches.com> wrote:
> > this makes the sizeof struct printf_spec more than
> > 8 bytes which isn't desireable on x86-32.
> 
> I'm pretty sure struct printf_spec
> purposely has sizeof==8 to allow it to be (relatively cheaply) passed
> around by value.

True.  https://lkml.org/lkml/2010/3/6/141

> > %*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?

No idea.  Tejun?

Perhaps the code in lib/vsprintf.c that sets
spec.field_width and spec.precision from format argument
input should be changed to use a temporary int and
clamped to S16_MIN -> S16_MAX.

Something like:
---
 lib/vsprintf.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7f0cdd2..2782129 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1913,13 +1913,21 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 		}
 
-		case FORMAT_TYPE_WIDTH:
-			spec.field_width = va_arg(args, int);
+		case FORMAT_TYPE_WIDTH: {
+			int tmp = va_arg(args, int);
+
+			spec.field_width = (s16)clamp_t(int, tmp,
+							S16_MIN, S16_MAX);
 			break;
+		}
 
-		case FORMAT_TYPE_PRECISION:
-			spec.precision = va_arg(args, int);
+		case FORMAT_TYPE_PRECISION: {
+			int tmp = va_arg(args, int);
+
+			spec.precision = (s16)clamp_t(int, tmp,
+						      S16_MIN, S16_MAX);
 			break;
+		}
 
 		case FORMAT_TYPE_CHAR: {
 			char c;

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

[Tejun Heo]

Hello, Joe.

On Wed, Sep 09, 2015 at 12:26:39PM -0700, Joe Perches wrote:
> > > %*pb is meant for smallish bitmaps, not big ones.

I'm not so sure about that.  It's one thing to truncate printk output
cuz the actual formatted result is too long but crippling the entire
printf family of functions regardless of the actual output, which
might as well be just "7-65540", doesn't seem like a good idea.  These
are pretty basic library functions and people shouldn't need to worry
about niggles like that.

> > 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?
> 
> No idea.  Tejun?

The use case isn't from me, but why not?

> Perhaps the code in lib/vsprintf.c that sets
> spec.field_width and spec.precision from format argument
> input should be changed to use a temporary int and
> clamped to S16_MIN -> S16_MAX.

I think it should be able to handle bitmaps >64k.  We don't want to
cripple the fundamental string formatter because we can't pass around
struct larger than 8 bytes.  That just reeks of poor decision making.
Why are we even copying the struct on invocations?  Only some
functions modify the values after all.  We might as well pass around
pointer to the struct and let the callees wihch modify them copy the
fieldsin local vars like normal functions.

Thanks.

-- 
tejun

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

[Joe Perches]

On Thu, 2015-09-10 at 10:36 -0400, Tejun Heo wrote:
> On Wed, Sep 09, 2015 at 12:26:39PM -0700, Joe Perches wrote:
> > > > %*pb is meant for smallish bitmaps, not big ones.
[]
> The use case isn't from me, but why not?

Imagine the output of the 500k bitmap if every other
bit is set.

%*pb isn't capable of multiple line output and
seq_printf output would also fail as it uses k.alloc
memory not vmalloc.

I think that a more limited mechanism might be to use a
multiple line oriented function like print_hex_debug
and not try to emit the entire thing in a single go.

> Why are we even copying the struct on invocations?
> Only some functions modify the values after all.
> We might as well pass around pointer to the struct
> and let the callees wihch modify them copy the
> fields in local vars like normal functions.

You are of course welcome and able to change it.

btw: the current implementation has a limitation
on 32 bit arches as it uses an int argument for the
unsigned long count of bits in a bitmap.

That's a bitmap that should not be printed anyway.

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

[Tejun Heo]

Hello, Joe.

On Thu, Sep 10, 2015 at 08:41:25AM -0700, Joe Perches wrote:
> On Thu, 2015-09-10 at 10:36 -0400, Tejun Heo wrote:
> > On Wed, Sep 09, 2015 at 12:26:39PM -0700, Joe Perches wrote:
> > > > > %*pb is meant for smallish bitmaps, not big ones.
> []
> > The use case isn't from me, but why not?
> 
> Imagine the output of the 500k bitmap if every other
> bit is set.

Yeah, but the caller still should be able to call, say, scnprintf()
with a limited buffer and get the output till the end of the buffer
along with the indication that the output has been truncated.

> %*pb isn't capable of multiple line output and
> seq_printf output would also fail as it uses k.alloc
> memory not vmalloc.

Heh, and it should fail.

> I think that a more limited mechanism might be to use a
> multiple line oriented function like print_hex_debug
> and not try to emit the entire thing in a single go.

I'm not that worried about the berserk cases which try to print a
really long output on consoles but the subtle failure modes are
worrying.

> > Why are we even copying the struct on invocations?
> > Only some functions modify the values after all.
> > We might as well pass around pointer to the struct
> > and let the callees wihch modify them copy the
> > fields in local vars like normal functions.
> 
> You are of course welcome and able to change it.
> 
> btw: the current implementation has a limitation
> on 32 bit arches as it uses an int argument for the
> unsigned long count of bits in a bitmap.
> 
> That's a bitmap that should not be printed anyway.

That's 256Mbytes of bitmap.  I don't think we need to worry about that
at the moment.

Thanks.

-- 
tejun

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

[Maurizio Lombardi]

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.

Joe, you mentioned that *pbl is meant for small bitmaps, what should I
use with big ones?

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.

Thanks,
Maurizio Lombardi

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

[Maurizio Lombardi]

On 09/10/2015 09:04 AM, Maurizio Lombardi wrote:
> 
> scsi-debug used the bitmap_scnlistprintf() function but since commit
> dbc760bcc150cc27160f0131b15db76350df4334 this function is just a wrapper
> around scnprintf("%*pbl");

I just want to add that since commit
46385326cc1577587ed3e7432c2425cf6d3e4308 the bitmap_scnlistprintf()
function has been completely removed because " all the bitmap formatting
usages have been converted to '%*pb[l]' ".

Thanks,
Maurizio Lombardi

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

[Joe Perches]

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.

vsnprintf / printk as used in the kernel does have
limitations that user mode vsnprintf does not.

> Joe, you mentioned that *pbl is meant for small bitmaps, what should I
> use with big ones?

Something else?

> 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.

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

[Rasmus Villemoes]

On Thu, Sep 10 2015, Joe Perches <joe@perches.com> 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

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

[Joe Perches]

On Thu, 2015-09-10 at 09:56 +0200, Rasmus Villemoes wrote:
> On Thu, Sep 10 2015, Joe Perches <joe@perches.com> wrote:
> > On Thu, 2015-09-10 at 09:04 +0200, Maurizio Lombardi wrote:
> >> 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.

But at least it should work for the bitmap sized <= S16_MAX
which should be the majority of uses.

> 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)

Yes, but it still couldn't work without the ability to have
large output buffers and printk doesn't support that.

seq_printf might have some performance issue with it too as
it would repetitively try to emit, fail, and grow the buffer.

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

[Tejun Heo]

Hello,

On Thu, Sep 10, 2015 at 01:17:36AM -0700, Joe Perches wrote:
> > It also won't work for the case at hand if/when the actual bitmap ever
> > gets a bit set beyond S16_MAX.
> 
> But at least it should work for the bitmap sized <= S16_MAX
> which should be the majority of uses.

The failure mode is too subtle and this is a utility function which is
too fundamental.  I think we really should just get it working
properly.

> > 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)
> 
> Yes, but it still couldn't work without the ability to have
> large output buffers and printk doesn't support that.

printk overrunning its output buffer is fine.  We've always had that
and while we probably should do a better job of indicating those
events, when the output line runs off that long, people (and it's
usually human minds that process printk outputs) already kinda suspect
that.

Long bitmaps don't mean long output and silently formatting the wrong
output sounds like a pretty bad idea and that because we can't make
the width field 32bit?  It doesn't make any sense.

> seq_printf might have some performance issue with it too as
> it would repetitively try to emit, fail, and grow the buffer.

That's kinda beside the point here.

Thanks.

-- 
tejun

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

[Maurizio Lombardi]

On 09/10/2015 09:38 AM, Joe Perches wrote:ing 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.

I tested your patch, it works for the simple cases where bits are set
below the S16_MAX limit, example:

# modprobe scsi-debug dev_size_mb=256 lbpu=1 lbpws10=1
# vgcreate tsvg /dev/sdb
# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-15   <--- OK!

# lvcreate -V200m -l99%FREE -T tsvg/pool -n lv1 --discards ignore

# cat /sys/bus/pseudo/drivers/scsi_debug/map
0-31,2048-2055    <---- NO! It should be "0-31,2048-2055,501760-501871"!

I think it's misleading.
Can I suggest as a temporary solution to restore the old
bitmap_scnlistprintf() function?

> 
> Perhaps the thin wrapper conversions in lib/bitmap.c
> in that commit for bitmap_scnprintf, bscnl_emit, and 
> bitmap_scnlistprintf should be reverted.
> 
> 

Thanks,
Maurizio Lombardi