Xen devel - Nov 2013 - [PATCH URGENT] common/vsprintf: Fix signed->unsigned error, causing glacial performance.

The original patch for

  c/s 67a3542c5bc356e6452d8305991617c875f87de4
  "common/vsprintf: Refactor string() out of vsnprintf()"

specifically used signed integers, identical to the code copied out of vsprintf.

When committed, these had changed to unsigned integers, which causes a
functional change.  This causes glacial boot performance and an excessive
quantity of spaces printed to the serial console, as we loop to the upper
bound of a 32bit integer.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/common/vsprintf.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index e8f45eb..43dc392 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -239,7 +239,7 @@ static char *number(
 static char *string(char *str, char *end, const char *s,
                     int field_width, int precision, int flags)
 {
-    unsigned int i, len = strnlen(s, precision);
+    int i, len = strnlen(s, precision);
 
     if (!(flags & LEFT)) {
         while (len < field_width--) {
-- 
1.7.10.4

>>> On 12.11.13 at 17:10, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> The original patch for
> 
>   c/s 67a3542c5bc356e6452d8305991617c875f87de4
>   "common/vsprintf: Refactor string() out of vsnprintf()"
> 
> specifically used signed integers, identical to the code copied out of 
> vsprintf.
> 
> When committed, these had changed to unsigned integers, which causes a
> functional change.  This causes glacial boot performance and an excessive
> quantity of spaces printed to the serial console, as we loop to the upper
> bound of a 32bit integer.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Considering that I had changed those on the fly while committing,
I committed this one without waiting for an ack. But ...
> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -239,7 +239,7 @@ static char *number(
>  static char *string(char *str, char *end, const char *s,
>                      int field_width, int precision, int flags)
>  {
> -    unsigned int i, len = strnlen(s, precision);
> +    int i, len = strnlen(s, precision);
... this just looks _so_ wrong (and whenever I''ll come across
this again, I''ll just be tempted again to adjust it)!

Jan

On 12/11/13 16:23, Jan Beulich wrote:
>>>> On 12.11.13 at 17:10, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> The original patch for
>>
>>   c/s 67a3542c5bc356e6452d8305991617c875f87de4
>>   "common/vsprintf: Refactor string() out of vsnprintf()"
>>
>> specifically used signed integers, identical to the code copied out of 
>> vsprintf.
>>
>> When committed, these had changed to unsigned integers, which causes a
>> functional change.  This causes glacial boot performance and an
excessive
>> quantity of spaces printed to the serial console, as we loop to the
upper
>> bound of a 32bit integer.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Considering that I had changed those on the fly while committing,
> I committed this one without waiting for an ack. But ...
>
>> --- a/xen/common/vsprintf.c
>> +++ b/xen/common/vsprintf.c
>> @@ -239,7 +239,7 @@ static char *number(
>>  static char *string(char *str, char *end, const char *s,
>>                      int field_width, int precision, int flags)
>>  {
>> -    unsigned int i, len = strnlen(s, precision);
>> +    int i, len = strnlen(s, precision);
> ... this just looks _so_ wrong (and whenever I''ll come across
> this again, I''ll just be tempted again to adjust it)!
>
> Jan
>
I agree in general, and do err on the side of unsigned whenever
possible.  In this case, I went with exactly what was present before.

~Andrew

>>> On 12.11.13 at 18:04, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 12/11/13 16:23, Jan Beulich wrote:
>>>>> On 12.11.13 at 17:10, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/common/vsprintf.c
>>> +++ b/xen/common/vsprintf.c
>>> @@ -239,7 +239,7 @@ static char *number(
>>>  static char *string(char *str, char *end, const char *s,
>>>                      int field_width, int precision, int flags)
>>>  {
>>> -    unsigned int i, len = strnlen(s, precision);
>>> +    int i, len = strnlen(s, precision);
>> ... this just looks _so_ wrong (and whenever I''ll come across
>> this again, I''ll just be tempted again to adjust it)!
> 
> I agree in general, and do err on the side of unsigned whenever
> possible.  In this case, I went with exactly what was present before.
And after having committed your fixup, I realized that I should
have made them explicitly "signed" (and going forward we
should try to do so elsewhere when signedness really matters).

Jan