[Qemu-devel] [PATCH 0/4] Guarantee valid JSON in QMP, even for nonfinite numbers

[Qemu-devel] [PATCH 0/4] Guarantee valid JSON in QMP, even for nonfinite numbers

[Eric Blake]

Flesh out an idea that came up during my JSON visitor series.
Semantic errors are nicer than lexical errors.  While we generally
consider any non-finite number a bug in our code (that is, we
don't plan to expose them over QMP), at least we can guarantee
that even with a bug in our code we are still giving the user
valid JSON, similar to how we convert non-UTF-8 byte sequences
to use the replacement character for valid UTF-8 output.

I hit a couple of checkpatch false negatives in while writing this
series; one was easy to fix, but the other stumped me.  It doesn't
help that checkpatch tweaks also trigger checkpatch warnings...

Eric Blake (4):
  qobject: Correct JSON lexer grammar comments
  checkpatch: There is no qemu_strtod()
  qobject: Parse non-finite numbers, as an extension
  qobject: Output valid JSON for non-finite numbers

 qobject/json-lexer.c  | 32 ++++++++++++++++++++-------
 qobject/json-parser.c | 13 +++++++++--
 qobject/qjson.c       | 15 ++++++++++---
 tests/check-qjson.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++-------
 docs/qmp-spec.txt     |  4 ++++
 scripts/checkpatch.pl |  2 +-
 6 files changed, 105 insertions(+), 22 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/4] qobject: Correct JSON lexer grammar comments

[Eric Blake]

Fix the regex comments describing what we parse as JSON.  No change
to the lexer itself, just to the comments:
- The "" and '' string construction was missing alternation between
different escape sequences
- The construction for numbers forgot to handle optional leading '-'
- The construction for numbers was grouped incorrectly so that it
didn't permit '0.1'
- The construction for numbers forgot to mark the exponent as optional
- No mention that our '' string and "\'" are JSON extensions
- No mention of our %d and related extensions when constructing JSON

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qobject/json-lexer.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index 496374d..ebd15d8 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -18,11 +18,22 @@
 #define MAX_TOKEN_SIZE (64ULL << 20)

 /*
- * \"([^\\\"]|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
- * '([^\\']|(\\\"\\'\\\\\\/\\b\\f\\n\\r\\t\\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*'
- * 0|([1-9][0-9]*(.[0-9]+)?([eE]([-+])?[0-9]+))
+ * Required by JSON (RFC 7159), plus \' extension in "":
+ *
+ * \"([^\\\"]|(\\\"|\\'|\\\\|\\/|\\b|\\f|\\n|\\r|\\t|
+ *    \\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
+ * -?(0|[1-9][0-9]*)(.[0-9]+)?([eE][-+]?[0-9]+)?
  * [{}\[\],:]
- * [a-z]+
+ * [a-z]+   # covers null, true, false
+ *
+ * Extension of '' strings:
+ *
+ * '([^\\']|(\\\"|\\'|\\\\|\\/|\\b|\\f|\\n|\\r|\\t|
+ *    \\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*'
+ *
+ * Extension for vararg handling in JSON construction:
+ *
+ * %((l|ll|I64)?d|[ipsf])
  *
  */

