escaped_string() first tests double quoted strings, then repeats a few
tests with single quotes.  Repeat all of them: store the strings to
test without quotes, and wrap them in either kind of quote for
testing.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qjson.c | 94 ++++++++++++++++++++++++++-------------------
 1 file changed, 55 insertions(+), 39 deletions(-)

diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 0a9a054c7b..1c7f24bc4d 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -22,55 +22,71 @@
 #include "qapi/qmp/qstring.h"
 #include "qemu-common.h"
 
+static QString *from_json_str(const char *jstr, Error **errp, bool single)
+{
+    char quote = single ? '\'' : '"';
+    char *qjstr = g_strdup_printf("%c%s%c", quote, jstr, quote);
+
+    return qobject_to(QString, qobject_from_json(qjstr, errp));
+}
+
+static char *to_json_str(QString *str)
+{
+    QString *json = qobject_to_json(QOBJECT(str));
+    char *jstr;
+
+    if (!json) {
+        return NULL;
+    }
+    /* peel off double quotes */
+    jstr = g_strndup(qstring_get_str(json) + 1,
+                     qstring_get_length(json) - 2);
+    qobject_unref(json);
+    return jstr;
+}
+
 static void escaped_string(void)
 {
-    int i;
     struct {
-        const char *encoded;
-        const char *decoded;
+        /* Content of JSON string to parse with qobject_from_json() */
+        const char *json_in;
+        /* Expected parse output; to unparse with qobject_to_json() */
+        const char *utf8_out;
         int skip;
     } test_cases[] = {
-        { "\"\\b\"", "\b" },
-        { "\"\\f\"", "\f" },
-        { "\"\\n\"", "\n" },
-        { "\"\\r\"", "\r" },
-        { "\"\\t\"", "\t" },
-        { "\"/\"", "/" },
-        { "\"\\/\"", "/", .skip = 1 },
-        { "\"\\\\\"", "\\" },
-        { "\"\\\"\"", "\"" },
-        { "\"hello world \\\"embedded string\\\"\"",
+        { "\\b", "\b" },
+        { "\\f", "\f" },
+        { "\\n", "\n" },
+        { "\\r", "\r" },
+        { "\\t", "\t" },
+        { "/", "/" },
+        { "\\/", "/", .skip = 1 },
+        { "\\\\", "\\" },
+        { "\\\"", "\"" },
+        { "hello world \\\"embedded string\\\"",
           "hello world \"embedded string\"" },
-        { "\"hello world\\nwith new line\"", "hello world\nwith new line" },
-        { "\"single byte utf-8 \\u0020\"", "single byte utf-8  ", .skip = 1 },
-        { "\"double byte utf-8 \\u00A2\"", "double byte utf-8 \xc2\xa2" },
-        { "\"triple byte utf-8 \\u20AC\"", "triple byte utf-8 \xe2\x82\xac" },
-        { "'\\b'", "\b", .skip = 1 },
-        { "'\\f'", "\f", .skip = 1 },
-        { "'\\n'", "\n", .skip = 1 },
-        { "'\\r'", "\r", .skip = 1 },
-        { "'\\t'", "\t", .skip = 1 },
-        { "'\\/'", "/", .skip = 1 },
-        { "'\\\\'", "\\", .skip = 1 },
+        { "hello world\\nwith new line", "hello world\nwith new line" },
+        { "single byte utf-8 \\u0020", "single byte utf-8  ", .skip = 1 },
+        { "double byte utf-8 \\u00A2", "double byte utf-8 \xc2\xa2" },
+        { "triple byte utf-8 \\u20AC", "triple byte utf-8 \xe2\x82\xac" },
         {}
     };
+    int i, j;
+    QString *cstr;
+    char *jstr;
 
-    for (i = 0; test_cases[i].encoded; i++) {
-        QObject *obj;
-        QString *str;
-
-        obj = qobject_from_json(test_cases[i].encoded, &error_abort);
-        str = qobject_to(QString, obj);
-        g_assert(str);
-        g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].decoded);
-
-        if (test_cases[i].skip == 0) {
-            str = qobject_to_json(obj);
-            g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].encoded);
-            qobject_unref(obj);
+    for (i = 0; test_cases[i].json_in; i++) {
+        for (j = 0; j < 2; j++) {
+            cstr = from_json_str(test_cases[i].json_in, &error_abort, j);
+            g_assert_cmpstr(qstring_get_try_str(cstr),
+                            ==, test_cases[i].utf8_out);
+            if (test_cases[i].skip == 0) {
+                jstr = to_json_str(cstr);
+                g_assert_cmpstr(jstr, ==, test_cases[i].json_in);
+                g_free(jstr);
+            }
+            qobject_unref(cstr);
         }
-
-        qobject_unref(str);
     }
 }
 
