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

[Collin L. Walling]

On 01/15/2018 12:05 PM, Eric Blake wrote:
> 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.


I'll have to check on that.  I was not the original author of the libc.h 
file.


> > + */
> > +
> > +#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.


Fair enough. Perhaps the "strtoi" convention suits this better.


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


It's not necessary for the purpose it serves.  The functions that
call itostr do some preprocessing to reject the negative sign and
the string.  We only care about positive numbers in the use cases.


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


It probably makes more sense to declare i as size_t, at least for the sake
of consistency.  I'll do some clean up.


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


--
- Collin L Walling