Re: [Qemu-devel] [PATCH 1/7] string-input-visitor: Fix uint64 parsing

[Eric Blake <eblake@redhat.com>]

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

On 09/25/2015 06:39 AM, Andreas Färber wrote:
> All integers would get parsed by strtoll(), not handling the case of
> UINT64 properties with the most significient bit set.
> 
> Implement a .type_uint64 visitor callback, reusing the existing
> parse_str() code through a new argument, using strtoull().
> 
> As a bug fix, ignore warnings about preference of qemu_strto[u]ll().
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  qapi/string-input-visitor.c | 57 +++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 5 deletions(-)
> 

> @@ -50,7 +50,11 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
>  
>      do {
>          errno = 0;
> -        start = strtoll(str, &endptr, 0);
> +        if (u64) {
> +            start = strtoull(str, &endptr, 0);

accepts the range [-ULLONG_MAX, ULLONG_MAX] (with 2s complement
wraparound). Do you really want -1 being a synonym for ULLONG_MAX, or do
you want to explicitly reject leading '-' when parsing unsigned
(arguments can be made for both behaviors; in fact, libvirt has two
separate wrappers for parsing uint64_t depending on which behavior is
wanted)

> +        } else {
> +            start = strtoll(str, &endptr, 0);

accepts the range [LLONG_MIN, LLONG_MAX] (that is, roughly half the
range of the unsigned version)

> +        }
>          if (errno == 0 && endptr > str) {
>              if (*endptr == '\0') {
>                  cur = g_malloc0(sizeof(*cur));
> @@ -60,7 +64,7 @@ static void parse_str(StringInputVisitor *siv, Error **errp)
>                                                            range_compare);
>                  cur = NULL;
>                  str = NULL;
> -            } else if (*endptr == '-') {
> +            } else if (*endptr == '-' && !u64) {

Why do you not want to handle ranges when using unsigned numbers?


>  
> +static void parse_type_uint64(Visitor *v, uint64_t *obj, const char *name,
> +                              Error **errp)
> +{
> +    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
> +
> +    if (!siv->string) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> +                   "integer");
> +        return;
> +    }
...

That's a lot of copy-and-paste. Can't you make parse_type_int64() and
parse_type_uint64() both call into a single helper method, that contains
the guts of the existing parse_type_int64() and adds a single parameter
for the one place where the two functions differ on their call to
parse_str()?

-- 
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: 604 bytes --]