[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 43/43] qobject: move dump_qobject() from bloc
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v3 43/43] qobject: move dump_qobject() from block/ to qobject/ |
Date: |
Fri, 9 Jun 2017 06:11:53 -0400 (EDT) |
----- Original Message -----
> Marc-André Lureau <address@hidden> writes:
>
> > The dump functions could be generally useful for any qobject user or for
> > debugging etc.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> > 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();
> > }
>
- [Qemu-devel] [PATCH v3 37/43] pnv-core: use get_uint() for "core-pir" property, (continued)
- [Qemu-devel] [PATCH v3 37/43] pnv-core: use get_uint() for "core-pir" property, Marc-André Lureau, 2017/06/07
- [Qemu-devel] [PATCH v3 38/43] numa: use get_uint() for "size" property, Marc-André Lureau, 2017/06/07
- [Qemu-devel] [PATCH v3 39/43] i386/cpu: use get_uint() for "min-level"/"min-xlevel" properties, Marc-André Lureau, 2017/06/07
- [Qemu-devel] [PATCH v3 40/43] console: use get_uint() for "head" property, Marc-André Lureau, 2017/06/07
- [Qemu-devel] [PATCH v3 41/43] tests/qdict: check more get_try_int() cases, Marc-André Lureau, 2017/06/07
- [Qemu-devel] [PATCH v3 42/43] RFC: qdict: add uint, Marc-André Lureau, 2017/06/07
- [Qemu-devel] [PATCH v3 43/43] qobject: move dump_qobject() from block/ to qobject/, Marc-André Lureau, 2017/06/07
- Re: [Qemu-devel] [PATCH v3 43/43] qobject: move dump_qobject() from block/ to qobject/, Max Reitz, 2017/06/09
Re: [Qemu-devel] [PATCH v3 00/43] qobject/qapi: add uint type, no-reply, 2017/06/07
Re: [Qemu-devel] [PATCH v3 00/43] qobject/qapi: add uint type, no-reply, 2017/06/07
Re: [Qemu-devel] [PATCH v3 00/43] qobject/qapi: add uint type, Markus Armbruster, 2017/06/09