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

[Collin L. Walling]

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?


> > +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?

--
- Collin L Walling