Marc-André Lureau <marcandre.lur...@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.lur...@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 <arm...@redhat.com>