Re: [Qemu-devel] [qemu-s390x] [PATCH v7 04/12] s390-ccw: update libc

["Collin L. Walling" <walling@linux.vnet.ibm.com>]

On 02/19/2018 11:19 AM, Collin L. Walling wrote:
> On 02/19/2018 11:00 AM, Thomas Huth wrote:
>> On 19.02.2018 16:40, Collin L. Walling wrote:
>>> On 02/17/2018 02:48 AM, Thomas Huth wrote:
>>>> On 16.02.2018 23:07, Collin L. Walling wrote:
>>>> [...]
>>>>> +/**
>>>>> + * uitoa:
>>>>> + * @num: an integer (base 10) to be converted.
>>>>> + * @str: a pointer to a string to store the conversion.
>>>>> + * @len: the length of the passed string.
>>>>> + *
>>>>> + * Given an integer @num, convert it to a string. The string @str
>>>>> must be
>>>>> + * allocated beforehand. The resulting string will be null
>>>>> terminated and
>>>>> + * returned. This function only handles numbers between 0 and
>>>>> UINT64_MAX
>>>>> + * inclusive.
>>>>> + *
>>>>> + * Returns: the string @str of the converted integer @num
>>>>> + */
>>>>> +char *uitoa(uint64_t num, char *str, size_t len)
>>>>> +{
>>>>> +    size_t num_idx = 0;
>>>>> +    uint64_t tmp = num;
>>>>> +
>>>>> +    IPL_assert(str != NULL, "uitoa: no space allocated to store
>>>>> string");
>>>>> +
>>>>> +    /* Get index to ones place */
>>>>> +    while ((tmp /= 10) != 0) {
>>>>> +        num_idx++;
>>>>> +    }
>>>>> +
>>>>> +    /* Check if we have enough space for num and null */
>>>>> +    IPL_assert(len > num_idx, "uitoa: array too small for 
>>>>> conversion");
>>>> Well, in v5 of this patch you've had "len >= num_idx + 1" where we
>>>> agreed that it was wrong. Now you have "len > num_idx" which is pretty
>>>> much the same. WTF?
>>>> I still think you need "len > num_idx + 1" here to properly take the
>>>> trailing NUL-byte into account properly. Please fix it!
>>>>
>>>>    Thomas
>>>>
>>> You're correct, and my apologies for not correcting the true problem 
>>> here:
>>> I messed up the value of num_idx.  It is off by one, but 
>>> initializing the
>>> value to 1 instead of 0 should fix this. I must've accounted for 
>>> this in
>>> my test file but forgot to update it in the actual source code.
>> Are you sure that initializing it to 1 is right? Unless you also change
>> the final while loop in this function, this will put the character into
>> the wrong location ("str[num_idx] = num % 10 + '0';"). Imagine a number
>> that only consists of one digit ... the digit will be placed in str[1]
>> which sounds wrong to me...?
>>
>>   Thomas
>>
> There's that, which we can solve by decrementing num_idx at the start 
> of the loop.
> We also have to change the line str[num_idx + 1] = '\0'; to no longer 
> add 1.
> It all boils down to "which way reads better", which I often struggle and
> bounce back-and-forth with (way) too much...
>
> Maybe I should also rename num_idx to just "idx" and let the comments 
> explain
> everything?
>
How is this for a compromise?

     - start num_idx at 1, provide comment as for why
     - change while loop comment to explain we are "counting the 
_indices_ _of_ _num_"
     - str[num_idx] is assigned \0, and we also decrement num_idx in one 
line
     - in conversion loop, post decrement num_idx as it is used

char *uitoa(int num, char *str, int len)
{
   int num_idx = 1; /* account for NULL */
   int tmp = num;

   assert(str != NULL, "uitoa: no space allocated to store string");

   /* Count indices of num */
   while ((tmp /= 10) != 0)
     num_idx++;

   /* Check if we have enough space for num and null */
   assert(len > num_idx, "uitoa: array too small for conversion");

   str[num_idx--] = '\0';

   /* Convert int to string */
   while (num_idx >= 0) {
     str[num_idx--] = num % 10 + '0';
     num /= 10;
   }

   return str;
}


-- 
- Collin L Walling