Re: [Qemu-devel] [PATCH 03/26] qboject: add literal qobject type

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

Hi

On Wed, Aug 16, 2017 at 10:59 AM, Markus Armbruster <armbru@redhat.com> wrote:
> In the subject: s/qboject: add/qobject: Add/
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Promote LiteralQObject from tests/check-qjson.c to qobject/qlit.c,
>> allowing to statically declare complex qobjects.
>>
>> Add a helper qobject_from_qlit() to instantiate a literal qobject to a
>> real QObject. Add some simple test.
>
> Suggest
>
> * to also move the existing function making use of LiteralQObject
>   compare_litqobj_to_qobj(), so other tests can use it to verify a
>   QObject matches expectations, and
>
> * to split the patch into the move and the addition of
>   qobject_from_qlit().
>

ok

>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  include/qapi/qmp/qlit.h | 53 ++++++++++++++++++++++++++++++++++++
>>  qobject/qlit.c          | 53 ++++++++++++++++++++++++++++++++++++
>>  tests/check-qjson.c     | 72 +++++++++++++++++--------------------------------
>>  tests/check-qlit.c      | 52 +++++++++++++++++++++++++++++++++++
>>  qobject/Makefile.objs   |  2 +-
>>  tests/Makefile.include  |  5 +++-
>>  6 files changed, 187 insertions(+), 50 deletions(-)
>>  create mode 100644 include/qapi/qmp/qlit.h
>>  create mode 100644 qobject/qlit.c
>>  create mode 100644 tests/check-qlit.c
>>
>> diff --git a/include/qapi/qmp/qlit.h b/include/qapi/qmp/qlit.h
>> new file mode 100644
>> index 0000000000..fd6bfc3e69
>> --- /dev/null
>> +++ b/include/qapi/qmp/qlit.h
>> @@ -0,0 +1,53 @@
>> +/*
>> + * Copyright IBM, Corp. 2009
>> + * Copyright (c) 2013, 2015 Red Hat Inc.
>> + *
>> + * Authors:
>> + *  Anthony Liguori   <aliguori@us.ibm.com>
>> + *  Markus Armbruster <armbru@redhat.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + *
>> + */
>> +#ifndef QLIT_H_
>> +#define QLIT_H_
>> +
>> +#include "qapi-types.h"
>> +#include "qobject.h"
>> +
>> +typedef struct QLitDictEntry QLitDictEntry;
>> +typedef struct QLitObject QLitObject;
>> +
>> +struct QLitObject {
>> +    int type;
>> +    union {
>> +        bool qbool;
>> +        int64_t qnum;
>
> Not this patch's fault: QLitObject restricts numbers to signed integers.
>
>> +        const char *qstr;
>> +        QLitDictEntry *qdict;
>> +        QLitObject *qlist;
>> +    } value;
>> +};
>> +
>> +struct QLitDictEntry {
>> +    const char *key;
>> +    QLitObject value;
>> +};
>> +
>> +#define QLIT_QNULL \
>> +    { .type = QTYPE_QNULL }
>> +#define QLIT_QBOOL(val) \
>> +    { .type = QTYPE_QBOOL, .value.qbool = (val) }
>> +#define QLIT_QNUM(val) \
>> +    { .type = QTYPE_QNUM, .value.qnum = (val) }
>> +#define QLIT_QSTR(val) \
>> +    { .type = QTYPE_QSTRING, .value.qstr = (val) }
>> +#define QLIT_QDICT(val) \
>> +    { .type = QTYPE_QDICT, .value.qdict = (val) }
>> +#define QLIT_QLIST(val) \
>> +    { .type = QTYPE_QLIST, .value.qlist = (val) }
>> +
>> +QObject *qobject_from_qlit(const QLitObject *qlit);
>> +
>> +#endif /* QLIT_H_ */
>> diff --git a/qobject/qlit.c b/qobject/qlit.c
>> new file mode 100644
>> index 0000000000..d7407b4b34
>> --- /dev/null
>> +++ b/qobject/qlit.c
>> @@ -0,0 +1,53 @@
>> +/*
>> + * QLit literal qobject
>> + *
>> + * Copyright (C) 2017 Red Hat Inc.
>> + *
>> + * Authors:
>> + *  Marc-André Lureau <marcandre.lureau@redhat.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +
>> +#include "qapi/qmp/qlit.h"
>> +#include "qapi/qmp/types.h"
>> +
>> +QObject *qobject_from_qlit(const QLitObject *qlit)
>> +{
>> +    int i;
>> +
>> +    switch (qlit->type) {
>> +    case QTYPE_QNULL:
>> +        return QOBJECT(qnull());
>> +    case QTYPE_QNUM:
>> +        return QOBJECT(qnum_from_int(qlit->value.qnum));
>> +    case QTYPE_QSTRING:
>> +        return QOBJECT(qstring_from_str(qlit->value.qstr));
>> +    case QTYPE_QDICT: {
>> +        QDict *qdict = qdict_new();
>> +        for (i = 0; qlit->value.qdict[i].value.type != QTYPE_NONE; i++) {
>> +            QLitDictEntry *e = &qlit->value.qdict[i];
>> +
>> +            qdict_put_obj(qdict, e->key, qobject_from_qlit(&e->value));
>> +        }
>> +        return QOBJECT(qdict);
>> +    }
>> +    case QTYPE_QLIST: {
>> +        QList *qlist = qlist_new();
>> +
>> +        for (i = 0; qlit->value.qlist[i].type != QTYPE_NONE; i++) {
>> +            qlist_append_obj(qlist, qobject_from_qlit(&qlit->value.qlist[i]));
>> +        }
>> +        return QOBJECT(qlist);
>> +    }
>> +    case QTYPE_QBOOL:
>> +        return QOBJECT(qbool_from_bool(qlit->value.qbool));
>> +    case QTYPE_NONE:
>> +        assert(0);
>> +    }
>> +
>> +    return NULL;
>> +}
>> diff --git a/tests/check-qjson.c b/tests/check-qjson.c
>> index 9c42a46b7d..1a95aff2ba 100644
>> --- a/tests/check-qjson.c
>> +++ b/tests/check-qjson.c
>> @@ -16,6 +16,7 @@
>>  #include "qapi/error.h"
>>  #include "qapi/qmp/types.h"
>>  #include "qapi/qmp/qjson.h"
>> +#include "qapi/qmp/qlit.h"
>>  #include "qemu-common.h"
>>
>>  static void escaped_string(void)
>> @@ -1059,39 +1060,14 @@ static void keyword_literal(void)
>>      QDECREF(null);
>>  }
>>
>> -typedef struct LiteralQDictEntry LiteralQDictEntry;
>> -typedef struct LiteralQObject LiteralQObject;
>> -
>> -struct LiteralQObject
>> -{
>> -    int type;
>> -    union {
>> -        int64_t qnum;
>> -        const char *qstr;
>> -        LiteralQDictEntry *qdict;
>> -        LiteralQObject *qlist;
>> -    } value;
>> -};
>> -
>> -struct LiteralQDictEntry
>> -{
>> -    const char *key;
>> -    LiteralQObject value;
>> -};
>> -
>> -#define QLIT_QNUM(val) (LiteralQObject){.type = QTYPE_QNUM, .value.qnum = (val)}
>> -#define QLIT_QSTR(val) (LiteralQObject){.type = QTYPE_QSTRING, .value.qstr = (val)}
>> -#define QLIT_QDICT(val) (LiteralQObject){.type = QTYPE_QDICT, .value.qdict = (val)}
>> -#define QLIT_QLIST(val) (LiteralQObject){.type = QTYPE_QLIST, .value.qlist = (val)}
>> -
>>  typedef struct QListCompareHelper
>>  {
>>      int index;
>> -    LiteralQObject *objs;
>> +    QLitObject *objs;
>>      int result;
>>  } QListCompareHelper;
>>
>> -static int compare_litqobj_to_qobj(LiteralQObject *lhs, QObject *rhs);
>> +static int compare_litqobj_to_qobj(QLitObject *lhs, QObject *rhs);
>>
>>  static void compare_helper(QObject *obj, void *opaque)
>>  {
>> @@ -1109,7 +1085,7 @@ static void compare_helper(QObject *obj, void *opaque)
>>      helper->result = compare_litqobj_to_qobj(&helper->objs[helper->index++], obj);
>>  }
>>
>> -static int compare_litqobj_to_qobj(LiteralQObject *lhs, QObject *rhs)
>> +static int compare_litqobj_to_qobj(QLitObject *lhs, QObject *rhs)
>>  {
>>      int64_t val;
>>
>> @@ -1159,23 +1135,23 @@ static void simple_dict(void)
>>      int i;
>>      struct {
>>          const char *encoded;
>> -        LiteralQObject decoded;
>> +        QLitObject decoded;
>>      } test_cases[] = {
>>          {
>>              .encoded = "{\"foo\": 42, \"bar\": \"hello world\"}",
>> -            .decoded = QLIT_QDICT(((LiteralQDictEntry[]){
>> +            .decoded = QLIT_QDICT(((QLitDictEntry[]){
>>                          { "foo", QLIT_QNUM(42) },
>>                          { "bar", QLIT_QSTR("hello world") },
>>                          { }
>>                      })),
>>          }, {
>>              .encoded = "{}",
>> -            .decoded = QLIT_QDICT(((LiteralQDictEntry[]){
>> +            .decoded = QLIT_QDICT(((QLitDictEntry[]){
>>                          { }
>>                      })),
>>          }, {
>>              .encoded = "{\"foo\": 43}",
>> -            .decoded = QLIT_QDICT(((LiteralQDictEntry[]){
>> +            .decoded = QLIT_QDICT(((QLitDictEntry[]){
>>                          { "foo", QLIT_QNUM(43) },
>>                          { }
>>                      })),
>> @@ -1257,11 +1233,11 @@ static void simple_list(void)
>>      int i;
>>      struct {
>>          const char *encoded;
>> -        LiteralQObject decoded;
>> +        QLitObject decoded;
>>      } test_cases[] = {
>>          {
>>              .encoded = "[43,42]",
>> -            .decoded = QLIT_QLIST(((LiteralQObject[]){
>> +            .decoded = QLIT_QLIST(((QLitObject[]){
>>                          QLIT_QNUM(43),
>>                          QLIT_QNUM(42),
>>                          { }
>> @@ -1269,21 +1245,21 @@ static void simple_list(void)
>>          },
>>          {
>>              .encoded = "[43]",
>> -            .decoded = QLIT_QLIST(((LiteralQObject[]){
>> +            .decoded = QLIT_QLIST(((QLitObject[]){
>>                          QLIT_QNUM(43),
>>                          { }
>>                      })),
>>          },
>>          {
>>              .encoded = "[]",
>> -            .decoded = QLIT_QLIST(((LiteralQObject[]){
>> +            .decoded = QLIT_QLIST(((QLitObject[]){
>>                          { }
>>                      })),
>>          },
>>          {
>>              .encoded = "[{}]",
>> -            .decoded = QLIT_QLIST(((LiteralQObject[]){
>> -                        QLIT_QDICT(((LiteralQDictEntry[]){
>> +            .decoded = QLIT_QLIST(((QLitObject[]){
>> +                        QLIT_QDICT(((QLitDictEntry[]){
>>                                      {},
>>                                          })),
>>                          {},
>> @@ -1314,11 +1290,11 @@ static void simple_whitespace(void)
>>      int i;
>>      struct {
>>          const char *encoded;
>> -        LiteralQObject decoded;
>> +        QLitObject decoded;
>>      } test_cases[] = {
>>          {
>>              .encoded = " [ 43 , 42 ]",
>> -            .decoded = QLIT_QLIST(((LiteralQObject[]){
>> +            .decoded = QLIT_QLIST(((QLitObject[]){
>>                          QLIT_QNUM(43),
>>                          QLIT_QNUM(42),
>>                          { }
>> @@ -1326,12 +1302,12 @@ static void simple_whitespace(void)
>>          },
>>          {
>>              .encoded = " [ 43 , { 'h' : 'b' }, [ ], 42 ]",
>> -            .decoded = QLIT_QLIST(((LiteralQObject[]){
>> +            .decoded = QLIT_QLIST(((QLitObject[]){
>>                          QLIT_QNUM(43),
>> -                        QLIT_QDICT(((LiteralQDictEntry[]){
>> +                        QLIT_QDICT(((QLitDictEntry[]){
>>                                      { "h", QLIT_QSTR("b") },
>>                                      { }})),
>> -                        QLIT_QLIST(((LiteralQObject[]){
>> +                        QLIT_QLIST(((QLitObject[]){
>>                                      { }})),
>>                          QLIT_QNUM(42),
>>                          { }
>> @@ -1339,13 +1315,13 @@ static void simple_whitespace(void)
>>          },
>>          {
>>              .encoded = " [ 43 , { 'h' : 'b' , 'a' : 32 }, [ ], 42 ]",
>> -            .decoded = QLIT_QLIST(((LiteralQObject[]){
>> +            .decoded = QLIT_QLIST(((QLitObject[]){
>>                          QLIT_QNUM(43),
>> -                        QLIT_QDICT(((LiteralQDictEntry[]){
>> +                        QLIT_QDICT(((QLitDictEntry[]){
>>                                      { "h", QLIT_QSTR("b") },
>>                                      { "a", QLIT_QNUM(32) },
>>                                      { }})),
>> -                        QLIT_QLIST(((LiteralQObject[]){
>> +                        QLIT_QLIST(((QLitObject[]){
>>                                      { }})),
>>                          QLIT_QNUM(42),
>>                          { }
>> @@ -1393,10 +1369,10 @@ static void simple_varargs(void)
>>  {
>>      QObject *embedded_obj;
>>      QObject *obj;
>> -    LiteralQObject decoded = QLIT_QLIST(((LiteralQObject[]){
>> +    QLitObject decoded = QLIT_QLIST(((QLitObject[]){
>>              QLIT_QNUM(1),
>>              QLIT_QNUM(2),
>> -            QLIT_QLIST(((LiteralQObject[]){
>> +            QLIT_QLIST(((QLitObject[]){
>>                          QLIT_QNUM(32),
>>                          QLIT_QNUM(42),
>>                          {}})),
>> diff --git a/tests/check-qlit.c b/tests/check-qlit.c
>> new file mode 100644
>> index 0000000000..cda4620f92
>> --- /dev/null
>> +++ b/tests/check-qlit.c
>> @@ -0,0 +1,52 @@
>> +/*
>> + * QLit unit-tests.
>> + *
>> + * Copyright (C) 2017 Red Hat Inc.
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +
>> +#include "qapi/qmp/qlit.h"
>> +
>> +static void qobject_from_qlit_test(void)
>> +{
>> +    char *str;
>> +    QObject *qobj = NULL;
>> +    QLitObject qlit = QLIT_QDICT((
>> +        (QLitDictEntry[]) {
>> +            { "foo", QLIT_QNUM(42) },
>> +            { "bar", QLIT_QSTR("hello world") },
>> +            { "baz", QLIT_QNULL },
>> +            { "bee", QLIT_QLIST((
>> +                (QLitObject[]) {
>> +                    QLIT_QNUM(43),
>> +                    QLIT_QNUM(44),
>> +                    QLIT_QBOOL(true),
>> +                    { },
>> +                }))
>> +            },
>> +            { },
>> +        }));
>> +
>> +    qobj = qobject_from_qlit(&qlit);
>> +
>> +    str = qobject_to_string(qobj);
>> +    g_assert_cmpstr(str, ==,
>> +                    "bee:\n    [0]: 43\n    [1]: 44\n    [2]: true\n"   \
>> +                    "baz: null\nbar: hello world\nfoo: 42\n");
>
> I don't like this.  The order of QDict members in @str depends on
> qdict_first()/qdict_next() iteration order, which is unspecified.
>
> Here's how we check elsewhere that a QObject matches expectations:
>
>     static void qobject_from_qlit_test(void)
>     {
>         QLitObject qlit = QLIT_QDICT((
>             (QLitDictEntry[]) {
>                 { "foo", QLIT_QNUM(42) },
>                 { "bar", QLIT_QSTR("hello world") },
>                 { "baz", QLIT_QNULL },
>                 { "bee", QLIT_QLIST((
>                     (QLitObject[]) {
>                         QLIT_QNUM(43),
>                         QLIT_QNUM(44),
>                         QLIT_QBOOL(true),
>                         { },
>                     }))
>                 },
>                 { },
>             }));
>         QObject *qobj = qobject_from_qlit(&qlit);
>         QDict *qdict;
>         QList *baz;
>
>         qdict = qobject_to_qdict(qobj);
>         g_assert_cmpint(qdict_get_int(qdict, "foo"), ==, 42);
>         g_assert_cmpstr(qdict_get_str(qdict, "bar"), ==, "hello world");
>         g_assert(qobject_type(qdict_get(qdict, "baz")) == QTYPE_QNULL);
>         baz = qdict_get_qlist(qdict, "bee");
>         g_assert_cmpint(qnum_get_int(qobject_to_qnum(qlist_pop(baz))), ==, 43);
>         g_assert_cmpint(qnum_get_int(qobject_to_qnum(qlist_pop(baz))), ==, 44);
>         g_assert_cmpint(qbool_get_bool(qobject_to_qbool(qlist_pop(baz))), ==, 1);
>
>         qobject_decref(qobj);
>     }
>
> Robust, just tedious.

done the tedious way

>
> check-qjson.c uses a differently tedious technique: compare expected
> QLitObject to actual QObject with compare_litqobj_to_qobj().  I like
> that better, because I find the QLIT... initializers easier to read, and
> less error prone to write.  You might prefer not to use QLit to test
> QLit, though.
>

ok

>> +
>> +    g_free(str);
>> +    qobject_decref(qobj);
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    g_test_init(&argc, &argv, NULL);
>> +
>> +    g_test_add_func("/qlit/qobject_from_qlit", qobject_from_qlit_test);
>> +
>> +    return g_test_run();
>> +}
>> diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
>> index fc8885c9a4..002d25873a 100644
>> --- a/qobject/Makefile.objs
>> +++ b/qobject/Makefile.objs
>> @@ -1,2 +1,2 @@
>> -util-obj-y = qnull.o qnum.o qstring.o qdict.o qlist.o qbool.o
>> +util-obj-y = qnull.o qnum.o qstring.o qdict.o qlist.o qbool.o qlit.o
>>  util-obj-y += qjson.o qobject.o json-lexer.o json-streamer.o json-parser.o
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 7af278db55..960ab8c6dd 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -12,6 +12,8 @@ check-unit-y += tests/test-char$(EXESUF)
>>  gcov-files-check-qdict-y = chardev/char.c
>>  check-unit-y += tests/check-qnum$(EXESUF)
>>  gcov-files-check-qnum-y = qobject/qnum.c
>> +check-unit-y += tests/check-qlit$(EXESUF)
>> +gcov-files-check-qlit-y = qobject/qlit.c
>>  check-unit-y += tests/check-qstring$(EXESUF)
>>  gcov-files-check-qstring-y = qobject/qstring.c
>>  check-unit-y += tests/check-qlist$(EXESUF)
>> @@ -523,7 +525,7 @@ test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
>>       tests/rcutorture.o tests/test-rcu-list.o \
>>       tests/test-qdist.o tests/test-shift128.o \
>>       tests/test-qht.o tests/qht-bench.o tests/test-qht-par.o \
>> -     tests/atomic_add-bench.o
>> +     tests/atomic_add-bench.o tests/check-qlit.o
>
> Please add it right next to the other qobject-related tests:
>

ok

>    test-obj-y = tests/check-qnum.o tests/check-qstring.o tests/check-qdict.o \
>         tests/check-qlist.o tests/check-qnull.o \
> -       tests/check-qjson.o \
> +       tests/check-qjson.o tests/check-qlit.o \
>
>>
>>  $(test-obj-y): QEMU_INCLUDES += -Itests
>>  QEMU_CFLAGS += -I$(SRC_PATH)/tests
>> @@ -541,6 +543,7 @@ test-io-obj-y = $(io-obj-y) $(test-crypto-obj-y)
>>  test-block-obj-y = $(block-obj-y) $(test-io-obj-y) tests/iothread.o
>>
>>  tests/check-qnum$(EXESUF): tests/check-qnum.o $(test-util-obj-y)
>> +tests/check-qlit$(EXESUF): tests/check-qlit.o $(test-util-obj-y)
>>  tests/check-qstring$(EXESUF): tests/check-qstring.o $(test-util-obj-y)
>>  tests/check-qdict$(EXESUF): tests/check-qdict.o $(test-util-obj-y)
>>  tests/check-qlist$(EXESUF): tests/check-qlist.o $(test-util-obj-y)
>
> Same order as in test-obj-y, please.
>

ok

-- 
Marc-André Lureau