@@ -213,7 +224,7 @@ static const uint8_t json_lexer[][256] =  {
         ['\t'] = IN_WHITESPACE,
         ['\r'] = IN_WHITESPACE,
         ['\n'] = IN_WHITESPACE,
-    },        
+    },

     /* escape */
     [IN_ESCAPE_LL] = {
-- 
2.5.5

[Qemu-devel] [PATCH 2/4] checkpatch: There is no qemu_strtod()

[Eric Blake]

Maybe there should be; but until there is, we should not flag
strtod() calls as something to replaced with qemu_strtod().

We also lack qemu_strtof() and qemu_strtold(), but as no one
has been using strtof() or strtold(), it's not worth complicating
the regex for them.

(Ironically, I had to use 'git commit -n' since checkpatch uses
TAB indents, in violation of its own recommendations.)

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c939a32..cf32c8f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2453,7 +2453,7 @@ sub process {
 		}

 # recommend qemu_strto* over strto* for numeric conversions
-		if ($line =~ /\b(strto[^k].*?)\s*\(/) {
+		if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
 			WARN("consider using qemu_$1 in preference to $1\n" . $herecurr);
 		}
 # check for module_init(), use category-specific init macros explicitly please
-- 
2.5.5

[Qemu-devel] [PATCH 3/4] qobject: Parse non-finite numbers, as an extension

[Eric Blake]

There have been times in the past where we have been careless and
allowed non-finite values to escape as a 'number' type in QMP
output (such as before commit 27ff42e).  This is in violation of
the JSON specification in RFC 7159, and usually caused by
floating-point division by 0 resulting in NaN, although infinity
is also a possible culprit.  While a future patch will tighten the
output generator to avoid a lexical error from a strict JSON
parser, we might as well first fix our parser to accept non-finite
values as an extension, so that we can always read what we have
output in the past ("be liberal in what you accept, strict in what
 you produce").

Rather than complicate the lexer to add LOTS of states for each
letter within 'nan' and 'inf[inity]', I chose to just abuse the
'keyword' token, but had to make it accept upper case.  Also, since
I want to parse '-inf', I had to tweak IN_NEG_NONZERO_NUMBER; and
renamed that in the process (we have always accepted '-0', so the
state name was a misnomer).  Then the parser then does the real magic
of creating a QFloat object if a non-finite "keyword" was recognized.

I intentionally did not support NAN(n-char-sequence) forms, even
though C99 requires it to work for strtod(), in part because
"implementation-specified" n-char-sequence is rather hard to test
in a platform-agnostic manner, and in part because we've never
actually output it.

Improve the testsuite to cover the new extension, including working
around the fact that g_assert_cmpfloat() does NOT test equivalence,
but merely mathematical equality (0.0 and -0.0 are equal but not
equivalent, NaN and NaN are equivalent but not equal), and enhance
the existing tests for -0.0 in the process.

Checkpatch has a false negative, complaining that there should be
spaces around binary '-' in what is really the float literal
-32.20e-10.  It was over my head to figure out how to silence that
one.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qobject/json-lexer.c  | 15 ++++++++-----
 qobject/json-parser.c | 13 +++++++++--
 tests/check-qjson.c   | 62 ++++++++++++++++++++++++++++++++++++++++++++-------
 docs/qmp-spec.txt     |  4 ++++
 4 files changed, 79 insertions(+), 15 deletions(-)

diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index ebd15d8..de16219 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -18,13 +18,14 @@
 #define MAX_TOKEN_SIZE (64ULL << 20)

 /*
- * Required by JSON (RFC 7159), plus \' extension in "":
+ * Required by JSON (RFC 7159), plus \' extension in "", and extension
+ * of parsing case-insensitive non-finite numbers like "NaN" and "-Inf":
  *
  * \"([^\\\"]|(\\\"|\\'|\\\\|\\/|\\b|\\f|\\n|\\r|\\t|
  *    \\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
  * -?(0|[1-9][0-9]*)(.[0-9]+)?([eE][-+]?[0-9]+)?
  * [{}\[\],:]
- * [a-z]+   # covers null, true, false
+ * -?[a-zA-Z]+   # covers null, true, false, nan, inf[inity]
  *
  * Extension of '' strings:
  *
@@ -58,7 +59,7 @@ enum json_lexer_state {
     IN_MANTISSA,
     IN_MANTISSA_DIGITS,
     IN_NONZERO_NUMBER,
-    IN_NEG_NONZERO_NUMBER,
+    IN_NEG_NUMBER,
     IN_KEYWORD,
     IN_ESCAPE,
     IN_ESCAPE_L,
@@ -206,15 +207,18 @@ static const uint8_t json_lexer[][256] =  {
         ['.'] = IN_MANTISSA,
     },

-    [IN_NEG_NONZERO_NUMBER] = {
+    [IN_NEG_NUMBER] = {
         ['0'] = IN_ZERO,
         ['1' ... '9'] = IN_NONZERO_NUMBER,
+        ['a' ... 'z'] = IN_KEYWORD,
+        ['A' ... 'Z'] = IN_KEYWORD,
     },

     /* keywords */
     [IN_KEYWORD] = {
         TERMINAL(JSON_KEYWORD),
         ['a' ... 'z'] = IN_KEYWORD,
+        ['A' ... 'Z'] = IN_KEYWORD,
     },

     /* whitespace */
@@ -264,7 +268,7 @@ static const uint8_t json_lexer[][256] =  {
         ['\''] = IN_SQ_STRING,
         ['0'] = IN_ZERO,
         ['1' ... '9'] = IN_NONZERO_NUMBER,
-        ['-'] = IN_NEG_NONZERO_NUMBER,
+        ['-'] = IN_NEG_NUMBER,
         ['{'] = JSON_LCURLY,
         ['}'] = JSON_RCURLY,
         ['['] = JSON_LSQUARE,
@@ -272,6 +276,7 @@ static const uint8_t json_lexer[][256] =  {
         [','] = JSON_COMMA,
         [':'] = JSON_COLON,
         ['a' ... 'z'] = IN_KEYWORD,
+        ['A' ... 'Z'] = IN_KEYWORD,
         ['%'] = IN_ESCAPE,
         [' '] = IN_WHITESPACE,
         ['\t'] = IN_WHITESPACE,
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index 67ed727..12519b6 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -450,6 +450,16 @@ static QObject *parse_keyword(JSONParserContext *ctxt)
         return QOBJECT(qbool_from_bool(false));
     } else if (!strcmp(token->str, "null")) {
         return qnull();
+    } else {
+        double d;
+        char *p;
+
+        /* The lexer feeds us "NaN" and "-Inf" as keywords */
+        errno = 0;
+        d = strtod(token->str, &p);
+        if (!errno && p != token->str && !*p) {
+            return QOBJECT(qfloat_from_double(d));
+        }
     }
     parse_error(ctxt, token, "invalid keyword '%s'", token->str);
     return NULL;
@@ -519,8 +529,7 @@ static QObject *parse_literal(JSONParserContext *ctxt)
     }
     case JSON_FLOAT:
         /* FIXME dependent on locale; a pervasive issue in QEMU */
-        /* FIXME our lexer matches RFC 7159 in forbidding Inf or NaN,
-         * but those might be useful extensions beyond JSON */
+        /* NaN and infinity are parsed as extensions under parse_keyword() */
         return QOBJECT(qfloat_from_double(strtod(token->str, NULL)));
     default:
         abort();
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 0e158f6..95adf64 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -11,6 +11,7 @@
  *
  */
 #include "qemu/osdep.h"
+#include <math.h>

 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qint.h"
@@ -932,34 +933,78 @@ static void float_number(void)
     struct {
         const char *encoded;
         double decoded;
-        int skip;
+        const char *canonical;
     } test_cases[] = {
         { "32.43", 32.43 },
         { "0.222", 0.222 },
         { "-32.12313", -32.12313 },
-        { "-32.20e-10", -32.20e-10, .skip = 1 },
+        { "-32.20e-10", -32.20e-10, "-0" }, /* FIXME: Our rounding is lousy */
+        { "-0.0", -0.0, "-0" },
         { },
     };

     for (i = 0; test_cases[i].encoded; i++) {
         QObject *obj;
         QFloat *qfloat;
+        QString *str;

         obj = qobject_from_json(test_cases[i].encoded);
         g_assert(obj != NULL);
         g_assert(qobject_type(obj) == QTYPE_QFLOAT);

         qfloat = qobject_to_qfloat(obj);
-        g_assert(qfloat_get_double(qfloat) == test_cases[i].decoded);
+        g_assert_cmpfloat(qfloat_get_double(qfloat), ==,
+                          test_cases[i].decoded);
+        g_assert_cmpint(signbit(qfloat_get_double(qfloat)), ==,
+                        signbit(test_cases[i].decoded));

-        if (test_cases[i].skip == 0) {
-            QString *str;
+        str = qobject_to_json(obj);
+        g_assert_cmpstr(qstring_get_str(str), ==,
+                        test_cases[i].canonical ?: test_cases[i].encoded);
+        QDECREF(str);
+        QDECREF(qfloat);
+    }
+}

-            str = qobject_to_json(obj);
-            g_assert(strcmp(qstring_get_str(str), test_cases[i].encoded) == 0);
-            QDECREF(str);
+static void non_finite_number(void)
+{
+    int i;
+    struct {
+        const char *encoded;
+        double decoded;
+        const char *canonical;
+    } test_cases[] = {
+        { "nan", NAN },
+        { "NaN", NAN, "nan" },
+        /* Not all libc implementations can round-trip '-nan' */
+        /* We do not support forms like 'NAN(0)' */
+        { "inf", INFINITY },
+        { "-INFINITY", -INFINITY, "-inf" },
+        { },
+    };
+
+    for (i = 0; test_cases[i].encoded; i++) {
+        QObject *obj;
+        QFloat *qfloat;
+        QString *str;
+
+        obj = qobject_from_json(test_cases[i].encoded);
+        g_assert(obj != NULL);
+        g_assert(qobject_type(obj) == QTYPE_QFLOAT);
+
+        qfloat = qobject_to_qfloat(obj);
+        /* g_assert_cmpfloat(NAN, ==, NAN) doesn't do what we want */
+        g_assert_cmpint(fpclassify(qfloat_get_double(qfloat)), ==,
+                        fpclassify(test_cases[i].decoded));
+        if (!isnan(test_cases[i].decoded)) {
+            g_assert_cmpfloat(qfloat_get_double(qfloat), ==,
+                              test_cases[i].decoded);
         }

+        str = qobject_to_json(obj);
+        g_assert_cmpstr(qstring_get_str(str), ==,
+                        test_cases[i].canonical ?: test_cases[i].encoded);
+        QDECREF(str);
         QDECREF(qfloat);
     }
 }
@@ -1520,6 +1565,7 @@ int main(int argc, char **argv)

     g_test_add_func("/literals/number/simple", simple_number);
     g_test_add_func("/literals/number/float", float_number);
+    g_test_add_func("/literals/number/non-finite", non_finite_number);
     g_test_add_func("/literals/number/vararg", vararg_number);

     g_test_add_func("/literals/keyword", keyword_literal);
diff --git a/docs/qmp-spec.txt b/docs/qmp-spec.txt
index f8b5356..bfba431 100644
--- a/docs/qmp-spec.txt
+++ b/docs/qmp-spec.txt
@@ -51,6 +51,10 @@ json-string, and both input forms of strings understand an additional
 escape sequence of "\'" for a single quote. The server will only use
 double quoting on output.

+As an extension, the server understands case-insensitive forms of
+non-finite numbers, such as "NaN", "Inf", and "-infinity" (but not
+"NaN(...)").
+
 2.1 General Definitions
 -----------------------

-- 
2.5.5

[Qemu-devel] [PATCH 4/4] qobject: Output valid JSON for non-finite numbers

[Eric Blake]

It's better to give downstream clients a valid JSON string,
even if they are semantically expecting a number, than it is
to give them a bare keyword extension that can cause a
lexical error.

Of course, as long as we don't recognize (certain) strings as valid
numbers during a conversion to QObject, this means our extension
of accepting bare keywords for non-finite numbers cannot undergo
a round trip (once converted into a string, we never get back to
a QFloat).  However, non-finite input is rare enough that it's
not worth bothering with at the moment.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qobject/qjson.c     | 15 ++++++++++++---
 tests/check-qjson.c | 11 +++++------
 docs/qmp-spec.txt   |  2 +-
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/qobject/qjson.c b/qobject/qjson.c
index ef160d2..465b080 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -12,6 +12,7 @@
  */

 #include "qemu/osdep.h"
+#include <math.h>
 #include "qapi/qmp/json-lexer.h"
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/json-streamer.h"
@@ -236,19 +237,27 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
     }
     case QTYPE_QFLOAT: {
         QFloat *val = qobject_to_qfloat(obj);
+        double d = qfloat_get_double(val);
         char buffer[1024];
         int len;

+        if (!isfinite(d)) {
+            /* Always output valid JSON - a semantic error due to a
+             * string where a number was expected is better than a
+             * lexical error from a strict parser */
+            snprintf(buffer, sizeof(buffer), "\"%f\"", d);
+            qstring_append(str, buffer);
+            break;
+        }
+
         /* FIXME: snprintf() is locale dependent; but JSON requires
          * numbers to be formatted as if in the C locale. Dependence
          * on C locale is a pervasive issue in QEMU. */
-        /* FIXME: This risks printing Inf or NaN, which are not valid
-         * JSON values. */
         /* FIXME: the default precision of 6 for %f often causes
          * rounding errors; we should be using DBL_DECIMAL_DIG (17),
          * and only rounding to a shorter number if the result would
          * still produce the same floating point value.  */
-        len = snprintf(buffer, sizeof(buffer), "%f", qfloat_get_double(val));
+        len = snprintf(buffer, sizeof(buffer), "%f", d);
         while (len > 0 && buffer[len - 1] == '0') {
             len--;
         }
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 95adf64..b0a9178 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -974,12 +974,12 @@ static void non_finite_number(void)
         double decoded;
         const char *canonical;
     } test_cases[] = {
-        { "nan", NAN },
-        { "NaN", NAN, "nan" },
+        { "nan", NAN, "\"nan\"" },
+        { "NaN", NAN, "\"nan\"" },
         /* Not all libc implementations can round-trip '-nan' */
         /* We do not support forms like 'NAN(0)' */
-        { "inf", INFINITY },
-        { "-INFINITY", -INFINITY, "-inf" },
+        { "inf", INFINITY, "\"inf\"" },
+        { "-INFINITY", -INFINITY, "\"-inf\"" },
         { },
     };

@@ -1002,8 +1002,7 @@ static void non_finite_number(void)
         }

         str = qobject_to_json(obj);
-        g_assert_cmpstr(qstring_get_str(str), ==,
-                        test_cases[i].canonical ?: test_cases[i].encoded);
+        g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].canonical);
         QDECREF(str);
         QDECREF(qfloat);
     }
diff --git a/docs/qmp-spec.txt b/docs/qmp-spec.txt
index bfba431..75c68d9 100644
--- a/docs/qmp-spec.txt
+++ b/docs/qmp-spec.txt
@@ -53,7 +53,7 @@ double quoting on output.

 As an extension, the server understands case-insensitive forms of
 non-finite numbers, such as "NaN", "Inf", and "-infinity" (but not
-"NaN(...)").
+"NaN(...)"); however, such numbers are output as a json-string.

 2.1 General Definitions
 -----------------------
-- 
2.5.5

Re: [Qemu-devel] [PATCH 3/4] qobject: Parse non-finite numbers, as an extension

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> There have been times in the past where we have been careless and
> allowed non-finite values to escape as a 'number' type in QMP
> output (such as before commit 27ff42e).  This is in violation of
> the JSON specification in RFC 7159, and usually caused by
> floating-point division by 0 resulting in NaN, although infinity
> is also a possible culprit.  While a future patch will tighten the
> output generator to avoid a lexical error from a strict JSON
> parser, we might as well first fix our parser to accept non-finite
> values as an extension, so that we can always read what we have
> output in the past ("be liberal in what you accept, strict in what
>  you produce").
>
> Rather than complicate the lexer to add LOTS of states for each
> letter within 'nan' and 'inf[inity]', I chose to just abuse the
> 'keyword' token, but had to make it accept upper case.  Also, since
> I want to parse '-inf', I had to tweak IN_NEG_NONZERO_NUMBER; and
> renamed that in the process (we have always accepted '-0', so the
> state name was a misnomer).  Then the parser then does the real magic

Then the parser does

> of creating a QFloat object if a non-finite "keyword" was recognized.
>
> I intentionally did not support NAN(n-char-sequence) forms, even
> though C99 requires it to work for strtod(), in part because
> "implementation-specified" n-char-sequence is rather hard to test
> in a platform-agnostic manner, and in part because we've never
> actually output it.

I'm afraid only because libc didn't.

§7.19.6.1:

    A double argument representing an infinity is converted in one of
    the styles [-]inf or [-]infinity -- which style is
    implementation-defined.  A double argument representing a NaN is
    converted in one of the styles [-]nan or [-]nan(n-char-sequence) --
    which style, and the meaning of any n-char- sequence, is
    implementation-defined.

> Improve the testsuite to cover the new extension, including working
> around the fact that g_assert_cmpfloat() does NOT test equivalence,
> but merely mathematical equality (0.0 and -0.0 are equal but not
> equivalent, NaN and NaN are equivalent but not equal), and enhance
> the existing tests for -0.0 in the process.
>
> Checkpatch has a false negative, complaining that there should be

Isn't this a false positive?

> spaces around binary '-' in what is really the float literal
> -32.20e-10.  It was over my head to figure out how to silence that
> one.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qobject/json-lexer.c  | 15 ++++++++-----
>  qobject/json-parser.c | 13 +++++++++--
>  tests/check-qjson.c   | 62 ++++++++++++++++++++++++++++++++++++++++++++-------
>  docs/qmp-spec.txt     |  4 ++++
>  4 files changed, 79 insertions(+), 15 deletions(-)
>
> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
> index ebd15d8..de16219 100644
> --- a/qobject/json-lexer.c
> +++ b/qobject/json-lexer.c
> @@ -18,13 +18,14 @@
>  #define MAX_TOKEN_SIZE (64ULL << 20)
>
>  /*
> - * Required by JSON (RFC 7159), plus \' extension in "":
> + * Required by JSON (RFC 7159), plus \' extension in "", and extension
> + * of parsing case-insensitive non-finite numbers like "NaN" and "-Inf":
>   *
>   * \"([^\\\"]|(\\\"|\\'|\\\\|\\/|\\b|\\f|\\n|\\r|\\t|
>   *    \\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
>   * -?(0|[1-9][0-9]*)(.[0-9]+)?([eE][-+]?[0-9]+)?
>   * [{}\[\],:]
> - * [a-z]+   # covers null, true, false
> + * -?[a-zA-Z]+   # covers null, true, false, nan, inf[inity]
>   *
>   * Extension of '' strings:
>   *
> @@ -58,7 +59,7 @@ enum json_lexer_state {
>      IN_MANTISSA,
>      IN_MANTISSA_DIGITS,
>      IN_NONZERO_NUMBER,
> -    IN_NEG_NONZERO_NUMBER,
> +    IN_NEG_NUMBER,
>      IN_KEYWORD,
>      IN_ESCAPE,
>      IN_ESCAPE_L,
> @@ -206,15 +207,18 @@ static const uint8_t json_lexer[][256] =  {
>          ['.'] = IN_MANTISSA,
>      },
>
> -    [IN_NEG_NONZERO_NUMBER] = {
> +    [IN_NEG_NUMBER] = {
>          ['0'] = IN_ZERO,
>          ['1' ... '9'] = IN_NONZERO_NUMBER,
> +        ['a' ... 'z'] = IN_KEYWORD,
> +        ['A' ... 'Z'] = IN_KEYWORD,
>      },
>
>      /* keywords */
>      [IN_KEYWORD] = {
>          TERMINAL(JSON_KEYWORD),
>          ['a' ... 'z'] = IN_KEYWORD,
> +        ['A' ... 'Z'] = IN_KEYWORD,
>      },
>
>      /* whitespace */
> @@ -264,7 +268,7 @@ static const uint8_t json_lexer[][256] =  {
>          ['\''] = IN_SQ_STRING,
>          ['0'] = IN_ZERO,
>          ['1' ... '9'] = IN_NONZERO_NUMBER,
> -        ['-'] = IN_NEG_NONZERO_NUMBER,
> +        ['-'] = IN_NEG_NUMBER,
>          ['{'] = JSON_LCURLY,
>          ['}'] = JSON_RCURLY,
>          ['['] = JSON_LSQUARE,
> @@ -272,6 +276,7 @@ static const uint8_t json_lexer[][256] =  {
>          [','] = JSON_COMMA,
>          [':'] = JSON_COLON,
>          ['a' ... 'z'] = IN_KEYWORD,
> +        ['A' ... 'Z'] = IN_KEYWORD,
>          ['%'] = IN_ESCAPE,
>          [' '] = IN_WHITESPACE,
>          ['\t'] = IN_WHITESPACE,
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c
> index 67ed727..12519b6 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -450,6 +450,16 @@ static QObject *parse_keyword(JSONParserContext *ctxt)
>          return QOBJECT(qbool_from_bool(false));
>      } else if (!strcmp(token->str, "null")) {
>          return qnull();
> +    } else {
> +        double d;
> +        char *p;
> +
> +        /* The lexer feeds us "NaN" and "-Inf" as keywords */
> +        errno = 0;
> +        d = strtod(token->str, &p);
> +        if (!errno && p != token->str && !*p) {
> +            return QOBJECT(qfloat_from_double(d));
> +        }
>      }
>      parse_error(ctxt, token, "invalid keyword '%s'", token->str);
>      return NULL;
> @@ -519,8 +529,7 @@ static QObject *parse_literal(JSONParserContext *ctxt)
>      }
>      case JSON_FLOAT:
>          /* FIXME dependent on locale; a pervasive issue in QEMU */
> -        /* FIXME our lexer matches RFC 7159 in forbidding Inf or NaN,
> -         * but those might be useful extensions beyond JSON */
> +        /* NaN and infinity are parsed as extensions under parse_keyword() */

"Under"?  What about "extensions, by parse_keyword()"?

>          return QOBJECT(qfloat_from_double(strtod(token->str, NULL)));
>      default:
>          abort();
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index 0e158f6..95adf64 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -11,6 +11,7 @@
>   *
>   */
>  #include "qemu/osdep.h"
> +#include <math.h>
>
>  #include "qapi/qmp/qstring.h"
>  #include "qapi/qmp/qint.h"
> @@ -932,34 +933,78 @@ static void float_number(void)
>      struct {
>          const char *encoded;
>          double decoded;
> -        int skip;
> +        const char *canonical;
>      } test_cases[] = {
>          { "32.43", 32.43 },
>          { "0.222", 0.222 },
>          { "-32.12313", -32.12313 },
> -        { "-32.20e-10", -32.20e-10, .skip = 1 },
> +        { "-32.20e-10", -32.20e-10, "-0" }, /* FIXME: Our rounding is lousy */
> +        { "-0.0", -0.0, "-0" },
>          { },
>      };
>
>      for (i = 0; test_cases[i].encoded; i++) {
>          QObject *obj;
>          QFloat *qfloat;
> +        QString *str;
>
>          obj = qobject_from_json(test_cases[i].encoded);
>          g_assert(obj != NULL);
>          g_assert(qobject_type(obj) == QTYPE_QFLOAT);
>
>          qfloat = qobject_to_qfloat(obj);
> -        g_assert(qfloat_get_double(qfloat) == test_cases[i].decoded);
> +        g_assert_cmpfloat(qfloat_get_double(qfloat), ==,
> +                          test_cases[i].decoded);
> +        g_assert_cmpint(signbit(qfloat_get_double(qfloat)), ==,
> +                        signbit(test_cases[i].decoded));

Could use a comment.

>
> -        if (test_cases[i].skip == 0) {
> -            QString *str;
> +        str = qobject_to_json(obj);
> +        g_assert_cmpstr(qstring_get_str(str), ==,
> +                        test_cases[i].canonical ?: test_cases[i].encoded);
> +        QDECREF(str);
> +        QDECREF(qfloat);
> +    }
> +}
>
> -            str = qobject_to_json(obj);
> -            g_assert(strcmp(qstring_get_str(str), test_cases[i].encoded) == 0);
> -            QDECREF(str);
> +static void non_finite_number(void)
> +{
> +    int i;
> +    struct {
> +        const char *encoded;
> +        double decoded;
> +        const char *canonical;
> +    } test_cases[] = {
> +        { "nan", NAN },
> +        { "NaN", NAN, "nan" },
> +        /* Not all libc implementations can round-trip '-nan' */
> +        /* We do not support forms like 'NAN(0)' */
> +        { "inf", INFINITY },
> +        { "-INFINITY", -INFINITY, "-inf" },
> +        { },
> +    };
> +
> +    for (i = 0; test_cases[i].encoded; i++) {
> +        QObject *obj;
> +        QFloat *qfloat;
> +        QString *str;
> +
> +        obj = qobject_from_json(test_cases[i].encoded);
> +        g_assert(obj != NULL);
> +        g_assert(qobject_type(obj) == QTYPE_QFLOAT);
> +
> +        qfloat = qobject_to_qfloat(obj);
> +        /* g_assert_cmpfloat(NAN, ==, NAN) doesn't do what we want */
> +        g_assert_cmpint(fpclassify(qfloat_get_double(qfloat)), ==,
> +                        fpclassify(test_cases[i].decoded));
> +        if (!isnan(test_cases[i].decoded)) {
> +            g_assert_cmpfloat(qfloat_get_double(qfloat), ==,
> +                              test_cases[i].decoded);
>          }
>
> +        str = qobject_to_json(obj);
> +        g_assert_cmpstr(qstring_get_str(str), ==,
> +                        test_cases[i].canonical ?: test_cases[i].encoded);
> +        QDECREF(str);
>          QDECREF(qfloat);
>      }
>  }
> @@ -1520,6 +1565,7 @@ int main(int argc, char **argv)
>
>      g_test_add_func("/literals/number/simple", simple_number);
>      g_test_add_func("/literals/number/float", float_number);
> +    g_test_add_func("/literals/number/non-finite", non_finite_number);
>      g_test_add_func("/literals/number/vararg", vararg_number);
>
>      g_test_add_func("/literals/keyword", keyword_literal);
> diff --git a/docs/qmp-spec.txt b/docs/qmp-spec.txt
> index f8b5356..bfba431 100644
> --- a/docs/qmp-spec.txt
> +++ b/docs/qmp-spec.txt
> @@ -51,6 +51,10 @@ json-string, and both input forms of strings understand an additional
>  escape sequence of "\'" for a single quote. The server will only use
>  double quoting on output.
>
> +As an extension, the server understands case-insensitive forms of
> +non-finite numbers, such as "NaN", "Inf", and "-infinity" (but not
> +"NaN(...)").
> +
>  2.1 General Definitions
>  -----------------------

Re: [Qemu-devel] [PATCH 4/4] qobject: Output valid JSON for non-finite numbers

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> It's better to give downstream clients a valid JSON string,
> even if they are semantically expecting a number, than it is
> to give them a bare keyword extension that can cause a
> lexical error.

Incompatible change.  If all clients are choking on non-finite numbers,
then the incompatibility is an improvement.  If a client exists that
groks non-finite numbers, ...  Absence is always hard to show.

Moreover, it turns query-qmp-schema into a liar: the schema it returns
claims a certain member of the reply has "type": "number", and then we
go on to send a string anyway.

> Of course, as long as we don't recognize (certain) strings as valid
> numbers during a conversion to QObject,

That would be even crazier!

>                                         this means our extension
> of accepting bare keywords for non-finite numbers cannot undergo
> a round trip (once converted into a string, we never get back to
> a QFloat).  However, non-finite input is rare enough that it's
> not worth bothering with at the moment.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

I'm afraid the only sane solution is to find all uses of number in QMP
output, audit the code producing them, then assert isfinite() in the
monitor.  For commands without a side effect, we could fail the command
instead of tripping an assertion.  We'd have to declare such commands.

Let's examine the occurences of "number" in output of query-qmp-schema,
or actually in the qmp-introspect.c that gets generated with -u:

* Object q_obj_migrate_set_downtime-arg member value: input

* Builtin number: d'uh!

* Object MigrationStats member mbps: in output of query-migrate

* Object XBZRLECacheStats member overflow: likewise

* Object KeyValue case number: not a type.

* Object BlockDeviceTimedStats members avg_rd_queue_depth,
  avg_wr_queue_depth: in output of query-blockstats

* Enum CommandLineParameterType member: not a type

* Enum JSONType member: not a type

* Enum KeyValueKind: not a type

* Object PciBusInfo member: not a type

So it's just query-migrate and query-blockstats.

Re: [Qemu-devel] [PATCH 1/4] qobject: Correct JSON lexer grammar comments

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Fix the regex comments describing what we parse as JSON.  No change
> to the lexer itself, just to the comments:
> - The "" and '' string construction was missing alternation between
> different escape sequences
> - The construction for numbers forgot to handle optional leading '-'
> - The construction for numbers was grouped incorrectly so that it
> didn't permit '0.1'
> - The construction for numbers forgot to mark the exponent as optional
> - No mention that our '' string and "\'" are JSON extensions
> - No mention of our %d and related extensions when constructing JSON
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

I'll take this one through qapi-next.  Thanks!

Re: [Qemu-devel] [PATCH 2/4] checkpatch: There is no qemu_strtod()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Maybe there should be; but until there is, we should not flag
> strtod() calls as something to replaced with qemu_strtod().
>
> We also lack qemu_strtof() and qemu_strtold(), but as no one
> has been using strtof() or strtold(), it's not worth complicating
> the regex for them.
>
> (Ironically, I had to use 'git commit -n' since checkpatch uses
> TAB indents, in violation of its own recommendations.)
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c939a32..cf32c8f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2453,7 +2453,7 @@ sub process {
>  		}
>
>  # recommend qemu_strto* over strto* for numeric conversions
> -		if ($line =~ /\b(strto[^k].*?)\s*\(/) {
> +		if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
>  			WARN("consider using qemu_$1 in preference to $1\n" . $herecurr);
>  		}
>  # check for module_init(), use category-specific init macros explicitly please

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Paolo, if you want me to smuggle this into master via qapi-next, let me
know.

Re: [Qemu-devel] [PATCH 3/4] qobject: Parse non-finite numbers, as an extension

[Markus Armbruster]

I think this commit mixes up parsing of non-finite numbers, which we may
or may not want, with general test improvements, which we'll want
regardless.  Please split the patch.

On the parsing of non-finite numbers: the code looks good to me, but as
long as we're not ready to extend QMP to include non-finite numbers both
ways, I doubt we need to parse them.

Re: [Qemu-devel] [PATCH 2/4] checkpatch: There is no qemu_strtod()

[Paolo Bonzini]

On 16/06/2016 18:20, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Maybe there should be; but until there is, we should not flag
>> strtod() calls as something to replaced with qemu_strtod().
>>
>> We also lack qemu_strtof() and qemu_strtold(), but as no one
>> has been using strtof() or strtold(), it's not worth complicating
>> the regex for them.
>>
>> (Ironically, I had to use 'git commit -n' since checkpatch uses
>> TAB indents, in violation of its own recommendations.)
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  scripts/checkpatch.pl | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index c939a32..cf32c8f 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2453,7 +2453,7 @@ sub process {
>>  		}
>>
>>  # recommend qemu_strto* over strto* for numeric conversions
>> -		if ($line =~ /\b(strto[^k].*?)\s*\(/) {
>> +		if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
>>  			WARN("consider using qemu_$1 in preference to $1\n" . $herecurr);
>>  		}
>>  # check for module_init(), use category-specific init macros explicitly please
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> Paolo, if you want me to smuggle this into master via qapi-next, let me
> know.

Yes, please do.

Paolo

Re: [Qemu-devel] [PATCH 1/4] qobject: Correct JSON lexer grammar comments

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]



On 06/16/2016 10:19 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Fix the regex comments describing what we parse as JSON.  No change
>> to the lexer itself, just to the comments:
>> - The "" and '' string construction was missing alternation between
>> different escape sequences
>> - The construction for numbers forgot to handle optional leading '-'
>> - The construction for numbers was grouped incorrectly so that it
>> didn't permit '0.1'
>> - The construction for numbers forgot to mark the exponent as optional
>> - No mention that our '' string and "\'" are JSON extensions
>> - No mention of our %d and related extensions when constructing JSON
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> I'll take this one through qapi-next.  Thanks!
> 

You may want to squash this in for shorter lines:


diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
index de16219..b030576 100644
--- a/qobject/json-lexer.c
+++ b/qobject/json-lexer.c
@@ -21,16 +21,14 @@
  * Required by JSON (RFC 7159), plus \' extension in "", and extension
  * of parsing case-insensitive non-finite numbers like "NaN" and "-Inf":
  *
- * \"([^\\\"]|(\\\"|\\'|\\\\|\\/|\\b|\\f|\\n|\\r|\\t|
- *    \\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
+ * \"([^\\\"]|\\[\"'\\/bfnrt]|\\u[0-9a-fA-F]{4})*\"
  * -?(0|[1-9][0-9]*)(.[0-9]+)?([eE][-+]?[0-9]+)?
  * [{}\[\],:]
  * -?[a-zA-Z]+   # covers null, true, false, nan, inf[inity]
  *
  * Extension of '' strings:
  *
- * '([^\\']|(\\\"|\\'|\\\\|\\/|\\b|\\f|\\n|\\r|\\t|
- *    \\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*'
+ * '([^\\']|\\[\"'\\/bfnrt]|\\u[0-9a-fA-F]{4})*'
  *
  * Extension for vararg handling in JSON construction:
  *

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/4] qobject: Parse non-finite numbers, as an extension

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1937 bytes --]

