[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 14/45] qapi: update the qobject visitor to us
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 14/45] qapi: update the qobject visitor to use QNUM_U64 |
Date: |
Fri, 02 Jun 2017 13:18:50 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Marc-André Lureau <address@hidden> 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 <address@hidden>
> ---
> 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 <address@hidden>
- Re: [Qemu-devel] [PATCH v2 14/45] qapi: update the qobject visitor to use QNUM_U64,
Markus Armbruster <=