-- 
2.17.1

On 08/08/2018 07:02 AM, Markus Armbruster wrote:
> escaped_string() first tests double quoted strings, then repeats a few
> tests with single quotes.  Repeat all of them: store the strings to
> test without quotes, and wrap them in either kind of quote for
> testing.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/check-qjson.c | 94 ++++++++++++++++++++++++++-------------------
>   1 file changed, 55 insertions(+), 39 deletions(-)
> 

>       struct {
> -        const char *encoded;
> -        const char *decoded;
> +        /* Content of JSON string to parse with qobject_from_json() */
> +        const char *json_in;
> +        /* Expected parse output; to unparse with qobject_to_json() */
> +        const char *utf8_out;
>           int skip;

Instead of int skip (and why is that not a bool?), would it be better to 
have an optional const char *json_out?

> +    for (i = 0; test_cases[i].json_in; i++) {
> +        for (j = 0; j < 2; j++) {
> +            cstr = from_json_str(test_cases[i].json_in, &error_abort, j);
> +            g_assert_cmpstr(qstring_get_try_str(cstr),
> +                            ==, test_cases[i].utf8_out);
> +            if (test_cases[i].skip == 0) {
> +                jstr = to_json_str(cstr);
> +                g_assert_cmpstr(jstr, ==, test_cases[i].json_in);
> +                g_free(jstr);

and here, write g_assert_cmpstr(jstr, ==, test_cases[i].json_out ?: 
test_cases[i].json_in)?  After all, the reason we're skipping is because 
there are some cases of multiple inputs that get canonicalized to 
constant output, such as " " vs. "\u0020", or "\\'" vs. "'".

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 08/08/2018 07:02 AM, Markus Armbruster wrote:
>> escaped_string() first tests double quoted strings, then repeats a few
>> tests with single quotes.  Repeat all of them: store the strings to
>> test without quotes, and wrap them in either kind of quote for
>> testing.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/check-qjson.c | 94 ++++++++++++++++++++++++++-------------------
>>   1 file changed, 55 insertions(+), 39 deletions(-)
>>
>
>>       struct {
>> -        const char *encoded;
>> -        const char *decoded;
>> +        /* Content of JSON string to parse with qobject_from_json() */
>> +        const char *json_in;
>> +        /* Expected parse output; to unparse with qobject_to_json() */
>> +        const char *utf8_out;
>>           int skip;
>
> Instead of int skip (and why is that not a bool?),

Ask Anthony ;)

>                                                    would it be better
> to have an optional const char *json_out?
>
>> +    for (i = 0; test_cases[i].json_in; i++) {
>> +        for (j = 0; j < 2; j++) {
>> +            cstr = from_json_str(test_cases[i].json_in, &error_abort, j);
>> +            g_assert_cmpstr(qstring_get_try_str(cstr),
>> +                            ==, test_cases[i].utf8_out);
>> +            if (test_cases[i].skip == 0) {
>> +                jstr = to_json_str(cstr);
>> +                g_assert_cmpstr(jstr, ==, test_cases[i].json_in);
>> +                g_free(jstr);
>
> and here, write g_assert_cmpstr(jstr, ==, test_cases[i].json_out ?:
> test_cases[i].json_in)?  After all, the reason we're skipping is
> because there are some cases of multiple inputs that get canonicalized
> to constant output, such as " " vs. "\u0020", or "\\'" vs. "'".

This would additionally test that qobject_from_json() correctly maps '/'
and ' ' to themselves.  Marginal.  If we want it, then comparing jstr to
cstr if skip would do.  Dunno.

On 08/08/2018 07:02 AM, Markus Armbruster wrote:
> escaped_string() first tests double quoted strings, then repeats a few
> tests with single quotes.  Repeat all of them: store the strings to
> test without quotes, and wrap them in either kind of quote for
> testing.

Does that properly cover the fact that "'" and '"' are valid, but the 
counterparts need escaping?

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/check-qjson.c | 94 ++++++++++++++++++++++++++-------------------
>   1 file changed, 55 insertions(+), 39 deletions(-)
> 
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> index 0a9a054c7b..1c7f24bc4d 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -22,55 +22,71 @@
>   #include "qapi/qmp/qstring.h"
>   #include "qemu-common.h"
>   
> +static QString *from_json_str(const char *jstr, Error **errp, bool single)
> +{
> +    char quote = single ? '\'' : '"';
> +    char *qjstr = g_strdup_printf("%c%s%c", quote, jstr, quote);
> +
> +    return qobject_to(QString, qobject_from_json(qjstr, errp));

Memory leak of qjstr.

> +}
> +
> +static char *to_json_str(QString *str)
> +{
> +    QString *json = qobject_to_json(QOBJECT(str));
> +    char *jstr;
> +
> +    if (!json) {
> +        return NULL;
> +    }
> +    /* peel off double quotes */
> +    jstr = g_strndup(qstring_get_str(json) + 1,
> +                     qstring_get_length(json) - 2);
> +    qobject_unref(json);
> +    return jstr;
> +}
> +
>   static void escaped_string(void)
>   {
> -    int i;
>       struct {
> -        const char *encoded;
> -        const char *decoded;
> +        /* Content of JSON string to parse with qobject_from_json() */
> +        const char *json_in;
> +        /* Expected parse output; to unparse with qobject_to_json() */
> +        const char *utf8_out;
>           int skip;
>       } test_cases[] = {

> +        { "\\\"", "\"" },

This covers the escaped version of ", but not of ', and not the 
unescaped version of either (per my comment above, the latter can only 
be done with the opposite quoting).

Otherwise looks sane.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 08/08/2018 07:02 AM, Markus Armbruster wrote:
>> escaped_string() first tests double quoted strings, then repeats a few
>> tests with single quotes.  Repeat all of them: store the strings to
>> test without quotes, and wrap them in either kind of quote for
>> testing.
>
> Does that properly cover the fact that "'" and '"' are valid, but the
> counterparts need escaping?
>
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/check-qjson.c | 94 ++++++++++++++++++++++++++-------------------
>>   1 file changed, 55 insertions(+), 39 deletions(-)
>>
>> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
>> index 0a9a054c7b..1c7f24bc4d 100644
>> --- a/tests/check-qjson.c
>> +++ b/tests/check-qjson.c
>> @@ -22,55 +22,71 @@
>>   #include "qapi/qmp/qstring.h"
>>   #include "qemu-common.h"
>>   +static QString *from_json_str(const char *jstr, Error **errp,
>> bool single)
>> +{
>> +    char quote = single ? '\'' : '"';
>> +    char *qjstr = g_strdup_printf("%c%s%c", quote, jstr, quote);
>> +
>> +    return qobject_to(QString, qobject_from_json(qjstr, errp));
>
> Memory leak of qjstr.

Fixing.

>> +}
>> +
>> +static char *to_json_str(QString *str)
>> +{
>> +    QString *json = qobject_to_json(QOBJECT(str));
>> +    char *jstr;
>> +
>> +    if (!json) {
>> +        return NULL;
>> +    }
>> +    /* peel off double quotes */
>> +    jstr = g_strndup(qstring_get_str(json) + 1,
>> +                     qstring_get_length(json) - 2);
>> +    qobject_unref(json);
>> +    return jstr;
>> +}
>> +
>>   static void escaped_string(void)
>>   {
>> -    int i;
>>       struct {
>> -        const char *encoded;
>> -        const char *decoded;
>> +        /* Content of JSON string to parse with qobject_from_json() */
>> +        const char *json_in;
>> +        /* Expected parse output; to unparse with qobject_to_json() */
>> +        const char *utf8_out;
>>           int skip;
>>       } test_cases[] = {
>
>> +        { "\\\"", "\"" },
>
> This covers the escaped version of ", but not of ', and not the
> unescaped version of either (per my comment above, the latter can only
> be done with the opposite quoting).

escaped_string() is about testing \-escapes.  Unescaped quotes are
covered by simple_string() and single_quote_string().

However, I drop both in PATCH 10.  That's actually a bad idea.

>> Otherwise looks sane.

Thanks!