On 06/16/2016 10:25 AM, Markus Armbruster wrote:
> I think this commit mixes up parsing of non-finite numbers, which we may
> or may not want, with general test improvements, which we'll want
> regardless.  Please split the patch.
> 
> On the parsing of non-finite numbers: the code looks good to me, but as
> long as we're not ready to extend QMP to include non-finite numbers both
> ways, I doubt we need to parse them.

Even if we want to guarantee that we will never generate a non-finite
number as the result of a QMP command, we DO have to worry about the
'id' field.  With all this series applied:

$  ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp
stdio{"QMP": {"version": {"qemu": {"micro": 50, "minor": 6, "major": 2},
"package": " (v2.6.0-1151-g1bb9f5e)"}, "capabilities": []}}
{'execute':'qmp_capabilities', 'id':[nan,inf,-Infinity]}
{"return": {}, "id": ["nan", "inf", "-inf"]}

and with just 1-3 applied:

$ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp
stdio{"QMP": {"version": {"qemu": {"micro": 50, "minor": 6, "major": 2},
"package": " (v2.6.0-1151-g1bb9f5e)"}, "capabilities": []}}
{'execute':'qmp_capabilities', 'id':[nan,inf,-Infinity]}
{"return": {}, "id": [nan, inf, -inf]}

[okay, I cheated, as evidenced by the new git stuff appended in the
version output being the same between the two lines, but you get the idea].

