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

[David Gibson]

On Wed, Oct 31, 2018 at 06:18:33PM +0100, David Hildenbrand wrote:
> On 31.10.18 15:32, Markus Armbruster wrote:
> > 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.
> 
> "Parse uin64_t separately. We don't have to worry about ranges as far as
> I can see. Ranges are parsed and processed via start_list()/next_list()
> and friends. parse_type_int64() only has to deal with ranges as it
> reuses the function parse_str(). E.g. parse_type_size() also does not
> have to handle ranges. (I assume that we could easily reimplement
> parse_type_int64() in a similar fashion, too).
> 
> The only thing that will change is that uint64_t properties that didn't
> expect a range will now actually bail out if a range is supplied."
> 
> I'll do some more testing.
> 
> > 
> >>
> >> 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.
> 
> Yes, I will add that!
> 
> > 
> >> 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.
> 
> I will also add a comment to the description that negative numbers will
> continue to work.
> 
> > 
> >> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : 
> >> "null",
> >> +                   "an uint64 value");
> > 
> > I think this should be "a uint64 value".
> 
> As I am not a native speaker, I will stick to your suggestion unless
> somebody else speaks up.

I am a native speaker and "a uint64 value" sounds better to me.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature