| Message ID | 20170531135709.345-15-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> > --- > hw/i386/acpi-build.c | 3 +-- > qapi/qobject-input-visitor.c | 21 ++++++++++++++++----- > qapi/qobject-output-visitor.c | 3 +-- > tests/test-qobject-input-visitor.c | 7 ++----- > tests/test-qobject-output-visitor.c | 28 +++++++++++++++++++++------- > 5 files changed, 41 insertions(+), 21 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 1709efdf1c..ba2be1e9da 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -2634,10 +2634,9 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg) > if (!o) { > return false; > } > - if (!qnum_get_int(qobject_to_qnum(o), &val)) { > + if (!qnum_get_uint(qobject_to_qnum(o), &mcfg->mcfg_base)) { > g_assert_not_reached(); > } > - mcfg->mcfg_base = val; > qobject_decref(o); > > o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL); > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c > index 74835ba339..7f9d6f57a1 100644 > --- a/qapi/qobject-input-visitor.c > +++ b/qapi/qobject-input-visitor.c > @@ -417,7 +417,6 @@ 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); > QNum *qnum; > @@ -427,11 +426,23 @@ static void qobject_input_type_uint64(Visitor *v, const char *name, > return; > } > qnum = qobject_to_qnum(qobj); > - if (!qnum || !qnum_get_int(qnum, &val)) { > - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, > - full_name(qiv, name), "integer"); > + if (!qnum) { > + goto err; > + } > + > + if (qnum_get_uint(qnum, obj)) { > + return; > } > - *obj = val; > + > + /* Need to accept negative values for backward compatibility */ > + if (qnum_get_int(qnum, &val)) { > + *obj = val; > + return; > + } > + > +err: > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, > + full_name(qiv, name), "uint64"); > } > > 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. You wrote that libvirt should cope fine, because its parsing of unsigned integers accepts negative values modulo 2^64. There's hope that other clients will, too. There's one thing left to do: please document the incompatible bug fix in the commit message. > static void qobject_output_type_bool(Visitor *v, const char *name, bool *obj, > diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c > index 983c59c474..6f94fc677c 100644 > --- a/tests/test-qobject-input-visitor.c > +++ b/tests/test-qobject-input-visitor.c > @@ -122,7 +122,6 @@ static void test_visitor_in_int(TestInputVisitorData *data, > static void test_visitor_in_uint(TestInputVisitorData *data, > const void *unused) > { > - Error *err = NULL; > uint64_t res = 0; > int64_t i64; > double dbl; > @@ -146,12 +145,10 @@ static void test_visitor_in_uint(TestInputVisitorData *data, > visit_type_uint64(v, NULL, &res, &error_abort); > g_assert_cmpuint(res, ==, (uint64_t)-value); > > - /* BUG: value between INT64_MAX+1 and UINT64_MAX rejected */ > - > v = visitor_input_test_init(data, "18446744073709551574"); > > - visit_type_uint64(v, NULL, &res, &err); > - error_free_or_abort(&err); > + visit_type_uint64(v, NULL, &res, &error_abort); > + g_assert_cmpuint(res, ==, 18446744073709551574U); > > visit_type_number(v, NULL, &dbl, &error_abort); > } > diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c > index 3180d8cbde..d9f106d52e 100644 > --- a/tests/test-qobject-output-visitor.c > +++ b/tests/test-qobject-output-visitor.c > @@ -602,17 +602,31 @@ 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 > - * we convert to QObjects, so we can check them all in the same > - * fashion, so simply fall through here > + for (i = 0; i < 32; i++) { > + QObject *tmp; > + QNum *qvalue; > + uint64_t val; > + > + tmp = qlist_peek(qlist); > + g_assert(tmp); > + qvalue = qobject_to_qnum(tmp); > + g_assert(qnum_get_uint(qvalue, &val)); > + g_assert_cmpuint(val, ==, 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 signed 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. > */ > case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: > for (i = 0; i < 32; i++) { With the incompatible fix explained in the commit message: Reviewed-by: Markus Armbruster <armbru@redhat.com>
One more nitpick: 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> [...] > diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c > index 3180d8cbde..d9f106d52e 100644 > --- a/tests/test-qobject-output-visitor.c > +++ b/tests/test-qobject-output-visitor.c > @@ -602,17 +602,31 @@ 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 > - * we convert to QObjects, so we can check them all in the same > - * fashion, so simply fall through here > + for (i = 0; i < 32; i++) { > + QObject *tmp; > + QNum *qvalue; > + uint64_t val; > + > + tmp = qlist_peek(qlist); > + g_assert(tmp); > + qvalue = qobject_to_qnum(tmp); > + g_assert(qnum_get_uint(qvalue, &val)); > + g_assert_cmpuint(val, ==, 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 signed 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. > */ > case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: > for (i = 0; i < 32; i++) { Wing both ends of the comment, please.
----- Original Message ----- > One more nitpick: > > 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> > [...] > > diff --git a/tests/test-qobject-output-visitor.c > > b/tests/test-qobject-output-visitor.c > > index 3180d8cbde..d9f106d52e 100644 > > --- a/tests/test-qobject-output-visitor.c > > +++ b/tests/test-qobject-output-visitor.c > > @@ -602,17 +602,31 @@ 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 > > - * we convert to QObjects, so we can check them all in the same > > - * fashion, so simply fall through here > > + for (i = 0; i < 32; i++) { > > + QObject *tmp; > > + QNum *qvalue; > > + uint64_t val; > > + > > + tmp = qlist_peek(qlist); > > + g_assert(tmp); > > + qvalue = qobject_to_qnum(tmp); > > + g_assert(qnum_get_uint(qvalue, &val)); > > + g_assert_cmpuint(val, ==, 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 signed 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. > > */ > > case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: > > for (i = 0; i < 32; i++) { > > Wing both ends of the comment, please. You want this: ? /* All signed integer elements in JSON arrays get stored into */ /* QInts when we convert to QObjects, so we can check them all */ ... I think this is quite annoying for refilling with emacs Perhaps rather /* All signed integer elements in JSON arrays get stored into * QInts when we convert to QObjects, so we can check them all */ quite uglier to me, but I don't care However, I'd prefer if either we have a common rule in qemu or we don't bike-sched over that... (even better would be to have this somehow automated with tools like git-clang-format)
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > ----- Original Message ----- >> One more nitpick: >> >> 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> >> [...] >> > diff --git a/tests/test-qobject-output-visitor.c >> > b/tests/test-qobject-output-visitor.c >> > index 3180d8cbde..d9f106d52e 100644 >> > --- a/tests/test-qobject-output-visitor.c >> > +++ b/tests/test-qobject-output-visitor.c >> > @@ -602,17 +602,31 @@ 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 >> > - * we convert to QObjects, so we can check them all in the same >> > - * fashion, so simply fall through here >> > + for (i = 0; i < 32; i++) { >> > + QObject *tmp; >> > + QNum *qvalue; >> > + uint64_t val; >> > + >> > + tmp = qlist_peek(qlist); >> > + g_assert(tmp); >> > + qvalue = qobject_to_qnum(tmp); >> > + g_assert(qnum_get_uint(qvalue, &val)); >> > + g_assert_cmpuint(val, ==, 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 signed 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. >> > */ >> > case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: >> > for (i = 0; i < 32; i++) { >> >> Wing both ends of the comment, please. > > You want this: ? > > /* All signed integer elements in JSON arrays get stored into */ > /* QInts when we convert to QObjects, so we can check them all */ > ... Ewww! > I think this is quite annoying for refilling with emacs Yup. > Perhaps rather > > /* All signed integer elements in JSON arrays get stored into > * QInts when we convert to QObjects, so we can check them all */ > > quite uglier to me, but I don't care I'm asking for this: /* * All signed 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. */ This comment style is commonly called "winged". It's fairly widespread in QEMU. > However, I'd prefer if either we have a common rule in qemu or we don't bike-sched over that... > > (even better would be to have this somehow automated with tools like git-clang-format) Tool support would be easier if we didn't insist on inventing our very own coding style, then fail to enforce it uniformly. Oh well, we'll muddle on.
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 1709efdf1c..ba2be1e9da 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2634,10 +2634,9 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg) if (!o) { return false; } - if (!qnum_get_int(qobject_to_qnum(o), &val)) { + if (!qnum_get_uint(qobject_to_qnum(o), &mcfg->mcfg_base)) { g_assert_not_reached(); } - mcfg->mcfg_base = val; qobject_decref(o); o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL); diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c index 74835ba339..7f9d6f57a1 100644 --- a/qapi/qobject-input-visitor.c +++ b/qapi/qobject-input-visitor.c @@ -417,7 +417,6 @@ 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); QNum *qnum; @@ -427,11 +426,23 @@ static void qobject_input_type_uint64(Visitor *v, const char *name, return; } qnum = qobject_to_qnum(qobj); - if (!qnum || !qnum_get_int(qnum, &val)) { - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, - full_name(qiv, name), "integer"); + if (!qnum) { + goto err; + } + + if (qnum_get_uint(qnum, obj)) { + return; } - *obj = val; + + /* Need to accept negative values for backward compatibility */ + if (qnum_get_int(qnum, &val)) { + *obj = val; + return; + } + +err: + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, + full_name(qiv, name), "uint64"); } 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-input-visitor.c b/tests/test-qobject-input-visitor.c index 983c59c474..6f94fc677c 100644 --- a/tests/test-qobject-input-visitor.c +++ b/tests/test-qobject-input-visitor.c @@ -122,7 +122,6 @@ static void test_visitor_in_int(TestInputVisitorData *data, static void test_visitor_in_uint(TestInputVisitorData *data, const void *unused) { - Error *err = NULL; uint64_t res = 0; int64_t i64; double dbl; @@ -146,12 +145,10 @@ static void test_visitor_in_uint(TestInputVisitorData *data, visit_type_uint64(v, NULL, &res, &error_abort); g_assert_cmpuint(res, ==, (uint64_t)-value); - /* BUG: value between INT64_MAX+1 and UINT64_MAX rejected */ - v = visitor_input_test_init(data, "18446744073709551574"); - visit_type_uint64(v, NULL, &res, &err); - error_free_or_abort(&err); + visit_type_uint64(v, NULL, &res, &error_abort); + g_assert_cmpuint(res, ==, 18446744073709551574U); visit_type_number(v, NULL, &dbl, &error_abort); } diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c index 3180d8cbde..d9f106d52e 100644 --- a/tests/test-qobject-output-visitor.c +++ b/tests/test-qobject-output-visitor.c @@ -602,17 +602,31 @@ 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 - * we convert to QObjects, so we can check them all in the same - * fashion, so simply fall through here + for (i = 0; i < 32; i++) { + QObject *tmp; + QNum *qvalue; + uint64_t val; + + tmp = qlist_peek(qlist); + g_assert(tmp); + qvalue = qobject_to_qnum(tmp); + g_assert(qnum_get_uint(qvalue, &val)); + g_assert_cmpuint(val, ==, 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 signed 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. */ case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: for (i = 0; i < 32; i++) {
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> --- hw/i386/acpi-build.c | 3 +-- qapi/qobject-input-visitor.c | 21 ++++++++++++++++----- qapi/qobject-output-visitor.c | 3 +-- tests/test-qobject-input-visitor.c | 7 ++----- tests/test-qobject-output-visitor.c | 28 +++++++++++++++++++++------- 5 files changed, 41 insertions(+), 21 deletions(-)