Message ID | 20170607163635.17635-44-marcandre.lureau@redhat.com |
---|---|
State | New |
Headers | show |
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. 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(); > }
----- 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(); > > } >
Hi ----- Original Message ----- > > > > 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) > Hmm, there are more allocations in qobject_to_string() though
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Hi > > ----- Original Message ----- >> > >> > 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) monitor_vprintf() formats to a dynamic buffer, which it passes to monitor_puts(). monitor_puts() uses a line buffer. fprintf() can be unbuffered, line-buffered, or fully buffered. Converting everything to a string first is different: the string buffer holds *everything* rather than just a line or some fixed size. > Hmm, there are more allocations in qobject_to_string() though
On 06/09/2017 07:40 AM, Markus Armbruster wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > >> Hi >> >> ----- Original Message ----- >>>> >>>> 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) > > monitor_vprintf() formats to a dynamic buffer, which it passes to > monitor_puts(). monitor_puts() uses a line buffer. > > fprintf() can be unbuffered, line-buffered, or fully buffered. > > Converting everything to a string first is different: the string buffer > holds *everything* rather than just a line or some fixed size. My patch series to create a QAPI-to-JSON output visitor, and convert the existing qobject-to-json conversion to be a thin wrapper over the JSON visitor, is a natural place to create a visitor constructor that can take a callback (where the callback either appends to a QString or uses pwrite() (no need to spend time with the slower printf()) to go straight to stdout. I'll have to revive that series on top of your work.
On 2017-06-08 19:43, Markus Armbruster wrote: > 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. Given that this function is called only by HMP's "info block -v" and qemu-img, I don't think efficiency is too important. In the former case, it's your own fault for using HMP, in the latter you'll have only a single image anyway. So I'm OK with this change. Max > > 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/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); } 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(); }
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(-)