Re: [Qemu-devel] [PATCH 06/17] qnum: add uint type

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> In order to store integer values superior to INT64_MAX, add a u64
> internal representation.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/qapi/qmp/qnum.h |  4 ++++
>  qobject/qnum.c          | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/check-qnum.c      | 51 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)
>
> diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> index 0e51427821..89c14e040f 100644
> --- a/include/qapi/qmp/qnum.h
> +++ b/include/qapi/qmp/qnum.h
> @@ -17,6 +17,7 @@
>  
>  typedef enum {
>      QNUM_I64,
> +    QNUM_U64,
>      QNUM_DOUBLE
>  } QNumType;
>  
> @@ -25,14 +26,17 @@ typedef struct QNum {
>      QNumType type;
>      union {
>          int64_t i64;
> +        uint64_t u64;
>          double dbl;
>      } u;
>  } QNum;
>  
>  QNum *qnum_from_int(int64_t value);
> +QNum *qnum_from_uint(uint64_t value);
>  QNum *qnum_from_double(double value);
>  
>  int64_t qnum_get_int(const QNum *qi, Error **errp);
> +uint64_t qnum_get_uint(const QNum *qi, Error **errp);
>  double qnum_get_double(QNum *qn);
>  
>  char *qnum_to_string(QNum *qn);
> diff --git a/qobject/qnum.c b/qobject/qnum.c
> index 8e9dd38350..be6307accf 100644
> --- a/qobject/qnum.c
> +++ b/qobject/qnum.c
> @@ -13,6 +13,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"

Superfluous, please drop.

