Re: [Qemu-devel] [PATCH v3 03/18] qapi: Factor out JSON string escaping

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Pull out a new qstring_append_json_string() helper, so that all
> JSON output producers can use the same output escaping rules.
>
> While it appears that vmstate's use of the simpler qjson.c
> formatter is not currently encountering any string that needs
> escapes to be valid JSON, it is better to be safe than sorry.
>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Fam Zheng <address@hidden>
>
> ---
> v3: rebase to include cleanups in master
> v2: no change
> ---
>  include/qapi/qmp/qstring.h |  1 +
>  qjson.c                    |  9 +++----
>  qobject/qobject-json.c     | 59 ++-------------------------------------------
>  qobject/qstring.c          | 60 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 66 insertions(+), 63 deletions(-)
>
> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index 10076b7..a254ee3 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -30,6 +30,7 @@ const char *qstring_get_str(const QString *qstring);
>  void qstring_append_int(QString *qstring, int64_t value);
>  void qstring_append(QString *qstring, const char *str);
>  void qstring_append_chr(QString *qstring, int c);
> +void qstring_append_json_string(QString *qstring, const char *raw);
>  QString *qobject_to_qstring(const QObject *obj);
>  void qstring_destroy_obj(QObject *obj);
>
> diff --git a/qjson.c b/qjson.c
> index b65ca6e..b9a9a36 100644
> --- a/qjson.c
> +++ b/qjson.c
> @@ -36,9 +36,8 @@ static void json_emit_element(QJSON *json, const char *name)
>      }
>
>      if (name) {
> -        qstring_append(json->str, "\"");
> -        qstring_append(json->str, name);
> -        qstring_append(json->str, "\" : ");
> +        qstring_append_json_string(json->str, name);
> +        qstring_append(json->str, " : ");
>      }
>  }
>
> @@ -77,9 +76,7 @@ void json_prop_int(QJSON *json, const char *name, int64_t 
> val)
>  void json_prop_str(QJSON *json, const char *name, const char *str)
>  {
>      json_emit_element(json, name);
> -    qstring_append_chr(json->str, '"');
> -    qstring_append(json->str, str);
> -    qstring_append_chr(json->str, '"');
> +    qstring_append_json_string(json->str, str);
>  }
>
>  const char *qjson_get_str(QJSON *json)
> diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c
> index 24e7d80..6fed1ee 100644
> --- a/qobject/qobject-json.c
> +++ b/qobject/qobject-json.c
> @@ -16,7 +16,6 @@
>  #include "qapi/qmp/json-parser.h"
>  #include "qapi/qmp/json-streamer.h"
>  #include "qapi/qmp/qobject-json.h"
> -#include "qemu/unicode.h"
>  #include "qapi/qmp/types.h"
>
>  typedef struct JSONParsingState
> @@ -81,7 +80,6 @@ static void to_json(const QObject *obj, QString *str, int 
> pretty, int indent);
>  static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
>  {
>      ToJsonIterState *s = opaque;
> -    QString *qkey;
>      int j;
>
>      if (s->count) {
> @@ -94,9 +92,7 @@ static void to_json_dict_iter(const char *key, QObject 
> *obj, void *opaque)
>              qstring_append(s->str, "    ");
>      }
>
> -    qkey = qstring_from_str(key);
> -    to_json(QOBJECT(qkey), s->str, s->pretty, s->indent);
> -    QDECREF(qkey);
> +    qstring_append_json_string(s->str, key);
>
>      qstring_append(s->str, ": ");
>      to_json(obj, s->str, s->pretty, s->indent);
> @@ -138,58 +134,7 @@ static void to_json(const QObject *obj, QString *str, 
> int pretty, int indent)
>      }
>      case QTYPE_QSTRING: {
>          QString *val = qobject_to_qstring(obj);
> -        const char *ptr;
> -        int cp;
> -        char buf[16];
> -        char *end;
> -
> -        ptr = qstring_get_str(val);
> -        qstring_append(str, "\"");
> -
> -        for (; *ptr; ptr = end) {
> -            cp = mod_utf8_codepoint(ptr, 6, &end);
> -            switch (cp) {
> -            case '\"':
> -                qstring_append(str, "\\\"");
> -                break;
> -            case '\\':
> -                qstring_append(str, "\\\\");
> -                break;
> -            case '\b':
> -                qstring_append(str, "\\b");
> -                break;
> -            case '\f':
> -                qstring_append(str, "\\f");
> -                break;
> -            case '\n':
> -                qstring_append(str, "\\n");
> -                break;
> -            case '\r':
> -                qstring_append(str, "\\r");
> -                break;
> -            case '\t':
> -                qstring_append(str, "\\t");
> -                break;
> -            default:
> -                if (cp < 0) {
> -                    cp = 0xFFFD; /* replacement character */
> -                }
> -                if (cp > 0xFFFF) {
> -                    /* beyond BMP; need a surrogate pair */
> -                    snprintf(buf, sizeof(buf), "\\u%04X\\u%04X",
> -                             0xD800 + ((cp - 0x10000) >> 10),
> -                             0xDC00 + ((cp - 0x10000) & 0x3FF));
> -                } else if (cp < 0x20 || cp >= 0x7F) {
> -                    snprintf(buf, sizeof(buf), "\\u%04X", cp);
> -                } else {
> -                    buf[0] = cp;
> -                    buf[1] = 0;
> -                }
> -                qstring_append(str, buf);
> -            }
> -        };
> -
> -        qstring_append(str, "\"");
> +        qstring_append_json_string(str, qstring_get_str(val));
>          break;
>      }
>      case QTYPE_QDICT: {
> diff --git a/qobject/qstring.c b/qobject/qstring.c
> index 5da7b5f..9f0df5b 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -14,6 +14,7 @@
>  #include "qapi/qmp/qobject.h"
>  #include "qapi/qmp/qstring.h"
>  #include "qemu-common.h"
> +#include "qemu/unicode.h"
>
>  /**
>   * qstring_new(): Create a new empty QString
> @@ -107,6 +108,65 @@ void qstring_append_chr(QString *qstring, int c)
>  }
>
>  /**
> + * qstring_append_json_string(): Append a raw string to a QString, while
> + * adding outer "" and JSON escape sequences.
> + */
> +void qstring_append_json_string(QString *qstring, const char *raw)
> +{
> +    const char *ptr = raw;
> +    int cp;
> +    char buf[16];
> +    char *end;
> +
> +    qstring_append(qstring, "\"");
> +
> +    for (; *ptr; ptr = end) {

Make that

       for (ptr = raw; *ptr; ptr = end) {

and drop ptr's initializer.

> +        cp = mod_utf8_codepoint(ptr, 6, &end);
> +        switch (cp) {
> +        case '\"':
> +            qstring_append(qstring, "\\\"");
> +            break;
> +        case '\\':
> +            qstring_append(qstring, "\\\\");
> +            break;
> +        case '\b':
> +            qstring_append(qstring, "\\b");
> +            break;
> +        case '\f':
> +            qstring_append(qstring, "\\f");
> +            break;
> +        case '\n':
> +            qstring_append(qstring, "\\n");
> +            break;
> +        case '\r':
> +            qstring_append(qstring, "\\r");
> +            break;
> +        case '\t':
> +            qstring_append(qstring, "\\t");
> +            break;
> +        default:
> +            if (cp < 0) {
> +                cp = 0xFFFD; /* replacement character */
> +            }
> +            if (cp > 0xFFFF) {
> +                /* beyond BMP; need a surrogate pair */
> +                snprintf(buf, sizeof(buf), "\\u%04X\\u%04X",
> +                         0xD800 + ((cp - 0x10000) >> 10),
> +                         0xDC00 + ((cp - 0x10000) & 0x3FF));
> +            } else if (cp < 0x20 || cp >= 0x7F) {
> +                snprintf(buf, sizeof(buf), "\\u%04X", cp);
> +            } else {
> +                buf[0] = cp;
> +                buf[1] = 0;
> +            }
> +            qstring_append(qstring, buf);
> +        }
> +    };
> +
> +    qstring_append(qstring, "\"");
> +}

I think this belongs to qobject-json.c, because it's very much about
JSON (it encapsulates knowledge on JSON string escaping), and a mere
user of QString (it knows nothing about QString's implementation).

Precedence: qobject_from_json() & friends are there, not in qobject.c.

Since qjson.c doesn't already use the space-wasting "start the function
comment with the function's name" style, let's drop that waste of screen
space and simply write

   /*
    * Append a JSON string to @qstring that encodes the C string @str.
    * The JSON string is enclosed in double quotes and has funny
    * characters escaped.
    */

and rename raw to str.

> +
> +/**
>   * qobject_to_qstring(): Convert a QObject to a QString
>   */
>  QString *qobject_to_qstring(const QObject *obj)