Re: [kvm-unit-tests PATCH v2 1/4] lib/string: Add strnlen, strrchr and strtoul

[Nikos Nikoleris <nikos.nikoleris@arm.com>]

Hi Drew,

On 22/03/2021 08:35, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 06:07:24PM +0000, Nikos Nikoleris wrote:
>> This change adds two functions from <string.h> and one from <stdlib.h>
>> in preparation for an update in libfdt.
>>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>>   lib/stdlib.h | 12 +++++++++
>>   lib/string.h |  4 ++-
>>   lib/string.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++------
>>   3 files changed, 77 insertions(+), 9 deletions(-)
>>   create mode 100644 lib/stdlib.h
>>
>> diff --git a/lib/stdlib.h b/lib/stdlib.h
>> new file mode 100644
>> index 0000000..e8abe75
>> --- /dev/null
>> +++ b/lib/stdlib.h
>> @@ -0,0 +1,12 @@
>> +/*
>> + * Header for libc stdlib functions
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU Library General Public License version 2.
>> + */
>> +#ifndef _STDLIB_H_
>> +#define _STDLIB_H_
>> +
>> +unsigned long int strtoul(const char *nptr, char **endptr, int base);
>> +
>> +#endif /* _STDLIB_H_ */
>> diff --git a/lib/string.h b/lib/string.h
>> index 493d51b..8da687e 100644
>> --- a/lib/string.h
>> +++ b/lib/string.h
>> @@ -7,12 +7,14 @@
>>   #ifndef __STRING_H
>>   #define __STRING_H
>>   
>> -extern unsigned long strlen(const char *buf);
>> +extern size_t strlen(const char *buf);
>> +extern size_t strnlen(const char *buf, size_t maxlen);
>>   extern char *strcat(char *dest, const char *src);
>>   extern char *strcpy(char *dest, const char *src);
>>   extern int strcmp(const char *a, const char *b);
>>   extern int strncmp(const char *a, const char *b, size_t n);
>>   extern char *strchr(const char *s, int c);
>> +extern char *strrchr(const char *s, int c);
>>   extern char *strstr(const char *haystack, const char *needle);
>>   extern void *memset(void *s, int c, size_t n);
>>   extern void *memcpy(void *dest, const void *src, size_t n);
>> diff --git a/lib/string.c b/lib/string.c
>> index 75257f5..f77881f 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -6,16 +6,26 @@
>>    */
>>   
>>   #include "libcflat.h"
>> +#include "stdlib.h"
>>   
>> -unsigned long strlen(const char *buf)
>> +size_t strlen(const char *buf)
>>   {
>> -    unsigned long len = 0;
>> +    size_t len = 0;
>>   
>>       while (*buf++)
>>   	++len;
>>       return len;
>>   }
>>   
>> +size_t strnlen(const char *buf, size_t maxlen)
>> +{
>> +    const char *sc;
>> +
>> +    for (sc = buf; maxlen-- && *sc != '\0'; ++sc)
>> +        /* nothing */;
>> +    return sc - buf;
>> +}
>> +
>>   char *strcat(char *dest, const char *src)
>>   {
>>       char *p = dest;
>> @@ -55,6 +65,16 @@ char *strchr(const char *s, int c)
>>       return (char *)s;
>>   }
>>   
>> +char *strrchr(const char *s, int c)
>> +{
>> +    const char *last = NULL;
>> +    do {
>> +        if (*s == (char)c)
>> +            last = s;
>> +    } while (*s++);
>> +    return (char *)last;
>> +}
>> +
>>   char *strstr(const char *s1, const char *s2)
>>   {
>>       size_t l1, l2;
>> @@ -135,13 +155,21 @@ void *memchr(const void *s, int c, size_t n)
>>       return NULL;
>>   }
>>   
>> -long atol(const char *ptr)
>> +static int isspace(int c)
>> +{
>> +    return c == ' ' || c == '\t' || c == '\f' || c == '\n' || c == '\r';
> 
> I added \v. We don't need to do it for this patch, but at some point we
> should consider adding a ctype.h file and consolidating all these is*
> functions. There's a few in lib/argv.c too.
> 

I agree.

>> +}
>> +
>> +unsigned long int strtoul(const char *nptr, char **endptr, int base)
>>   {
>>       long acc = 0;
>> -    const char *s = ptr;
>> +    const char *s = nptr;
>>       int neg, c;
>>   
>> -    while (*s == ' ' || *s == '\t')
>> +    if (base < 0 || base == 1 || base > 32)
>> +        goto out; // errno = EINVAL
> 
> I changed this to
> 
>   assert(base == 0 || (base >= 2 && base <= 36));
> 
> Any reason why you weren't allowing bases 33 - 36?
> 

I was going through the manpage for strtoul and I got confused. 36 is 
the right value.

I wasn't sure if we should assert, the manpage seems to imply that it 
will return without converting and set the errno and endptr. I guess it 
might be better to assert().

>> +
>> +    while (isspace(*s))
>>           s++;
>>       if (*s == '-'){
>>           neg = 1;
>> @@ -152,20 +180,46 @@ long atol(const char *ptr)
>>               s++;
>>       }
>>   
>> +    if (base == 0 || base == 16) {
>> +        if (*s == '0') {
>> +            s++;
>> +            if (*s == 'x') {
> 
> I changed this to (*s == 'x' || *s == 'X')
> 

Here my intent was to not parse 0X as a valid prefix for base 16, 0X is 
not in the manpage.

>> +                 s++;
>> +                 base = 16;
>> +            } else if (base == 0)
>> +                 base = 8;
>> +        } else if (base == 0)
>> +            base = 10;
>> +    }
>> +
>>       while (*s) {
>> -        if (*s < '0' || *s > '9')
>> +        if (*s >= '0' && *s < '0' + base && *s <= '9')
>> +            c = *s - '0';
>> +        else if (*s >= 'a' && *s < 'a' + base - 10)
>> +            c = *s - 'a' + 10;
>> +        else if (*s >= 'A' && *s < 'A' + base - 10)
>> +            c = *s - 'A' + 10;
>> +        else
>>               break;
>> -        c = *s - '0';
>> -        acc = acc * 10 + c;
>> +        acc = acc * base + c;
> 
> I changed this to catch overflow.
> 

Thanks! Some thoughts on the assertion.

>>           s++;
>>       }
>>   
>>       if (neg)
>>           acc = -acc;
>>   
>> + out:
>> +    if (endptr)
>> +        *endptr = (char *)s;
>> +
>>       return acc;
>>   }
>>   
>> +long atol(const char *ptr)
>> +{
>> +    return strtoul(ptr, NULL, 10);
> 
> Since atol should be strtol, I went ahead and also added strtol.
>

Not very important but we could also add it to stdlib.h?


Thanks for the fixes it looks much better now!

Nikos


>> +}
>> +
>>   extern char **environ;
>>   
>>   char *getenv(const char *name)
>> -- 
>> 2.25.1
>>
> 
> Here's a diff of my changes on top of your patch
> 
> 
> diff --git a/lib/string.c b/lib/string.c
> index 30592c5603c5..b684271bb18f 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -164,21 +164,22 @@ void *memchr(const void *s, int c, size_t n)
>   
>   static int isspace(int c)
>   {
> -    return c == ' ' || c == '\t' || c == '\f' || c == '\n' || c == '\r';
> +    return c == ' ' || c == '\t' || c == '\r' || c == '\n' || c == '\v' || c == '\f';
>   }
>   
> -unsigned long int strtoul(const char *nptr, char **endptr, int base)
> -{
> -    long acc = 0;
> +static unsigned long __strtol(const char *nptr, char **endptr,
> +                              int base, bool is_signed) {
> +    unsigned long acc = 0;
>       const char *s = nptr;
> +    bool overflow;
>       int neg, c;
>   
> -    if (base < 0 || base == 1 || base > 32)
> -        goto out; // errno = EINVAL
> +    assert(base == 0 || (base >= 2 && base <= 36));
>   
>       while (isspace(*s))
>           s++;
> -    if (*s == '-'){
> +
> +    if (*s == '-') {
>           neg = 1;
>           s++;
>       } else {
> @@ -190,7 +191,7 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
>       if (base == 0 || base == 16) {
>           if (*s == '0') {
>               s++;
> -            if (*s == 'x') {
> +            if (*s == 'x' || *s == 'X') {
>                    s++;
>                    base = 16;
>               } else if (base == 0)
> @@ -208,23 +209,46 @@ unsigned long int strtoul(const char *nptr, char **endptr, int base)
>               c = *s - 'A' + 10;
>           else
>               break;
> -        acc = acc * base + c;
> +
> +        if (is_signed) {
> +            long __acc = (long)acc;
> +            overflow = __builtin_smull_overflow(__acc, base, &__acc);
> +            assert(!overflow);
> +            overflow = __builtin_saddl_overflow(__acc, c, &__acc);
> +            assert(!overflow);
> +            acc = (unsigned long)__acc;
> +        } else {
> +            overflow = __builtin_umull_overflow(acc, base, &acc);
> +            assert(!overflow);
> +            overflow = __builtin_uaddl_overflow(acc, c, &acc);
> +            assert(!overflow);
> +        }
> +
>           s++;
>       }
>   
>       if (neg)
>           acc = -acc;
>   
> - out:
>       if (endptr)
>           *endptr = (char *)s;
>   
>       return acc;
>   }
>   
> +long int strtol(const char *nptr, char **endptr, int base)
> +{
> +    return __strtol(nptr, endptr, base, true);
> +}
> +
> +unsigned long int strtoul(const char *nptr, char **endptr, int base)
> +{
> +    return __strtol(nptr, endptr, base, false);
> +}
> +
>   long atol(const char *ptr)
>   {
> -    return strtoul(ptr, NULL, 10);
> +    return strtol(ptr, NULL, 10);
>   }
>   
>   extern char **environ;
> 
> 
> Thanks,
> drew
>