>  #include "qapi/qmp/qnum.h"
>  #include "qapi/qmp/qobject.h"
>  #include "qemu-common.h"
> @@ -34,6 +35,22 @@ QNum *qnum_from_int(int64_t value)
>  }
>  
>  /**
> + * qnum_from_uint(): Create a new QNum from an uint64_t
> + *
> + * Return strong reference.
> + */
> +QNum *qnum_from_uint(uint64_t value)
> +{
> +    QNum *qn = g_new(QNum, 1);
> +
> +    qobject_init(QOBJECT(qn), QTYPE_QNUM);
> +    qn->type = QNUM_U64;
> +    qn->u.u64 = value;
> +
> +    return qn;
> +}
> +
> +/**
>   * qnum_from_double(): Create a new QNum from a double
>   *
>   * Return strong reference.
> @@ -57,6 +74,34 @@ int64_t qnum_get_int(const QNum *qn, Error **errp)
>      switch (qn->type) {
>      case QNUM_I64:
>          return qn->u.i64;
> +    case QNUM_U64:
> +        if (qn->u.u64 > INT64_MAX) {
> +            error_setg(errp, "The number is too large, use qnum_get_uint()");

The user has no way to "use qnum_get_uint()".

> +            return 0;
> +        }
> +        return qn->u.u64;
> +    case QNUM_DOUBLE:
> +        error_setg(errp, "The number is a float");
> +        return 0;
> +    }
> +
> +    g_assert_not_reached();
> +}
> +
> +/**
> + * qnum_get_uint(): Get an unsigned integer from the number
> + */
> +uint64_t qnum_get_uint(const QNum *qn, Error **errp)
> +{
> +    switch (qn->type) {
> +    case QNUM_I64:
> +        if (qn->u.i64 < 0) {
> +            error_setg(errp, "The number is negative");

The new error messages are inadequate in the same way as the existing
one.

> +            return 0;
> +        }
> +        return qn->u.i64;
> +    case QNUM_U64:
> +        return qn->u.u64;
>      case QNUM_DOUBLE:
>          error_setg(errp, "The number is a float");
>          return 0;
> @@ -73,6 +118,8 @@ double qnum_get_double(QNum *qn)
>      switch (qn->type) {
>      case QNUM_I64:
>          return qn->u.i64;
> +    case QNUM_U64:
> +        return qn->u.u64;
>      case QNUM_DOUBLE:
>          return qn->u.dbl;
>      }
> @@ -88,6 +135,8 @@ char *qnum_to_string(QNum *qn)
>      switch (qn->type) {
>      case QNUM_I64:
>          return g_strdup_printf("%" PRId64, qn->u.i64);
> +    case QNUM_U64:
> +        return g_strdup_printf("%" PRIu64, qn->u.u64);
>      case QNUM_DOUBLE:
>          /* FIXME: snprintf() is locale dependent; but JSON requires
>           * numbers to be formatted as if in the C locale. Dependence
> diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> index d08d35e85a..8199546f99 100644
> --- a/tests/check-qnum.c
> +++ b/tests/check-qnum.c
> @@ -36,6 +36,21 @@ static void qnum_from_int_test(void)
>      g_free(qi);
>  }
>  
> +static void qnum_from_uint_test(void)
> +{
> +    QNum *qu;
> +    const int value = UINT_MAX;
> +
> +    qu = qnum_from_int(value);
> +    g_assert(qu != NULL);
> +    g_assert(qu->u.u64 == value);
> +    g_assert(qu->base.refcnt == 1);
> +    g_assert(qobject_type(QOBJECT(qu)) == QTYPE_QNUM);
> +
> +    // destroy doesn't exit yet
> +    g_free(qu);
> +}
> +

Matches the other qnum_from_TYPE_test().  Let's clean them up before
this patch: use QDECREF() instead of g_free(), and drop the comment (see
Luiz's reply to PATCH 04).

>  static void qnum_from_double_test(void)
>  {
>      QNum *qf;
> @@ -73,6 +88,37 @@ static void qnum_get_int_test(void)
>      QDECREF(qi);
>  }
>  
> +static void qnum_get_uint_test(void)
> +{
> +    QNum *qn;
> +    const int value = 123456;
> +    Error *err = NULL;
> +
> +    qn = qnum_from_uint(value);
> +    g_assert(qnum_get_uint(qn, &error_abort) == value);
> +    QDECREF(qn);
> +
> +    qn = qnum_from_int(value);
> +    g_assert(qnum_get_uint(qn, &error_abort) == value);
> +    QDECREF(qn);
> +
> +    qn = qnum_from_int(-1);
> +    qnum_get_uint(qn, &err);
> +    error_free_or_abort(&err);
> +    QDECREF(qn);
> +
> +    qn = qnum_from_uint(-1ULL);
> +    qnum_get_int(qn, &err);
> +    error_free_or_abort(&err);
> +    QDECREF(qn);
> +
> +    /* invalid case */

No more invalid than the previous two, isn't it?

> +    qn = qnum_from_double(0.42);
> +    qnum_get_uint(qn, &err);
> +    error_free_or_abort(&err);
> +    QDECREF(qn);
> +}
> +
>  static void qobject_to_qnum_test(void)
>  {
>      QNum *qn;
> @@ -111,6 +157,9 @@ static void qnum_destroy_test(void)
>      qn = qnum_from_int(0);
>      QDECREF(qn);
>  
> +    qn = qnum_from_uint(0);
> +    QDECREF(qn);
> +
>      qn = qnum_from_double(0.42);
>      QDECREF(qn);
>  }

Let's drop this test before this patch.

> @@ -120,10 +169,12 @@ int main(int argc, char **argv)
>      g_test_init(&argc, &argv, NULL);
>  
>      g_test_add_func("/qnum/from_int", qnum_from_int_test);
> +    g_test_add_func("/qnum/from_uint", qnum_from_uint_test);
>      g_test_add_func("/qnum/from_double", qnum_from_double_test);
>      g_test_add_func("/qnum/destroy", qnum_destroy_test);
>      g_test_add_func("/qnum/from_int64", qnum_from_int64_test);
>      g_test_add_func("/qnum/get_int", qnum_get_int_test);
> +    g_test_add_func("/qnum/get_uint", qnum_get_uint_test);
>      g_test_add_func("/qnum/to_qnum", qobject_to_qnum_test);
>      g_test_add_func("/qnum/to_string", qnum_to_string_test);