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

[Thomas Huth <thuth@redhat.com>]

On 23.01.2018 23:33, Collin L. Walling wrote:
> On 01/23/2018 02:23 PM, Eric Blake wrote:
>> On 01/23/2018 12:26 PM, Collin L. Walling wrote:
>>> [...]
>>> +/**
>>> + * atoi:
>>> + * @str: the string to be converted.
>>> + *
>>> + * Given a string @str, convert it to an integer. Leading whitespace is
>>> + * ignored. The first character (after any whitespace) is checked
>>> for the
>>> + * negative sign. Any other non-numerical value will terminate the
>>> + * conversion.
>>> + *
>>> + * Returns: an integer converted from the string @str.
>>> + */
>>> +int atoi(const char *str)
>>> +{
>>> +    int val = 0;
>>> +    int sign = 1;
>>> +
>>> +    if (!str || !str[0]) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    while (*str == ' ') {
>>> +        str++;
>>> +    }
>> That's not "any whitespace", but only spaces.  A fully compliant
>> implementation would be checking isspace(), but I don't expect you to
>> implement that; at a minimum, also checking '\t' would get you closer
>> (but not all the way to) compliance.
> 
> 
> I'll fix the comment to be more clear.
> 
> I think it's okay to just have the menu code treat any other kind
> of whitespace as an error (it will check before calling atoi). I
> added support for negatives in bothfunctions because it was easy
> enough to do so and for the sakeof completeness.
> 
> However, I worry trying to be 100% compliant will just bloat the
> code when we only need it for very specific use cases.
> 
> Would you say what we have (along with the fix to itostr below) is
> sufficient enough?

IMHO the current way is good enough for a BIOS implementation. We're not
doing a full replacement of glibc here ;-)

> 
>>
>>
>>> +static char *_itostr(int num, char *str, size_t len)
>>> +{
>>> +    int num_idx = 0;
>>> +    int tmp = num;
>>> +    char sign = 0;
>>> +
>>> +    if (!str) {
>>> +        return NULL;
>>> +    }
>>> +
>>> +    /* Get index to ones place */
>>> +    while ((tmp /= 10) != 0) {
>>> +        num_idx++;
>>> +    }
>>> +
>>> +    if (num < 0) {
>>> +        num *= -1;
>>> +        sign = 1;
>>> +    }
>> If num == INT_MIN, then num is still negative at this point...
>>
>>> +
>>> +    /* Check if we have enough space for num, sign, and null */
>>> +    if (len <= num_idx + sign + 1) {
>>> +        return NULL;
>>> +    }
>>> +
>>> +    str[num_idx + sign + 1] = '\0';
>>> +
>>> +    /* Convert int to string */
>>> +    while (num_idx >= 0) {
>>> +        str[num_idx + sign] = num % 10 + '0';
>> ...which breaks this.
>>
>> Either make it work, or document the corner case as unsupported.
>>
> 
> Might as well just make it work at this point:
> 
> #define INT32_MIN 0x80000000
> 
> static char *itostr(int num, char *str, size_t len)
> {
>     int num_idx = 0;
>     int tmp = num;
>     char sign = !!(num & INT32_MIN);
> 
>     if (!str) {
>         return NULL;
>     }
> 
>     /* Get index to ones place */
>     while ((tmp /= 10) != 0) {
>         num_idx++;
>     }
> 
>     /* Check if we have enough space for num, sign, and null */
>     if (len <= num_idx + sign + 1) {
>         return NULL;
>     }
> 
>     str[num_idx + sign + 1] = '\0';
> 
>     if (sign) {
>         str[0] = '-';
>         if (num == INT32_MIN) {
>             str[num_idx + sign] = '8';
>             num /= 10;
>             num_idx--;
>         }
>         num *= -1;
>     }
> 
>     /* Convert int to string */
>     while (num_idx >= 0) {
>         str[num_idx + sign] = num % 10 + '0';
>         num /= 10;
>         num_idx--;
>     }
> 
>     return str;
> }
> 
> Thoughts?

Looks fine to me. With that modification:

Reviewed-by: Thomas Huth <thuth@redhat.com>