I'm comfortable with a compromise that asserts that QMP commands will
never output a non-finite number anywhere that introspection lists
'number', but that QMP itself understands the extension of parsing a
bare inf anywhere, and if a bare inf appears under 'id', then the replay
of 'id' will also include a bare inf (if your JSON library allows you to
send non-JSON, it should also allow you to parse non-JSON).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/4] qobject: Output valid JSON for non-finite numbers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3413 bytes --]

On 06/16/2016 10:17 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> It's better to give downstream clients a valid JSON string,
>> even if they are semantically expecting a number, than it is
>> to give them a bare keyword extension that can cause a
>> lexical error.
> 
> Incompatible change.  If all clients are choking on non-finite numbers,
> then the incompatibility is an improvement.  If a client exists that
> groks non-finite numbers, ...  Absence is always hard to show.

The 'id' field is an outlier - there, we replay the user's input with no
contextual interpretation (however, we DO reserve the right to reorder
the keys in the dicts that we replay, and to canonicalize UTF-8 text or
otherwise alter their input to something "equivalent").

> 
> Moreover, it turns query-qmp-schema into a liar: the schema it returns
> claims a certain member of the reply has "type": "number", and then we
> go on to send a string anyway.

The 'id' field is documented as sending ANY JSON value, so if we argue
that canonicalizing their extension input of a bare inf into a proper
JSON string on output is reasonable, then we may want this patch in
addition to adding assertions that none of the QMP commands with
introspectible 'number' ever output non-finite values.

