Re: [Qemu-devel] [PATCH 1/8 v5] cutils: unsigned int parsing functions

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3738 bytes --]

On 01/18/2013 10:57 AM, Eduardo Habkost wrote:
> There are lots of duplicate parsing code using strto*() in QEMU, and
> most of that code is broken in one way or another. Even the visitors
> code have duplicate integer parsing code[1]. This introduces functions
> to help parsing unsigned int values: parse_uint() and parse_uint_full().
> 
> Parsing functions for signed ints and floats will be submitted later.
> 
> parse_uint_full() has all the checks made by opts_type_uint64() at
> opts-visitor.c:
> 
>  - Check for NULL (returns -EINVAL)
>  - Check for negative numbers (returns -EINVAL)
>  - Check for empty string (returns -EINVAL)
>  - Check for overflow or other errno values set by strtoll() (returns
>    -errno)
>  - Check for end of string (reject invalid characters after number)
>    (returns -EINVAL)
> 
> parse_uint() does everything above except checking for the end of the
> string, so callers can continue parsing the remainder of string after
> the number.
> 
> Unit tests included.
> 
> [1] string-input-visitor.c:parse_int() could use the same parsing code
>     used by opts-visitor.c:opts_type_int(), instead of duplicating that
>     logic.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>     + *
>     + * If @s is null, or @base is invalid, or @s doesn't start with an
>     + * integer in the syntax above, set *@value to 0, *@endptr to @s, and
>     + * return -EINVAL.
>     + *
>     + * Set @endptr to point right beyond the parsed integer.
>     + *
>     + * If the integer overflows unsigned long long, set *@value to
>     + * ULLONG_MAX, and return -ERANGE.

Is it worth explicitly mentioning that *@endptr is set past the last
digit parsed in the -ERANGE case?  It's implied that it was set beyond
the parsed integer, but did you stop parsing the moment you detected
overflow (and thus *endptr might still be pointing to a digit), or is it
set beyond all possible digits to the first non-digit?

>     +/**
>     + * parse_uint_full:
>     + *
>     + * @s: String to parse
>     + * @value: Destination for parsed integer value
>     + * @base: integer base, between 2 and 36 inclusive, or 0
>     + *
>     + * Parse unsigned integer from entire string
>       *
>       * Have the same behavior of parse_uint(), but with an additional check
>     - * for additional data after the parsed number (in that case, the function
>     - * will return -EINVAL).
>     + * for additional data after the parsed number. If extra characters are present
>     + * after the parsed number, the function will return -EINVAL, and the caller
>     + * should not rely on the value set on *@value.

This says *value is unreliable;

>       */
>      int parse_uint_full(const char *s, unsigned long long *value, int base)
>      {
>     @@ -345,6 +360,7 @@
>              return r;
>          }
>          if (*endp) {
>     +        *value = 0;
>              return -EINVAL;

while this says it is explicitly 0.  Is this an intentional mismatch,
especially given that parse_uint explicitly documents that *value is
always set to a reliable value even on -EINVAL?


> +    /* make sure we reject negative numbers: */
> +    sp = s;
> +    while (isspace((unsigned char)*sp)) {
> +        ++sp;
> +    }
> +    if (*sp == '-') {
> +        r = -EINVAL;
> +        goto out;
> +    }
> +
> +    errno = 0;
> +    val = strtoull(s, &endp, base);

Is it worth a micro-optimization of calling strtoull(sp,...) instead os
strtoull(s,...), to avoid reparsing all the space that we just skipped?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]