Re: [qemu-s390x] [Qemu-devel] [PATCH v3 1/8] s390-ccw: update libc

[Eric Blake]

On 01/15/2018 10:44 AM, 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: Janosch Frank <address@hidden>
> ---

> +++ b/pc-bios/s390-ccw/libc.c
> @@ -0,0 +1,83 @@
> +/*
> + * libc-style definitions and functions
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.

I'm not a lawyer, but generically, the GPL and its variants depend on a
copyright owner to actually work.  You may want to add a copyright line.

> + */
> +
> +#include "libc.h"
> +#include "s390-ccw.h"
> +
> +/**
> + * atoi:
> + * @str: the string to be converted.
> + *
> + * Given a string @str, convert it to an integer. Any non-numerical value
> + * will terminate the conversion.
> + *
> + * Returns: an integer converted from the string @str.
> + */
> +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';

Silently gives garbage on integer overflow, but matches the fact that
POSIX atoi() can't flag errors.  However, it does not handle leading
whitespace nor '-', which means it is NOT doing a POSIX-compatible
atoi() implementation; naming it atoi() is perhaps thus a disservice to
end users.

> +    }
> +
> +    return val;
> +}
> +
> +/**
> + * itostr:
> + * @num: the integer 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.
> + *
> + * Returns: the string @str of the converted integer @num.
> + */
> +static char *_itostr(int num, char *str, size_t len)
> +{
> +    long num_len = 1;
> +    int tmp = num;
> +    int i;
> +
> +    /* Count length of num */
> +    while ((tmp /= 10) > 0) {
> +        num_len++;
> +    }
> +
> +    /* Check if we have enough space for num and null */
> +    if (len <= num_len) {
> +        return 0;
> +    }
> +
> +    /* Convert int to string */
> +    for (i = num_len - 1; i >= 0; i--) {
> +        str[i] = num % 10 + '0';
> +        num /= 10;
> +    }

No handling of negative integers?

> +
> +    str[num_len] = '\0';
> +
> +    return str;
> +}
> +
> +char *itostr(int num, char *str, size_t len)
> +{
> +    char *tmp = _itostr(num, str, len);
> +    IPL_assert(tmp != 0, "array too small for itostr conversion");
> +    return tmp;
> +}
> diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h
> index 0142ea8..00268e3 100644
> --- a/pc-bios/s390-ccw/libc.h
> +++ b/pc-bios/s390-ccw/libc.h
> @@ -42,4 +42,35 @@ static inline void *memcpy(void *s1, const void *s2, 
> size_t n)
>      return s1;
>  }
>  
> +static inline int memcmp(const void *s1, const void *s2, size_t n)
> +{
> +    int i;
> +    const uint8_t *p1 = s1, *p2 = s2;
> +
> +    for (i = 0; i < n; i++) {

Are you safe comparing int to size_t, or would it be safer to use size_t
for your iterator?  I guess this shim is unlikely to be abused by
someone trying to compare 2G of memory at once.

> +        if (p1[i] != p2[i]) {
> +            return p1[i] > p2[i] ? 1 : -1;
> +        }
> +    }

Not the fastest of implementations, but that probably doesn't matter
(the complexity of writing a vectored implementation that works on a
long or larger per loop iteration is important in a generic libc, but
less so in a compatibility shim).

> +
> +    return 0;
> +}
> +
> +static inline size_t strlen(const char *str)
> +{
> +    size_t i;
> +    for (i = 0; *str; i++) {
> +        str++;
> +    }
> +    return i;

Again, not the fastest implementation, but that shouldn't matter.

> +}
> +
> +static inline int isdigit(int c)
> +{
> +    return (c >= '0') && (c <= '9');
> +}
> +
> +int atoi(const char *str);
> +char *itostr(int num, char *str, size_t len);
> +
>  #endif
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature