| Message ID | 20181023152306.3123-3-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi/range/memory-device: fixes and cleanups | expand |
On Tue, Oct 23, 2018 at 05:23:01PM +0200, David Hildenbrand wrote: > 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. > > 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 > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > 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)) { > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null", > + "an uint64 value"); > + return; > } > + > + *obj = val; > } It's not obvious to me why this looks so different from the code in parse_type_int64(). Should we be using qemu_strtoi64() in the pre-existing function, instead of what's there now? > > static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
> > It's not obvious to me why this looks so different from the code in > parse_type_int64(). Should we be using qemu_strtoi64() in the > pre-existing function, instead of what's there now? The existing function has to be that complicated because it calls into the same function used to parse ranges. We don't need ranges (or create/modify) any, so this is not necessary. This function is similar to the other parse functions (not parsing ranges), e.g. parse_type_bool(). Thanks! > >> >> static void parse_type_size(Visitor *v, const char *name, uint64_t *obj, >
David Hildenbrand <david@redhat.com> 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 <david@redhat.com> > --- > 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.
Markus Armbruster <armbru@redhat.com> writes: > David Hildenbrand <david@redhat.com> 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. One more thing: the qobject-input-visitor change also updated the corresponding output visitor. Shouldn't we do the same here?
On 31.10.18 15:32, Markus Armbruster wrote: > David Hildenbrand <david@redhat.com> 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 <david@redhat.com> >> --- >> 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. > >> + return; >> } >> + >> + *obj = val; >> } >> >> static void parse_type_size(Visitor *v, const char *name, uint64_t *obj, > > Patch looks good to me otherwise. > Thanks!
On 31.10.18 15:44, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > >> David Hildenbrand <david@redhat.com> 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. > > One more thing: the qobject-input-visitor change also updated the > corresponding output visitor. Shouldn't we do the same here? > I'll have a look if something has to be done on that side.
On Wed, Oct 31, 2018 at 06:18:33PM +0100, David Hildenbrand wrote: > On 31.10.18 15:32, Markus Armbruster wrote: > > David Hildenbrand <david@redhat.com> 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 <david@redhat.com> > >> --- > >> 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.
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)) { + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null", + "an uint64 value"); + return; } + + *obj = val; } static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
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. 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 Signed-off-by: David Hildenbrand <david@redhat.com> --- qapi/string-input-visitor.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)