Re: [Qemu-devel] [PATCH v3 11/43] qapi: update the qobject visitor to use QNUM_U64

[Markus Armbruster]

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).
>
> Note: before the patch, uint64_t values above INT64_MAX are sent over
> json QMP 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.  libvirt should cope fine,
> because its parsing of unsigned integers accepts negative values
> modulo 2^64.  There's hope that other clients will, too.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> Reviewed-by: Markus Armbruster <address@hidden>
> ---
>  hw/i386/acpi-build.c                |  2 +-
>  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 | 20 ++++++++++++++++----
>  5 files changed, 36 insertions(+), 17 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d7d2b65fe4..3eb43677f0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2614,7 +2614,7 @@ static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
>      if (!o) {
>          return false;
>      }
> -    mcfg->mcfg_base = qnum_get_int(qobject_to_qnum(o));
> +    mcfg->mcfg_base = qnum_get_uint(qobject_to_qnum(o));
>      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 539b3b825d..35aff78f2b 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_try_int(qnum, &val)) {
> -        error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
> -                   full_name(qiv, name), "integer");
> +    if (!qnum) {
> +        goto err;
> +    }
> +
> +    if (qnum_get_try_uint(qnum, obj)) {
> +        return;
>      }
> -    *obj = val;
> +
> +    /* Need to accept negative values for backward compatibility */
> +    if (qnum_get_try_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 40a135db89..3bcdb9a045 100644
> --- a/tests/test-qobject-input-visitor.c
> +++ b/tests/test-qobject-input-visitor.c
> @@ -123,7 +123,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;
> @@ -147,12 +146,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 a16c8f663f..d23c8eb352 100644
> --- a/tests/test-qobject-output-visitor.c
> +++ b/tests/test-qobject-output-visitor.c
> @@ -602,14 +602,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:
> +        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), ==, i);

You use qnum_get_uint() here, but...

> +            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 QNums
>           * 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++) {
               QObject *tmp;
               QNum *qvalue;
               int64_t val;

               tmp = qlist_peek(qlist);
               g_assert(tmp);
               qvalue = qobject_to_qnum(tmp);
               g_assert(qnum_get_try_int(qvalue, &val));

qnum_get_try_int() here.  Either is fine with me, but let's to it the
same way.

You seem to prefer qnum_get_try_int() elsewhere in tests, probably
because it fails more nicely, at the cost of one extra line.  I don't
mind.

               g_assert_cmpint(val, ==, i);
               qobject_decref(qlist_pop(qlist));
           }
           break;

With that tidied up, my R-by stands.