Re: [Qemu-devel] [PATCH v3 2/7] qapi: correctly parse uint64_t values from strings

[Markus Armbruster]

David Hildenbrand <address@hidden> writes:

> Right now, we parse uint64_t values just like int64_t values, resulting
> in negative values getting accepted and certain valid large numbers only
> being representable as negative numbers. Also, reported errors indicate
> that an int64_t is expected.
>
> Parse uin64_t separately. We don't have to worry about ranges.

The commit message should mention *why* we don't we have to worry about
ranges.

>
> E.g. we can now also specify
>     -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000
> Instead of only going via negative values
>     -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000
>
> Resulting in the same values
>
> (qemu) info memory-devices
> Memory device [nvdimm]: "nv1"
>   addr: 0xffffffffc0000000
>   slot: 0
>   node: 0
>

Suggest to mention this makes the string-input-visitor catch up with the
qobject-input-visitor, which got changed similarly in commit
5923f85fb82.

> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  qapi/string-input-visitor.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index c1454f999f..f2df027325 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -247,15 +247,16 @@ error:
>  static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
>                                Error **errp)
>  {
> -    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
> -    int64_t i;
> -    Error *err = NULL;
> -    parse_type_int64(v, name, &i, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> -    } else {
> -        *obj = i;
> +    StringInputVisitor *siv = to_siv(v);
> +    uint64_t val;
> +
> +    if (qemu_strtou64(siv->string, NULL, 0, &val)) {

Works because qemu_strtou64() accepts negative numbers and interprets
them modulo 2^64.

> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> +                   "an uint64 value");

I think this should be "a uint64 value".

> +        return;
>      }
> +
> +    *obj = val;
>  }
>  
>  static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,

Patch looks good to me otherwise.