> 
>> Of course, as long as we don't recognize (certain) strings as valid
>> numbers during a conversion to QObject,
> 
> That would be even crazier!
> 
>>                                         this means our extension
>> of accepting bare keywords for non-finite numbers cannot undergo
>> a round trip (once converted into a string, we never get back to
>> a QFloat).  However, non-finite input is rare enough that it's
>> not worth bothering with at the moment.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> I'm afraid the only sane solution is to find all uses of number in QMP
> output, audit the code producing them, then assert isfinite() in the
> monitor.  For commands without a side effect, we could fail the command
> instead of tripping an assertion.  We'd have to declare such commands.
> 
> Let's examine the occurences of "number" in output of query-qmp-schema,
> or actually in the qmp-introspect.c that gets generated with -u:
> 
> * Object q_obj_migrate_set_downtime-arg member value: input

Even though it's not output, it does need to be checked that it will
behave sanely with Inf or NaN input if we extend our parser to allow
those (behaving sanely may include a graceful error that the input was
out of range).

> 
> * Builtin number: d'uh!
> 
> * Object MigrationStats member mbps: in output of query-migrate
> 
> * Object XBZRLECacheStats member overflow: likewise
> 
> * Object KeyValue case number: not a type.
> 
> * Object BlockDeviceTimedStats members avg_rd_queue_depth,
>   avg_wr_queue_depth: in output of query-blockstats
> 
> * Enum CommandLineParameterType member: not a type
> 
> * Enum JSONType member: not a type
> 
> * Enum KeyValueKind: not a type
> 
> * Object PciBusInfo member: not a type
> 
> So it's just query-migrate and query-blockstats.
> 

