Re: [Qemu-devel] [PATCH v2 13/45] json: learn to parse uint64 numbers

[Markus Armbruster]

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

> Switch strtoll() usage to qemu_strtoi64() helper while at it.
>
> Add a few tests for large numbers.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  qobject/json-lexer.c  |  4 ++++
>  qobject/json-parser.c | 30 ++++++++++++++++++++++++------
>  tests/check-qjson.c   | 37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+), 6 deletions(-)
>
> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
> index af4a75e05b..980ba159d6 100644
> --- a/qobject/json-lexer.c
> +++ b/qobject/json-lexer.c
> @@ -227,15 +227,18 @@ static const uint8_t json_lexer[][256] =  {
>      /* escape */
>      [IN_ESCAPE_LL] = {
>          ['d'] = JSON_ESCAPE,
> +        ['u'] = JSON_ESCAPE,
>      },
>  
>      [IN_ESCAPE_L] = {
>          ['d'] = JSON_ESCAPE,
>          ['l'] = IN_ESCAPE_LL,
> +        ['u'] = JSON_ESCAPE,
>      },
>  
>      [IN_ESCAPE_I64] = {
>          ['d'] = JSON_ESCAPE,
> +        ['u'] = JSON_ESCAPE,
>      },
>  
>      [IN_ESCAPE_I6] = {
> @@ -251,6 +254,7 @@ static const uint8_t json_lexer[][256] =  {
>          ['i'] = JSON_ESCAPE,
>          ['p'] = JSON_ESCAPE,
>          ['s'] = JSON_ESCAPE,
> +        ['u'] = JSON_ESCAPE,
>          ['f'] = JSON_ESCAPE,
>          ['l'] = IN_ESCAPE_L,
>          ['I'] = IN_ESCAPE_I,
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index b90b2fb45a..62dcac8128 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "qapi/qmp/types.h"
> @@ -472,6 +473,13 @@ static QObject *parse_escape(JSONParserContext *ctxt, 
> va_list *ap)
>      } else if (!strcmp(token->str, "%lld") ||
>                 !strcmp(token->str, "%I64d")) {
>          return QOBJECT(qnum_from_int(va_arg(*ap, long long)));
> +    } else if (!strcmp(token->str, "%u")) {
> +        return QOBJECT(qnum_from_uint(va_arg(*ap, unsigned int)));
> +    } else if (!strcmp(token->str, "%lu")) {
> +        return QOBJECT(qnum_from_uint(va_arg(*ap, unsigned long)));
> +    } else if (!strcmp(token->str, "%llu") ||
> +               !strcmp(token->str, "%I64u")) {
> +        return QOBJECT(qnum_from_uint(va_arg(*ap, unsigned long long)));
>      } else if (!strcmp(token->str, "%s")) {
>          return QOBJECT(qstring_from_str(va_arg(*ap, const char *)));
>      } else if (!strcmp(token->str, "%f")) {
> @@ -493,20 +501,30 @@ static QObject *parse_literal(JSONParserContext *ctxt)
>      case JSON_INTEGER: {
>          /*
>           * Represent JSON_INTEGER as QNUM_I64 if possible, else as
> -         * QNUM_DOUBLE. Note that strtoll() fails with ERANGE when
> -         * it's not possible.
> +         * QNUM_U64, else as QNUM_DOUBLE.  Note that qemu_strtoi64()
> +         * and qemu_strtou64 fail with ERANGE when it's not possible.

qemu_strtou64(), please.

>           *
>           * qnum_get_int() will then work for any signed 64-bit
> -         * JSON_INTEGER, and qnum_get_double both for any JSON_INTEGER
> +         * JSON_INTEGER, qnum_get_uint() for any unsigned 64-bit
> +         * integer, and qnum_get_double() both for any JSON_INTEGER
>           * and any JSON_FLOAT.
>           */
> +        int ret;
>          int64_t value;
> +        uint64_t uvalue;
>  
> -        errno = 0; /* strtoll doesn't set errno on success */
> -        value = strtoll(token->str, NULL, 10);
> -        if (errno != ERANGE) {
> +        ret = qemu_strtoi64(token->str, NULL, 10, &value);
> +        if (!ret) {
>              return QOBJECT(qnum_from_int(value));
>          }
> +        assert(ret == -ERANGE);
> +
> +        if (token->str[0] != '-') {
> +            ret = qemu_strtou64(token->str, NULL, 10, &uvalue);
> +            if (!ret) {
> +                return QOBJECT(qnum_from_uint(uvalue));
> +            }

assert(ret == -ERANGE), please.

> +        }
>          /* fall through to JSON_FLOAT */
>      }
>      case JSON_FLOAT:
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index 8ec728a702..6fb14445a3 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -906,6 +906,42 @@ static void simple_number(void)
>      }
>  }
>  
> +static void large_number(void)
> +{
> +    const char *maxu64 = "18446744073709551615"; /* 2^64-1 */
> +    const char *gtu64 = "18446744073709551616"; /* 2^64 */
> +    const char *range = "-9223372036854775809";

Why is this called @range?

Let's add /* -2^63-1 */.

> +    QNum *qnum;
> +    QString *str;
> +    uint64_t val;
> +
> +    qnum = qobject_to_qnum(qobject_from_json(maxu64, &error_abort));
> +    g_assert(qnum);
> +    g_assert(qnum_get_uint(qnum, &val));
> +    g_assert_cmpuint(val, ==, 18446744073709551615U);
> +
> +    str = qobject_to_json(QOBJECT(qnum));
> +    g_assert_cmpstr(qstring_get_str(str), ==, maxu64);
> +    QDECREF(str);
> +    QDECREF(qnum);
> +
> +    qnum = qobject_to_qnum(qobject_from_json(gtu64, &error_abort));
> +    g_assert(qnum);
> +    g_assert_cmpfloat(qnum_get_double(qnum), >, 0);

Why not check for the exact expected number?

> +    g_assert(!qnum_get_uint(qnum, &val));
> +
> +    str = qobject_to_json(QOBJECT(qnum));
> +    g_assert_cmpstr(qstring_get_str(str), ==, gtu64);
> +    QDECREF(str);
> +    QDECREF(qnum);
> +
> +    qnum = qobject_to_qnum(qobject_from_json(range, &error_abort));
> +    g_assert(qnum);
> +    g_assert_cmpfloat(qnum_get_double(qnum), <, 0);

Likewise.

> +    g_assert(!qnum_get_uint(qnum, &val));

Shouldn't we check the result of qobject_to_json() here?

> +    QDECREF(qnum);
> +}
> +
>  static void float_number(void)
>  {
>      int i;
> @@ -1475,6 +1511,7 @@ int main(int argc, char **argv)
>      g_test_add_func("/literals/string/vararg", vararg_string);
>  
>      g_test_add_func("/literals/number/simple", simple_number);
> +    g_test_add_func("/literals/number/large", large_number);
>      g_test_add_func("/literals/number/float", float_number);
>      g_test_add_func("/literals/number/vararg", vararg_number);