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

[Thomas Huth]

On 19.02.2018 18:17, Collin L. Walling wrote:
> 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;
> }

Yes, looks fine to me that way (with the "NUL" change mentioned by Eric).

 Thomas