Okay, looks like I need to respin this, and the rest of my JSON output
visitor on top of it, with this audit done first.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/4] qobject: Correct JSON lexer grammar comments

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/16/2016 10:19 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Fix the regex comments describing what we parse as JSON.  No change
>>> to the lexer itself, just to the comments:
>>> - The "" and '' string construction was missing alternation between
>>> different escape sequences
>>> - The construction for numbers forgot to handle optional leading '-'
>>> - The construction for numbers was grouped incorrectly so that it
>>> didn't permit '0.1'
>>> - The construction for numbers forgot to mark the exponent as optional
>>> - No mention that our '' string and "\'" are JSON extensions
>>> - No mention of our %d and related extensions when constructing JSON
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> 
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> 
>> I'll take this one through qapi-next.  Thanks!
>> 
>
> You may want to squash this in for shorter lines:
>
>
> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
> index de16219..b030576 100644
> --- a/qobject/json-lexer.c
> +++ b/qobject/json-lexer.c
> @@ -21,16 +21,14 @@
>   * Required by JSON (RFC 7159), plus \' extension in "", and extension
>   * of parsing case-insensitive non-finite numbers like "NaN" and "-Inf":
>   *
> - * \"([^\\\"]|(\\\"|\\'|\\\\|\\/|\\b|\\f|\\n|\\r|\\t|
> - *    \\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
> + * \"([^\\\"]|\\[\"'\\/bfnrt]|\\u[0-9a-fA-F]{4})*\"
>   * -?(0|[1-9][0-9]*)(.[0-9]+)?([eE][-+]?[0-9]+)?
>   * [{}\[\],:]
>   * -?[a-zA-Z]+   # covers null, true, false, nan, inf[inity]
>   *
>   * Extension of '' strings:
>   *
> - * '([^\\']|(\\\"|\\'|\\\\|\\/|\\b|\\f|\\n|\\r|\\t|
> - *    \\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*'
> + * '([^\\']|\\[\"'\\/bfnrt]|\\u[0-9a-fA-F]{4})*'
>   *
>   * Extension for vararg handling in JSON construction:
>   *

Doesn't apply as is, but I get what you mean.  Applied to qapi-next,
please double-check the result there.  Thanks!

Re: [Qemu-devel] [PATCH 3/4] qobject: Parse non-finite numbers, as an extension

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/16/2016 10:25 AM, Markus Armbruster wrote:
>> I think this commit mixes up parsing of non-finite numbers, which we may
>> or may not want, with general test improvements, which we'll want
>> regardless.  Please split the patch.
>> 
>> On the parsing of non-finite numbers: the code looks good to me, but as
>> long as we're not ready to extend QMP to include non-finite numbers both
>> ways, I doubt we need to parse them.
>
> Even if we want to guarantee that we will never generate a non-finite
> number as the result of a QMP command, we DO have to worry about the
> 'id' field.  With all this series applied:
>
> $  ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp
> stdio{"QMP": {"version": {"qemu": {"micro": 50, "minor": 6, "major": 2},
> "package": " (v2.6.0-1151-g1bb9f5e)"}, "capabilities": []}}
> {'execute':'qmp_capabilities', 'id':[nan,inf,-Infinity]}
> {"return": {}, "id": ["nan", "inf", "-inf"]}

You get a different id back, in violation of the QMP spec.

> and with just 1-3 applied:
>
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp
> stdio{"QMP": {"version": {"qemu": {"micro": 50, "minor": 6, "major": 2},
> "package": " (v2.6.0-1151-g1bb9f5e)"}, "capabilities": []}}
> {'execute':'qmp_capabilities', 'id':[nan,inf,-Infinity]}
> {"return": {}, "id": [nan, inf, -inf]}

This would conform to a revision of the QMP spec that supports
non-finite numbers.  With the current spec, it's in "garbage in, garbage
out" territory.  At least it's the same garbage :)

Current master:

    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 6, "major": 2}, "package": " (v2.6.0-1114-g585fcd4)"}, "capabilities": []}}
    {'execute':'qmp_capabilities', 'id':[nan,inf,-Infinity]}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}

