Re: [Qemu-devel] [PATCH v3 43/43] qobject: move dump_qobject() from block/ to qobject/

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

----- Original Message -----
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> > The dump functions could be generally useful for any qobject user or for
> > debugging etc.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/qapi/qmp/qdict.h   |  2 ++
> >  include/qapi/qmp/qlist.h   |  2 ++
> >  include/qapi/qmp/qobject.h |  7 ++++
> >  block/qapi.c               | 90
> >  +++-------------------------------------------
> >  qobject/qdict.c            | 30 ++++++++++++++++
> >  qobject/qlist.c            | 23 ++++++++++++
> >  qobject/qobject.c          | 19 ++++++++++
> >  tests/check-qjson.c        | 19 ++++++++++
> >  8 files changed, 106 insertions(+), 86 deletions(-)
> >
> > diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> > index 8c7c2b762b..1ef3bc8cda 100644
> > --- a/include/qapi/qmp/qdict.h
> > +++ b/include/qapi/qmp/qdict.h
> > @@ -91,4 +91,6 @@ QObject *qdict_crumple(const QDict *src, Error **errp);
> >  
> >  void qdict_join(QDict *dest, QDict *src, bool overwrite);
> >  
> > +char *qdict_to_string(QDict *dict, int indent);
> > +
> >  #endif /* QDICT_H */
> > diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
> > index c4b5fdad9b..c93ac3e15b 100644
> > --- a/include/qapi/qmp/qlist.h
> > +++ b/include/qapi/qmp/qlist.h
> > @@ -60,6 +60,8 @@ size_t qlist_size(const QList *qlist);
> >  QList *qobject_to_qlist(const QObject *obj);
> >  void qlist_destroy_obj(QObject *obj);
> >  
> > +char *qlist_to_string(QList *list, int indent);
> > +
> >  static inline const QListEntry *qlist_first(const QList *qlist)
> >  {
> >      return QTAILQ_FIRST(&qlist->head);
> > diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> > index b8ddbca405..0d6ae5048a 100644
> > --- a/include/qapi/qmp/qobject.h
> > +++ b/include/qapi/qmp/qobject.h
> > @@ -101,4 +101,11 @@ static inline QObject *qnull(void)
> >      return &qnull_;
> >  }
> >  
> > +char *qobject_to_string_indent(QObject *obj, int indent);
> > +
> > +static inline char *qobject_to_string(QObject *obj)
> > +{
> > +    return qobject_to_string_indent(obj, 0);
> > +}
> > +
> >  #endif /* QOBJECT_H */
> > diff --git a/block/qapi.c b/block/qapi.c
> > index 2050df29e4..9b7d42e50a 100644
> > --- a/block/qapi.c
> > +++ b/block/qapi.c
> > @@ -586,101 +586,19 @@ void bdrv_snapshot_dump(fprintf_function
> > func_fprintf, void *f,
> >      }
> >  }
> >  
> > -static void dump_qdict(fprintf_function func_fprintf, void *f, int
> > indentation,
> > -                       QDict *dict);
> > -static void dump_qlist(fprintf_function func_fprintf, void *f, int
> > indentation,
> > -                       QList *list);
> > -
> > -static void dump_qobject(fprintf_function func_fprintf, void *f,
> > -                         int comp_indent, QObject *obj)
> > -{
> > -    switch (qobject_type(obj)) {
> > -        case QTYPE_QNUM: {
> > -            QNum *value = qobject_to_qnum(obj);
> > -            char *tmp = qnum_to_string(value);
> > -            func_fprintf(f, "%s", tmp);
> > -            g_free(tmp);
> > -            break;
> > -        }
> > -        case QTYPE_QSTRING: {
> > -            QString *value = qobject_to_qstring(obj);
> > -            func_fprintf(f, "%s", qstring_get_str(value));
> > -            break;
> > -        }
> > -        case QTYPE_QDICT: {
> > -            QDict *value = qobject_to_qdict(obj);
> > -            dump_qdict(func_fprintf, f, comp_indent, value);
> > -            break;
> > -        }
> > -        case QTYPE_QLIST: {
> > -            QList *value = qobject_to_qlist(obj);
> > -            dump_qlist(func_fprintf, f, comp_indent, value);
> > -            break;
> > -        }
> > -        case QTYPE_QBOOL: {
> > -            QBool *value = qobject_to_qbool(obj);
> > -            func_fprintf(f, "%s", qbool_get_bool(value) ? "true" :
> > "false");
> > -            break;
> > -        }
> > -        default:
> > -            abort();
> > -    }
> > -}
> > -
> > -static void dump_qlist(fprintf_function func_fprintf, void *f, int
> > indentation,
> > -                       QList *list)
> > -{
> > -    const QListEntry *entry;
> > -    int i = 0;
> > -
> > -    for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++)
> > {
> > -        QType type = qobject_type(entry->value);
> > -        bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> > -        func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
> > -                     composite ? '\n' : ' ');
> > -        dump_qobject(func_fprintf, f, indentation + 1, entry->value);
> > -        if (!composite) {
> > -            func_fprintf(f, "\n");
> > -        }
> > -    }
> > -}
> > -
> > -static void dump_qdict(fprintf_function func_fprintf, void *f, int
> > indentation,
> > -                       QDict *dict)
> > -{
> > -    const QDictEntry *entry;
> > -
> > -    for (entry = qdict_first(dict); entry; entry = qdict_next(dict,
> > entry)) {
> > -        QType type = qobject_type(entry->value);
> > -        bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> > -        char *key = g_malloc(strlen(entry->key) + 1);
> > -        int i;
> > -
> > -        /* replace dashes with spaces in key (variable) names */
> > -        for (i = 0; entry->key[i]; i++) {
> > -            key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
> > -        }
> > -        key[i] = 0;
> > -        func_fprintf(f, "%*s%s:%c", indentation * 4, "", key,
> > -                     composite ? '\n' : ' ');
> > -        dump_qobject(func_fprintf, f, indentation + 1, entry->value);
> > -        if (!composite) {
> > -            func_fprintf(f, "\n");
> > -        }
> > -        g_free(key);
> > -    }
> > -}
> > -
> >  void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
> >                                     ImageInfoSpecific *info_spec)
> >  {
> >      QObject *obj, *data;
> >      Visitor *v = qobject_output_visitor_new(&obj);
> > +    char *tmp;
> >  
> >      visit_type_ImageInfoSpecific(v, NULL, &info_spec, &error_abort);
> >      visit_complete(v, &obj);
> >      data = qdict_get(qobject_to_qdict(obj), "data");
> > -    dump_qobject(func_fprintf, f, 1, data);
> > +    tmp = qobject_to_string_indent(data, 1);
> > +    func_fprintf(f, "%s", tmp);
> > +    g_free(tmp);
> >      qobject_decref(obj);
> >      visit_free(v);
> >  }
> 
> The title claims "move dump_qobject() from block/ to qobject/", but
> that's not what the patch does.  It *replaces* dump_qobject() by
> qobject_to_string().  The former dumps to a callback, the latter to a
> dynamic string buffer.
> 
> Providing dump functionality in one way doesn't preclude the other way:
> given callback, one could define a callback that builds up a string
> buffer, and given buffer, one could (and you actually do) pass the
> buffer to a callback.  That's less efficient, though.
> 
> Trading efficiency for ease-of-use should be okay here, but I'm cc'ing
> Max and Kevin to double-check.

I believe convenience is more important than efficiency here. It's easy to call qobject_to_string(foo) from gdb for example, with a callback, it's less easy.

(fprintf or monitor_fprintf will both build an internal buffer anyway, efficiency is probably similar)

> 
> Two ways forward:
> 
> 1. Get Max / Kevin's blessing, change the commit message to match the
> code.
> 
> 2. Change the code to match the commit message.
> 
> > diff --git a/qobject/qdict.c b/qobject/qdict.c
> > index 65069baa1b..7e5a945c5e 100644
> > --- a/qobject/qdict.c
> > +++ b/qobject/qdict.c
> > @@ -1055,3 +1055,33 @@ void qdict_join(QDict *dest, QDict *src, bool
> > overwrite)
> >          entry = next;
> >      }
> >  }
> > +
> > +char *qdict_to_string(QDict *dict, int indent)
> > +{
> > +    const QDictEntry *entry;
> > +    GString *str = g_string_new(NULL);
> > +
> > +    for (entry = qdict_first(dict); entry; entry = qdict_next(dict,
> > entry)) {
> > +        QType type = qobject_type(entry->value);
> > +        bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> > +        char *key = g_malloc(strlen(entry->key) + 1);
> > +        char *val = qobject_to_string_indent(entry->value, indent + 1);
> > +        int i;
> > +
> > +        /* replace dashes with spaces in key (variable) names */
> > +        for (i = 0; entry->key[i]; i++) {
> > +            key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
> > +        }
> > +        key[i] = 0;
> > +        g_string_append_printf(str, "%*s%s:", indent * 4, "", key);
> > +        g_string_append_c(str, composite ? '\n' : ' ');
> > +        g_string_append(str, val);
> > +        if (!composite) {
> > +            g_string_append_c(str, '\n');
> > +        }
> > +        g_free(val);
> > +        g_free(key);
> > +    }
> > +
> > +    return g_string_free(str, false);
> > +}
> > diff --git a/qobject/qlist.c b/qobject/qlist.c
> > index 86b60cb88c..b769248290 100644
> > --- a/qobject/qlist.c
> > +++ b/qobject/qlist.c
> > @@ -158,3 +158,26 @@ void qlist_destroy_obj(QObject *obj)
> >  
> >      g_free(qlist);
> >  }
> > +
> > +char *qlist_to_string(QList *list, int indent)
> > +{
> > +    GString *str = g_string_new(NULL);
> > +    const QListEntry *entry;
> > +    int i = 0;
> > +
> > +    for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++)
> > {
> > +        QType type = qobject_type(entry->value);
> > +        bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
> > +        char *val = qobject_to_string_indent(entry->value, indent + 1);
> > +
> > +        g_string_append_printf(str, "%*s[%i]:", indent * 4, "", i);
> > +        g_string_append_c(str, composite ? '\n' : ' ');
> > +        g_string_append(str, val);
> > +        if (!composite) {
> > +            g_string_append_c(str, '\n');
> > +        }
> > +        g_free(val);
> > +    }
> > +
> > +    return g_string_free(str, false);
> > +}
> > diff --git a/qobject/qobject.c b/qobject/qobject.c
> > index b0cafb66f1..64e959c54f 100644
> > --- a/qobject/qobject.c
> > +++ b/qobject/qobject.c
> > @@ -27,3 +27,22 @@ void qobject_destroy(QObject *obj)
> >      assert(QTYPE_QNULL < obj->type && obj->type < QTYPE__MAX);
> >      qdestroy[obj->type](obj);
> >  }
> > +
> > +char *qobject_to_string_indent(QObject *obj, int indent)
> > +{
> > +    switch (qobject_type(obj)) {
> > +    case QTYPE_QNUM:
> > +        return qnum_to_string(qobject_to_qnum(obj));
> > +    case QTYPE_QSTRING:
> > +        return g_strdup(qstring_get_str(qobject_to_qstring(obj)));
> > +    case QTYPE_QDICT:
> > +        return qdict_to_string(qobject_to_qdict(obj), indent);
> > +    case QTYPE_QLIST:
> > +        return qlist_to_string(qobject_to_qlist(obj), indent);
> > +    case QTYPE_QBOOL:
> > +        return g_strdup(qbool_get_bool(qobject_to_qbool(obj)) ?
> > +                        "true" : "false");
> > +    default:
> > +        abort();
> > +    }
> > +}
> > diff --git a/tests/check-qjson.c b/tests/check-qjson.c
> > index 53f2275b9b..dd3c102bc0 100644
> > --- a/tests/check-qjson.c
> > +++ b/tests/check-qjson.c
> > @@ -1372,6 +1372,23 @@ static void simple_whitespace(void)
> >      }
> >  }
> >  
> > +static void qobject_to_string_test(void)
> > +{
> > +    QObject *obj;
> > +    char *tmp;
> > +
> > +    obj = qobject_from_json("[ 43, { 'c': { 'd' : 12 } }, [ 1, 2 ], 42 ]",
> > +                            &error_abort);
> > +    tmp = qobject_to_string(obj);
> > +    g_assert_cmpstr(tmp, ==,
> > +                    "[0]: 43\n"
> > +                    "[1]:\n    c:\n        d: 12\n"
> > +                    "[2]:\n    [0]: 1\n    [1]: 2\n"
> > +                    "[3]: 42\n");
> > +    g_free(tmp);
> > +    qobject_decref(obj);
> > +}
> > +
> >  static void simple_varargs(void)
> >  {
> >      QObject *embedded_obj;
> > @@ -1545,5 +1562,7 @@ int main(int argc, char **argv)
> >      g_test_add_func("/errors/unterminated/literal", unterminated_literal);
> >      g_test_add_func("/errors/limits/nesting", limits_nesting);
> >  
> > +    g_test_add_func("/qobject/to_string", qobject_to_string_test);
> > +
> >      return g_test_run();
> >  }
>