Date:	Wed, 16 Sep 2015 14:27:23 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Maurizio Lombardi <mlombard@...hat.com>
Cc:	tj@...nel.org, joe@...ches.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/3] fix *pbl format support

On Wed, Sep 16 2015, Maurizio Lombardi <mlombard@...hat.com> wrote:

> Hi,
>
> I tried to fix the "*pb[l]" format issue while taking care of the problems
> discussed in this thread:
>
> https://lkml.org/lkml/2015/9/9/153
>
> I would like to know whether this approach is more acceptable to you:
>
> PATCH 1 modifies the code so that the printf_spec struct is not passed by value
> anymore, instead a const pointer is used and the structure is copied to
> a local variable only when necessary.
>

If we want to fix the problem with 3/3, then this seems obviously
necessary. There may be stuff we want to optimize later (for example, I
don't think we should always make a local copy of the entire struct; if
we're only modifying one of the fields, it's better to copy that field
to a local variable and use that).

Nit: Please don't say that the parameter is passed around _on the
stack_. Making it fit in 8 bytes is very much so that sane architectures
have a chance to pass it in a register, and _how_ parameters are passed
around is in any case very arch-dependent. Just say "struct printf_spec
is passed by value".

>
> PATCH 2 modifies the bitmap_*_string() functions so they'll append
>"..." to the
> output string whenever the buffer is not sufficiently large.
>
> example of output:
>
> *pb: cccccccc,...
> *pbl: 1-2,5-7,...

This part I really don't like. We shouldn't make the output depend on
the size of the output buffer (other than truncating it if necessary, of
course).

I haven't looked carefully at your code, but it does seem that you make
sure that at least the return value is as expected, which will make
kasprintf work. But it seems there is another kasprintf
problem. [reminder: kasprintf works by doing a va_copy, then doing a
first call of vsnprintf, passing NULL for the buffer and 0 for the
length to determine the size to allocate, and then doing the actual
formatting with a second call]

+	if (buf >= end && buf_start != end) {
+		int spc = 0;
+		char *trunc = end - 1;
+
+		while (trunc > buf_start) {
+			if (*trunc == ',' && spc > 3) {
+				trunc++;
+				break;
+			}
+			trunc--;
+			spc++;
+		}

On the first call from kasprintf, we have end == NULL + 0 == NULL.
Suppose the format is "hello world %pb". By the time the bitmap helper
is called, we have advanced buf away from end, so the stored buf_start
is != end, and also of course buf >= end. Then we set trunc = (void*)-1,
and trunc will continue to be > buf_start for a very very long time...

I may have misread, or it might be fixable, but I really don't like
playing these subtle games. snprintf already provides a method to
reliably detect truncation; it is up to the user to decide whether and
how to deal with that. But yes, this of course requires that snprintf
actually attempted to format the entire bitmap, which in turn requires
some way to pass the correct size all the way through to the bitmap
formatter.

> PATCH 3 increases the size of printf_spec.field_width (from s16 to s32).

I'm not yet completely convinced this is the right solution. Obviously,
if other problems with the small .field_width size show up, this might
be necessary, but as long as it's only the %pb formatter (and so far
only a single user of that), I think smaller/other hammers should be
thought about. So far I think there've been two alternatives: (1)
reintroduce the dedicated bitmap pretty printer(s), (2) my half-ugly
proposal allowing the user to pass struct printf_bitmap to the %pbh[l]
specifier. I'll try to actually code up (2).

Rasmus

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

Powered by blists - more mailing lists