Our diagnosis and reporting of lexical errors sucks, but that's not
news.

> [okay, I cheated, as evidenced by the new git stuff appended in the
> version output being the same between the two lines, but you get the idea].
>
> I'm comfortable with a compromise that asserts that QMP commands will
> never output a non-finite number anywhere that introspection lists
> 'number', but that QMP itself understands the extension of parsing a
> bare inf anywhere, and if a bare inf appears under 'id', then the replay
> of 'id' will also include a bare inf (if your JSON library allows you to
> send non-JSON, it should also allow you to parse non-JSON).

Yes.

However, non-finite numbers have never worked in input.  I don't see why
we should make them work in input now, unless we extend QMP to include
non-finite numbers, and make them work both ways.

Re: [Qemu-devel] [PATCH 4/4] qobject: Output valid JSON for non-finite numbers

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 06/16/2016 10:17 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> It's better to give downstream clients a valid JSON string,
>>> even if they are semantically expecting a number, than it is
>>> to give them a bare keyword extension that can cause a
>>> lexical error.
>> 
>> Incompatible change.  If all clients are choking on non-finite numbers,
>> then the incompatibility is an improvement.  If a client exists that
>> groks non-finite numbers, ...  Absence is always hard to show.
>
> The 'id' field is an outlier - there, we replay the user's input with no
> contextual interpretation (however, we DO reserve the right to reorder
> the keys in the dicts that we replay, and to canonicalize UTF-8 text or
> otherwise alter their input to something "equivalent").

Yes, the response's id must the the same JSON value, but it needn't be
the same text.

>> Moreover, it turns query-qmp-schema into a liar: the schema it returns
>> claims a certain member of the reply has "type": "number", and then we
>> go on to send a string anyway.
>
> The 'id' field is documented as sending ANY JSON value, so if we argue
> that canonicalizing their extension input of a bare inf into a proper
> JSON string on output is reasonable, then we may want this patch in
> addition to adding assertions that none of the QMP commands with
> introspectible 'number' ever output non-finite values.

I read this thrice, and I'm still not sure I got the argument :)

>>> Of course, as long as we don't recognize (certain) strings as valid
>>> numbers during a conversion to QObject,
>> 
>> That would be even crazier!
>> 
>>>                                         this means our extension
>>> of accepting bare keywords for non-finite numbers cannot undergo
>>> a round trip (once converted into a string, we never get back to
>>> a QFloat).  However, non-finite input is rare enough that it's
>>> not worth bothering with at the moment.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> 
>> I'm afraid the only sane solution is to find all uses of number in QMP
>> output, audit the code producing them, then assert isfinite() in the
>> monitor.  For commands without a side effect, we could fail the command
>> instead of tripping an assertion.  We'd have to declare such commands.
>> 
>> Let's examine the occurences of "number" in output of query-qmp-schema,
>> or actually in the qmp-introspect.c that gets generated with -u:
>> 
>> * Object q_obj_migrate_set_downtime-arg member value: input
>
> Even though it's not output, it does need to be checked that it will
> behave sanely with Inf or NaN input if we extend our parser to allow
> those (behaving sanely may include a graceful error that the input was
> out of range).

Yes, *if* we extend QMP.

>> 
>> * Builtin number: d'uh!
>> 
>> * Object MigrationStats member mbps: in output of query-migrate
>> 
>> * Object XBZRLECacheStats member overflow: likewise
>> 
>> * Object KeyValue case number: not a type.
>> 
>> * Object BlockDeviceTimedStats members avg_rd_queue_depth,
>>   avg_wr_queue_depth: in output of query-blockstats
>> 
>> * Enum CommandLineParameterType member: not a type
>> 
>> * Enum JSONType member: not a type
>> 
>> * Enum KeyValueKind: not a type
>> 
>> * Object PciBusInfo member: not a type
>> 
>> So it's just query-migrate and query-blockstats.
>> 
>
> Okay, looks like I need to respin this, and the rest of my JSON output
> visitor on top of it, with this audit done first.

Audit, plus isfinite() assertions to guard the JSON output.

The (misnamed) QMP output visitor shouldn't assert, because it can
legitimately be used for purposes other than QMP.  Only the actual
conversion to JSON should assert.  Currently, to_json().  With your JSON
output visitor, it would be qstring_append_json_number(), or its caller.

Re: [Qemu-devel] [PATCH 1/4] qobject: Correct JSON lexer grammar comments

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

On 06/17/2016 01:54 AM, Markus Armbruster wrote:

>>>
>>> I'll take this one through qapi-next.  Thanks!
>>>
>>
>> You may want to squash this in for shorter lines:
>>
>>
>> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
>> index de16219..b030576 100644
>> --- a/qobject/json-lexer.c
>> +++ b/qobject/json-lexer.c
>> @@ -21,16 +21,14 @@
>>   * Required by JSON (RFC 7159), plus \' extension in "", and extension
>>   * of parsing case-insensitive non-finite numbers like "NaN" and "-Inf":
>>   *
>> - * \"([^\\\"]|(\\\"|\\'|\\\\|\\/|\\b|\\f|\\n|\\r|\\t|
>> - *    \\u[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]))*\"
>> + * \"([^\\\"]|\\[\"'\\/bfnrt]|\\u[0-9a-fA-F]{4})*\"

> Doesn't apply as is, but I get what you mean.  Applied to qapi-next,
> please double-check the result there.  Thanks!

You lost the comment about 'plus \' extension in ""', which is probably
still worth keeping; but otherwise looks okay.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]