| Message ID | 20170509173559.31598-9-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On the subject: there is no such thing as "QUInt". I guess you mean
"uint type" (like in PATCH 06's subject). Could also say "QNUM_U64".
Apropos subject: humor me, and start your subjects with a capital
letter, like this:
qapi: Update the qobject visitor ...
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Switch to use QNum/uint where appropriate to remove i64 limitation.
>
> The input visitor will cast i64 input to u64 for compatibility
> reasons (existing json QMP client already use negative i64 for large
> u64, and expect an implicit cast in qemu).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qapi/qobject-input-visitor.c | 13 +++++++++++--
> qapi/qobject-output-visitor.c | 3 +--
> tests/test-qobject-output-visitor.c | 21 ++++++++++++++++-----
> 3 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 785949ebab..72cefcf677 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -420,9 +420,9 @@ static void qobject_input_type_int64_keyval(Visitor *v, const char *name,
> static void qobject_input_type_uint64(Visitor *v, const char *name,
> uint64_t *obj, Error **errp)
> {
> - /* FIXME: qobject_to_qnum mishandles values over INT64_MAX */
> QObjectInputVisitor *qiv = to_qiv(v);
> QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
> + Error *err = NULL;
> QNum *qnum;
>
> if (!qobj) {
> @@ -435,7 +435,16 @@ static void qobject_input_type_uint64(Visitor *v, const char *name,
> return;
> }
>
> - *obj = qnum_get_int(qnum, errp);
> + /* XXX: compatibility case, accept negative values as u64 */
What does "XXX" signify?
> + *obj = qnum_get_int(qnum, &err);
> +
Shouldn't the comment go right here?
> + if (err) {
> + error_free(err);
> + err = NULL;
> + *obj = qnum_get_uint(qnum, &err);
> + }
> +
> + error_propagate(errp, err);
> }
>
> static void qobject_input_type_uint64_keyval(Visitor *v, const char *name,
> diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c
> index 2ca5093b22..70be84ccb5 100644
> --- a/qapi/qobject-output-visitor.c
> +++ b/qapi/qobject-output-visitor.c
> @@ -150,9 +150,8 @@ static void qobject_output_type_int64(Visitor *v, const char *name,
> static void qobject_output_type_uint64(Visitor *v, const char *name,
> uint64_t *obj, Error **errp)
> {
> - /* FIXME values larger than INT64_MAX become negative */
> QObjectOutputVisitor *qov = to_qov(v);
> - qobject_output_add(qov, name, qnum_from_int(*obj));
> + qobject_output_add(qov, name, qnum_from_uint(*obj));
Before the patch, uint64_t values above INT64_MAX are sent as negative
values, e.g. UINT64_MAX is sent as -1.
After the patch, they are sent unmodified. Clearly a bug fix, but we
have to consider compatibility issues anyway. Does libvirt expect large
integers to be sent as negative integers? Does it cope with this fix
gracefully? Eric, any idea?
> }
>
> static void qobject_output_type_bool(Visitor *v, const char *name, bool *obj,
> diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
> index 66a682d5a8..767818e393 100644
> --- a/tests/test-qobject-output-visitor.c
> +++ b/tests/test-qobject-output-visitor.c
> @@ -595,15 +595,26 @@ static void check_native_list(QObject *qobj,
> qlist = qlist_copy(qobject_to_qlist(qdict_get(qdict, "data")));
>
> switch (kind) {
> - case USER_DEF_NATIVE_LIST_UNION_KIND_S8:
> - case USER_DEF_NATIVE_LIST_UNION_KIND_S16:
> - case USER_DEF_NATIVE_LIST_UNION_KIND_S32:
> - case USER_DEF_NATIVE_LIST_UNION_KIND_S64:
> case USER_DEF_NATIVE_LIST_UNION_KIND_U8:
> case USER_DEF_NATIVE_LIST_UNION_KIND_U16:
> case USER_DEF_NATIVE_LIST_UNION_KIND_U32:
> case USER_DEF_NATIVE_LIST_UNION_KIND_U64:
> - /* all integer elements in JSON arrays get stored into QNums when
> + for (i = 0; i < 32; i++) {
> + QObject *tmp;
> + QNum *qvalue;
> + tmp = qlist_peek(qlist);
> + g_assert(tmp);
> + qvalue = qobject_to_qnum(tmp);
> + g_assert_cmpuint(qnum_get_uint(qvalue, &error_abort), ==, i);
> + qobject_decref(qlist_pop(qlist));
> + }
> + break;
> +
> + case USER_DEF_NATIVE_LIST_UNION_KIND_S8:
> + case USER_DEF_NATIVE_LIST_UNION_KIND_S16:
> + case USER_DEF_NATIVE_LIST_UNION_KIND_S32:
> + case USER_DEF_NATIVE_LIST_UNION_KIND_S64:
> + /* all integer elements in JSON arrays get stored into QInts when
> * we convert to QObjects, so we can check them all in the same
> * fashion, so simply fall through here
> */
Make that "All signed integer ...", and wing both ends of the comment.
Or simply drop the comment.
Markus Armbruster <armbru@redhat.com> writes: > On the subject: there is no such thing as "QUInt". I guess you mean > "uint type" (like in PATCH 06's subject). Could also say "QNUM_U64". > > Apropos subject: humor me, and start your subjects with a capital > letter, like this: > > qapi: Update the qobject visitor ... > > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > >> Switch to use QNum/uint where appropriate to remove i64 limitation. >> >> The input visitor will cast i64 input to u64 for compatibility >> reasons (existing json QMP client already use negative i64 for large >> u64, and expect an implicit cast in qemu). >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> qapi/qobject-input-visitor.c | 13 +++++++++++-- >> qapi/qobject-output-visitor.c | 3 +-- >> tests/test-qobject-output-visitor.c | 21 ++++++++++++++++----- >> 3 files changed, 28 insertions(+), 9 deletions(-) >> >> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c >> index 785949ebab..72cefcf677 100644 >> --- a/qapi/qobject-input-visitor.c >> +++ b/qapi/qobject-input-visitor.c >> @@ -420,9 +420,9 @@ static void qobject_input_type_int64_keyval(Visitor *v, const char *name, >> static void qobject_input_type_uint64(Visitor *v, const char *name, >> uint64_t *obj, Error **errp) >> { >> - /* FIXME: qobject_to_qnum mishandles values over INT64_MAX */ >> QObjectInputVisitor *qiv = to_qiv(v); >> QObject *qobj = qobject_input_get_object(qiv, name, true, errp); >> + Error *err = NULL; >> QNum *qnum; >> >> if (!qobj) { >> @@ -435,7 +435,16 @@ static void qobject_input_type_uint64(Visitor *v, const char *name, >> return; >> } >> >> - *obj = qnum_get_int(qnum, errp); >> + /* XXX: compatibility case, accept negative values as u64 */ > > What does "XXX" signify? > >> + *obj = qnum_get_int(qnum, &err); >> + > > Shouldn't the comment go right here? Nope, I misread the code. We first try qnum_get_int(). Works for integers between -2^63 and 2^63-1. The assignment to *obj adds 2^64 to negative ones. When it doesn't work, we try qnum_get_uint(). Works for integers between 0 and 2^64-1, but only integers between 2^63 and 2^64-1 can occur. >> + if (err) { >> + error_free(err); >> + err = NULL; >> + *obj = qnum_get_uint(qnum, &err); >> + } >> + >> + error_propagate(errp, err); >> } Let's do this the other way round: *obj = qnum_get_uint(qnum, &err); if (err) { /* Need to accept negative values for backward compatibility */ error_free(err); err = NULL; *obj = qnum_get_int(qnum, &err); } error_propagate(errp, err); This way, the backward compatibility code is entirely contained in the conditional. It's also how I misread the code :) [...]
Hi On Tue, May 16, 2017 at 9:33 PM Markus Armbruster <armbru@redhat.com> wrote: > On the subject: there is no such thing as "QUInt". I guess you mean > "uint type" (like in PATCH 06's subject). Could also say "QNUM_U64". > > Apropos subject: humor me, and start your subjects with a capital > letter, like this: > > qapi: Update the qobject visitor ... > > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > > > Switch to use QNum/uint where appropriate to remove i64 limitation. > > > > The input visitor will cast i64 input to u64 for compatibility > > reasons (existing json QMP client already use negative i64 for large > > u64, and expect an implicit cast in qemu). > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > qapi/qobject-input-visitor.c | 13 +++++++++++-- > > qapi/qobject-output-visitor.c | 3 +-- > > tests/test-qobject-output-visitor.c | 21 ++++++++++++++++----- > > 3 files changed, 28 insertions(+), 9 deletions(-) > > > > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c > > index 785949ebab..72cefcf677 100644 > > --- a/qapi/qobject-input-visitor.c > > +++ b/qapi/qobject-input-visitor.c > > @@ -420,9 +420,9 @@ static void qobject_input_type_int64_keyval(Visitor > *v, const char *name, > > static void qobject_input_type_uint64(Visitor *v, const char *name, > > uint64_t *obj, Error **errp) > > { > > - /* FIXME: qobject_to_qnum mishandles values over INT64_MAX */ > > QObjectInputVisitor *qiv = to_qiv(v); > > QObject *qobj = qobject_input_get_object(qiv, name, true, errp); > > + Error *err = NULL; > > QNum *qnum; > > > > if (!qobj) { > > @@ -435,7 +435,16 @@ static void qobject_input_type_uint64(Visitor *v, > const char *name, > > return; > > } > > > > - *obj = qnum_get_int(qnum, errp); > > + /* XXX: compatibility case, accept negative values as u64 */ > > What does "XXX" signify? > It's a fairly common marker for something similar to FIXME (there are hundreds of them in qemu source tree). I'd like to leave a fixme that means that there should be a visitor flag/capability to fix this compatibility behaviour. (this could be exposed as a qmp capability) > > > + *obj = qnum_get_int(qnum, &err); > > + > > Shouldn't the comment go right here? > > Above qnum_get_int() is the right place imho. > > + if (err) { > > + error_free(err); > > + err = NULL; > > + *obj = qnum_get_uint(qnum, &err); > > + } > > + > > + error_propagate(errp, err); > > } > > > > static void qobject_input_type_uint64_keyval(Visitor *v, const char > *name, > > diff --git a/qapi/qobject-output-visitor.c > b/qapi/qobject-output-visitor.c > > index 2ca5093b22..70be84ccb5 100644 > > --- a/qapi/qobject-output-visitor.c > > +++ b/qapi/qobject-output-visitor.c > > @@ -150,9 +150,8 @@ static void qobject_output_type_int64(Visitor *v, > const char *name, > > static void qobject_output_type_uint64(Visitor *v, const char *name, > > uint64_t *obj, Error **errp) > > { > > - /* FIXME values larger than INT64_MAX become negative */ > > QObjectOutputVisitor *qov = to_qov(v); > > - qobject_output_add(qov, name, qnum_from_int(*obj)); > > + qobject_output_add(qov, name, qnum_from_uint(*obj)); > > Before the patch, uint64_t values above INT64_MAX are sent as negative > values, e.g. UINT64_MAX is sent as -1. > > After the patch, they are sent unmodified. Clearly a bug fix, but we > have to consider compatibility issues anyway. Does libvirt expect large > integers to be sent as negative integers? Does it cope with this fix > gracefully? Eric, any idea? > The libvirt json parser seems to rely on virStrToLong_ui(), which is a wrapper around strtoul(), so it accepts negative values with -2^63..-1. Changing it to return values larger than INT64_MAX should be ok. > > > } > > > > static void qobject_output_type_bool(Visitor *v, const char *name, bool > *obj, > > diff --git a/tests/test-qobject-output-visitor.c > b/tests/test-qobject-output-visitor.c > > index 66a682d5a8..767818e393 100644 > > --- a/tests/test-qobject-output-visitor.c > > +++ b/tests/test-qobject-output-visitor.c > > @@ -595,15 +595,26 @@ static void check_native_list(QObject *qobj, > > qlist = qlist_copy(qobject_to_qlist(qdict_get(qdict, "data"))); > > > > switch (kind) { > > - case USER_DEF_NATIVE_LIST_UNION_KIND_S8: > > - case USER_DEF_NATIVE_LIST_UNION_KIND_S16: > > - case USER_DEF_NATIVE_LIST_UNION_KIND_S32: > > - case USER_DEF_NATIVE_LIST_UNION_KIND_S64: > > case USER_DEF_NATIVE_LIST_UNION_KIND_U8: > > case USER_DEF_NATIVE_LIST_UNION_KIND_U16: > > case USER_DEF_NATIVE_LIST_UNION_KIND_U32: > > case USER_DEF_NATIVE_LIST_UNION_KIND_U64: > > - /* all integer elements in JSON arrays get stored into QNums > when > > + for (i = 0; i < 32; i++) { > > + QObject *tmp; > > + QNum *qvalue; > > + tmp = qlist_peek(qlist); > > + g_assert(tmp); > > + qvalue = qobject_to_qnum(tmp); > > + g_assert_cmpuint(qnum_get_uint(qvalue, &error_abort), ==, > i); > > + qobject_decref(qlist_pop(qlist)); > > + } > > + break; > > + > > + case USER_DEF_NATIVE_LIST_UNION_KIND_S8: > > + case USER_DEF_NATIVE_LIST_UNION_KIND_S16: > > + case USER_DEF_NATIVE_LIST_UNION_KIND_S32: > > + case USER_DEF_NATIVE_LIST_UNION_KIND_S64: > > + /* all integer elements in JSON arrays get stored into QInts > when > > * we convert to QObjects, so we can check them all in the same > > * fashion, so simply fall through here > > */ > > Make that "All signed integer ...", and wing both ends of the comment. > ok > Or simply drop the comment.
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Tue, May 16, 2017 at 9:33 PM Markus Armbruster <armbru@redhat.com> wrote: > >> On the subject: there is no such thing as "QUInt". I guess you mean >> "uint type" (like in PATCH 06's subject). Could also say "QNUM_U64". >> >> Apropos subject: humor me, and start your subjects with a capital >> letter, like this: >> >> qapi: Update the qobject visitor ... >> >> Marc-André Lureau <marcandre.lureau@redhat.com> writes: >> >> > Switch to use QNum/uint where appropriate to remove i64 limitation. >> > >> > The input visitor will cast i64 input to u64 for compatibility >> > reasons (existing json QMP client already use negative i64 for large >> > u64, and expect an implicit cast in qemu). >> > >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> > --- >> > qapi/qobject-input-visitor.c | 13 +++++++++++-- >> > qapi/qobject-output-visitor.c | 3 +-- >> > tests/test-qobject-output-visitor.c | 21 ++++++++++++++++----- >> > 3 files changed, 28 insertions(+), 9 deletions(-) >> > >> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c >> > index 785949ebab..72cefcf677 100644 >> > --- a/qapi/qobject-input-visitor.c >> > +++ b/qapi/qobject-input-visitor.c >> > @@ -420,9 +420,9 @@ static void qobject_input_type_int64_keyval(Visitor *v, const char *name, >> > static void qobject_input_type_uint64(Visitor *v, const char *name, >> > uint64_t *obj, Error **errp) >> > { >> > - /* FIXME: qobject_to_qnum mishandles values over INT64_MAX */ >> > QObjectInputVisitor *qiv = to_qiv(v); >> > QObject *qobj = qobject_input_get_object(qiv, name, true, errp); >> > + Error *err = NULL; >> > QNum *qnum; >> > >> > if (!qobj) { >> > @@ -435,7 +435,16 @@ static void qobject_input_type_uint64(Visitor *v, const char *name, >> > return; >> > } >> > >> > - *obj = qnum_get_int(qnum, errp); >> > + /* XXX: compatibility case, accept negative values as u64 */ >> >> What does "XXX" signify? >> > > It's a fairly common marker for something similar to FIXME (there are > hundreds of them in qemu source tree). > > I'd like to leave a fixme that means that there should be a visitor > flag/capability to fix this compatibility behaviour. (this could be exposed > as a qmp capability) What I don't like about XXX: it doesn't say whether it merely marks something we intend to do or something that's actually broken. The text following it should clarify, but let's stick to FIXME for the latter case, and TODO for the former. A FIXME comment must explain what's broken. It may explain how to fix it. A TODO comment should explain what we intend to do (d'oh). Your XXX comment does neither, and that's also why I asked :) >> >> > + *obj = qnum_get_int(qnum, &err); >> > + >> >> Shouldn't the comment go right here? >> >> > Above qnum_get_int() is the right place imho. I may have a stronger opinion once I see the revised comment text. >> > + if (err) { >> > + error_free(err); >> > + err = NULL; >> > + *obj = qnum_get_uint(qnum, &err); >> > + } >> > + >> > + error_propagate(errp, err); >> > } >> > >> > static void qobject_input_type_uint64_keyval(Visitor *v, const char *name, >> > diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c >> > index 2ca5093b22..70be84ccb5 100644 >> > --- a/qapi/qobject-output-visitor.c >> > +++ b/qapi/qobject-output-visitor.c >> > @@ -150,9 +150,8 @@ static void qobject_output_type_int64(Visitor *v, const char *name, >> > static void qobject_output_type_uint64(Visitor *v, const char *name, >> > uint64_t *obj, Error **errp) >> > { >> > - /* FIXME values larger than INT64_MAX become negative */ >> > QObjectOutputVisitor *qov = to_qov(v); >> > - qobject_output_add(qov, name, qnum_from_int(*obj)); >> > + qobject_output_add(qov, name, qnum_from_uint(*obj)); >> >> Before the patch, uint64_t values above INT64_MAX are sent as negative >> values, e.g. UINT64_MAX is sent as -1. >> >> After the patch, they are sent unmodified. Clearly a bug fix, but we >> have to consider compatibility issues anyway. Does libvirt expect large >> integers to be sent as negative integers? Does it cope with this fix >> gracefully? Eric, any idea? >> > > The libvirt json parser seems to rely on virStrToLong_ui(), which is a > wrapper around strtoul(), so it accepts negative values with -2^63..-1. > Changing it to return values larger than INT64_MAX should be ok. Thanks for checking. [...]
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index 785949ebab..72cefcf677 100644 --- a/qapi/qobject-input-visitor.c +++ b/qapi/qobject-input-visitor.c @@ -420,9 +420,9 @@ static void qobject_input_type_int64_keyval(Visitor *v, const char *name, static void qobject_input_type_uint64(Visitor *v, const char *name, uint64_t *obj, Error **errp) { - /* FIXME: qobject_to_qnum mishandles values over INT64_MAX */ QObjectInputVisitor *qiv = to_qiv(v); QObject *qobj = qobject_input_get_object(qiv, name, true, errp); + Error *err = NULL; QNum *qnum; if (!qobj) { @@ -435,7 +435,16 @@ static void qobject_input_type_uint64(Visitor *v, const char *name, return; } - *obj = qnum_get_int(qnum, errp); + /* XXX: compatibility case, accept negative values as u64 */ + *obj = qnum_get_int(qnum, &err); + + if (err) { + error_free(err); + err = NULL; + *obj = qnum_get_uint(qnum, &err); + } + + error_propagate(errp, err); } static void qobject_input_type_uint64_keyval(Visitor *v, const char *name, diff --git a/qapi/qobject-output-visitor.c b/qapi/qobject-output-visitor.c index 2ca5093b22..70be84ccb5 100644 --- a/qapi/qobject-output-visitor.c +++ b/qapi/qobject-output-visitor.c @@ -150,9 +150,8 @@ static void qobject_output_type_int64(Visitor *v, const char *name, static void qobject_output_type_uint64(Visitor *v, const char *name, uint64_t *obj, Error **errp) { - /* FIXME values larger than INT64_MAX become negative */ QObjectOutputVisitor *qov = to_qov(v); - qobject_output_add(qov, name, qnum_from_int(*obj)); + qobject_output_add(qov, name, qnum_from_uint(*obj)); } static void qobject_output_type_bool(Visitor *v, const char *name, bool *obj, diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c index 66a682d5a8..767818e393 100644 --- a/tests/test-qobject-output-visitor.c +++ b/tests/test-qobject-output-visitor.c @@ -595,15 +595,26 @@ static void check_native_list(QObject *qobj, qlist = qlist_copy(qobject_to_qlist(qdict_get(qdict, "data"))); switch (kind) { - case USER_DEF_NATIVE_LIST_UNION_KIND_S8: - case USER_DEF_NATIVE_LIST_UNION_KIND_S16: - case USER_DEF_NATIVE_LIST_UNION_KIND_S32: - case USER_DEF_NATIVE_LIST_UNION_KIND_S64: case USER_DEF_NATIVE_LIST_UNION_KIND_U8: case USER_DEF_NATIVE_LIST_UNION_KIND_U16: case USER_DEF_NATIVE_LIST_UNION_KIND_U32: case USER_DEF_NATIVE_LIST_UNION_KIND_U64: - /* all integer elements in JSON arrays get stored into QNums when + for (i = 0; i < 32; i++) { + QObject *tmp; + QNum *qvalue; + tmp = qlist_peek(qlist); + g_assert(tmp); + qvalue = qobject_to_qnum(tmp); + g_assert_cmpuint(qnum_get_uint(qvalue, &error_abort), ==, i); + qobject_decref(qlist_pop(qlist)); + } + break; + + case USER_DEF_NATIVE_LIST_UNION_KIND_S8: + case USER_DEF_NATIVE_LIST_UNION_KIND_S16: + case USER_DEF_NATIVE_LIST_UNION_KIND_S32: + case USER_DEF_NATIVE_LIST_UNION_KIND_S64: + /* all integer elements in JSON arrays get stored into QInts when * we convert to QObjects, so we can check them all in the same * fashion, so simply fall through here */
Switch to use QNum/uint where appropriate to remove i64 limitation. The input visitor will cast i64 input to u64 for compatibility reasons (existing json QMP client already use negative i64 for large u64, and expect an implicit cast in qemu). Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- qapi/qobject-input-visitor.c | 13 +++++++++++-- qapi/qobject-output-visitor.c | 3 +-- tests/test-qobject-output-visitor.c | 21 ++++++++++++++++----- 3 files changed, 28 insertions(+), 9 deletions(-)