Re: [qemu-s390x] [PATCH v1 1/5] s390-ccw: update libc.h

[Thomas Huth]

On 27.11.2017 21:55, Collin L. Walling wrote:
> Moved:
>   memcmp from bootmap.h to libc.h (renamed from _memcmp)
>   strlen from sclp.c to libc.h (renamed from _strlen)
> 
> Added C standard functions:
>   isdigit
>   atoi
> 
> Added non-C standard function:
>   itostr
> 
> Signed-off-by: Collin L. Walling <address@hidden>
> Acked-by: Christian Borntraeger <address@hidden>
> Reviewed-by: address@hidden
> ---
[...]
> +/* atoi
> + *
> + * Given a string, convert it to an integer. Any
> + * non-numerical value will end the conversion.
> + *
> + * @str - the string to be converted
> + *
> + * @return - an integer converted from the string str
> + */

Looks like you want to use some kind of doc text markup here, but it
does not look like any valid syntax that I know ... may I suggest to use
gtk-doc as this is what most other QEMU developers are using (AFAIK)?

> +static inline int atoi(const char *str)
> +{
> +    int i;
> +    int val = 0;
> +
> +    for (i = 0; str[i]; i++) {
> +        char c = str[i];
> +        if (!isdigit(c)) {
> +            break;
> +        }
> +        val *= 10;
> +        val += c - '0';
> +    }
> +
> +    return val;
> +}
> +
> +/* itostr
> + *
> + * Given an integer, convert it to a string. The string must be allocated
> + * beforehand. The resulting string will be null terminated and returned.
> + *
> + * @str - the integer to be converted
> + * @num - a pointer to a string to store the conversion
> + *
> + * @return - the string of the converted integer
> + */

Smells like a good candidate for hard-to-debug buffer overruns later.
Could you please add a "length" parameter to do some sanity checking of
the destination buffer length, please?

> +static inline char *itostr(int num, char *str)
> +{
> +    int i;
> +    int len = 0;
> +    long tens = 1;
> +
> +    /* Handle 0 */
> +    if (num == 0) {
> +        str[0] = '0';
> +        str[1] = '\0';
> +        return str;
> +    }
> +
> +    /* Count number of digits */
> +    while (num / tens != 0) {
> +        tens *= 10;
> +        len++;
> +    }
> +
> +    /* Convert int -> string */
> +    for (i = 0; i < len; i++) {
> +        tens /= 10;
> +        str[i] = num / tens % 10 + '0';
> +    }
> +
> +    str[i] = '\0';
> +
> +    return str;
> +}

Having such a bigger, and likely performance-uncritical function as an
inline function in a header sounds somewhat wrong. Could you maybe move
this (and atoi()) into a proper .c file (libc.c?) instead, please?

 Thanks,
  Thomas