* [Qemu-devel] [PATCH v6 01/15] qapi: Visitor documentation tweak
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 02/15] qapi: Assert finite use of 'number' Eric Blake
` (14 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Make it clear that the name parameter to visit_start_struct()
has the same semantics as for visit_start_int().
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: split into separate patch
---
include/qapi/visitor.h | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 6c77a91..4dd7532 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -60,11 +60,12 @@
* visitors are declared here; the remaining visitors are generated in
* qapi-visit.h.
*
- * The @name parameter of visit_type_FOO() describes the relation
- * between this QAPI value and its parent container. When visiting
- * the root of a tree, @name is ignored; when visiting a member of an
- * object, @name is the key associated with the value; and when
- * visiting a member of a list, @name is NULL.
+ * The @name parameter of visit_type_FOO() and visit_start_OBJECT()
+ * describes the relation between this QAPI value and its parent
+ * container. When visiting the root of a tree, @name is ignored;
+ * when visiting a member of an object, @name is the key associated
+ * with the value; and when visiting a member of a list, @name is
+ * NULL.
*
* FIXME: Clients must pass NULL for @name when visiting a member of a
* list, but this leads to poor error messages; it might be nicer to
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 02/15] qapi: Assert finite use of 'number'
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 01/15] qapi: Visitor documentation tweak Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 03/15] qapi: Factor out JSON string escaping Eric Blake
` (13 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: armbru, Kevin Wolf, Max Reitz, Juan Quintela, Amit Shah,
open list:Block layer core
Add assertions to all QMP commands that use a QAPI number (namely
migrate_set_downtime on input; query-migrate and query-blockstats
on output), to make it obvious where we would have to revisit the
code if we were to extend QMP to support Infinity or NaN.
Furthermore, make it part of our contract that we don't plan to
convert QObject to JSON unless all numbers are finite.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: new patch
---
block/accounting.c | 6 +++++-
migration/migration.c | 4 ++++
qobject/qjson.c | 8 ++++----
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/block/accounting.c b/block/accounting.c
index 3f457c4..8b0e909 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h"
+#include <math.h>
#include "block/accounting.h"
#include "block/block_int.h"
#include "qemu/timer.h"
@@ -164,10 +165,13 @@ double block_acct_queue_depth(BlockAcctTimedStats *stats,
enum BlockAcctType type)
{
uint64_t sum, elapsed;
+ double result;
assert(type < BLOCK_MAX_IOTYPE);
sum = timed_average_sum(&stats->latency[type], &elapsed);
- return (double) sum / elapsed;
+ result = (double) sum / elapsed;
+ assert(isfinite(result));
+ return result;
}
diff --git a/migration/migration.c b/migration/migration.c
index e7a425b..8ac4e90 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -14,6 +14,7 @@
*/
#include "qemu/osdep.h"
+#include <math.h>
#include "qemu/cutils.h"
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
@@ -1192,6 +1193,7 @@ void qmp_migrate_set_speed(int64_t value, Error **errp)
void qmp_migrate_set_downtime(double value, Error **errp)
{
+ assert(isfinite(value)); /* JSON (thus QMP) doesn't permit inf or NaN */
value *= 1000; /* Convert to milliseconds */
value = MAX(0, MIN(INT64_MAX, value));
@@ -1811,6 +1813,7 @@ static void *migration_thread(void *opaque)
s->mbps = (((double) transferred_bytes * 8.0) /
((double) time_spent / 1000.0)) / 1000.0 / 1000.0;
+ assert(isfinite(s->mbps));
trace_migrate_transferred(transferred_bytes, time_spent,
bandwidth, max_size);
@@ -1846,6 +1849,7 @@ static void *migration_thread(void *opaque)
if (s->total_time) {
s->mbps = (((double) transferred_bytes * 8.0) /
((double) s->total_time)) / 1000;
+ assert(isfinite(s->mbps));
}
runstate_set(RUN_STATE_POSTMIGRATE);
} else {
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 9a0de89..906a8fc 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -12,6 +12,7 @@
*/
#include "qemu/osdep.h"
+#include <math.h>
#include "qapi/qmp/json-lexer.h"
#include "qapi/qmp/json-parser.h"
#include "qapi/qmp/json-streamer.h"
@@ -231,20 +232,19 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
break;
}
case QTYPE_QFLOAT: {
- QFloat *val = qobject_to_qfloat(obj);
+ double val = qfloat_get_double(qobject_to_qfloat(obj));
char buffer[1024];
int len;
+ assert(isfinite(val));
/* FIXME: snprintf() is locale dependent; but JSON requires
* numbers to be formatted as if in the C locale. Dependence
* on C locale is a pervasive issue in QEMU. */
- /* FIXME: This risks printing Inf or NaN, which are not valid
- * JSON values. */
/* FIXME: the default precision of 6 for %f often causes
* rounding errors; we should be using DBL_DECIMAL_DIG (17),
* and only rounding to a shorter number if the result would
* still produce the same floating point value. */
- len = snprintf(buffer, sizeof(buffer), "%f", qfloat_get_double(val));
+ len = snprintf(buffer, sizeof(buffer), "%f", val);
while (len > 0 && buffer[len - 1] == '0') {
len--;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 03/15] qapi: Factor out JSON string escaping
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 01/15] qapi: Visitor documentation tweak Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 02/15] qapi: Assert finite use of 'number' Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 04/15] qapi: Factor out JSON number formatting Eric Blake
` (12 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Juan Quintela, Amit Shah
Pull out a new qstring_append_json_string() helper, so that all
JSON output producers can use the same output escaping rules.
While it appears that vmstate's use of the simpler qjson.c
formatter is not currently encountering any string that needs
escapes to be valid JSON, it is better to be safe than sorry,
and this substitution is not adding any potential asserts
during migration.
Adding a return value will let callers that care diagnose the
situation where an attempt was made to output invalid JSON (we
use substitute characters, and in fact guarantee that our
output is ASCII via escapes even if the input was UTF-8). None
of the current callers care, but a future patch wants to make
it possible to conditionally turn this situation into an error.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: comment tweaks
[no v5 due to series split]
v4: keep helper in qobject-json.[ch], tweak variable name and
for loop iterator, add return value, drop R-b
v3: rebase to include cleanups in master
v2: no change
---
include/qapi/qmp/qjson.h | 2 +
migration/qjson.c | 10 ++--
qobject/qjson.c | 126 ++++++++++++++++++++++++++---------------------
3 files changed, 76 insertions(+), 62 deletions(-)
diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 02b1f2c..aa8ddd7 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -24,4 +24,6 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap) GCC_FMT_ATTR(1, 0);
QString *qobject_to_json(const QObject *obj);
QString *qobject_to_json_pretty(const QObject *obj);
+int qstring_append_json_string(QString *qstring, const char *str);
+
#endif /* QJSON_H */
diff --git a/migration/qjson.c b/migration/qjson.c
index f345904..741f9e6 100644
--- a/migration/qjson.c
+++ b/migration/qjson.c
@@ -26,6 +26,7 @@
#include "qemu/osdep.h"
#include "qapi/qmp/qstring.h"
#include "migration/qjson.h"
+#include "qapi/qmp/qjson.h"
struct QJSON {
QString *str;
@@ -42,9 +43,8 @@ static void json_emit_element(QJSON *json, const char *name)
}
if (name) {
- qstring_append(json->str, "\"");
- qstring_append(json->str, name);
- qstring_append(json->str, "\" : ");
+ qstring_append_json_string(json->str, name);
+ qstring_append(json->str, " : ");
}
}
@@ -83,9 +83,7 @@ void json_prop_int(QJSON *json, const char *name, int64_t val)
void json_prop_str(QJSON *json, const char *name, const char *str)
{
json_emit_element(json, name);
- qstring_append_chr(json->str, '"');
- qstring_append(json->str, str);
- qstring_append_chr(json->str, '"');
+ qstring_append_json_string(json->str, str);
}
const char *qjson_get_str(QJSON *json)
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 906a8fc..6810726 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -82,7 +82,6 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent);
static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
{
ToJsonIterState *s = opaque;
- QString *qkey;
int j;
if (s->count) {
@@ -95,9 +94,7 @@ static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
qstring_append(s->str, " ");
}
- qkey = qstring_from_str(key);
- to_json(QOBJECT(qkey), s->str, s->pretty, s->indent);
- QDECREF(qkey);
+ qstring_append_json_string(s->str, key);
qstring_append(s->str, ": ");
to_json(obj, s->str, s->pretty, s->indent);
@@ -139,58 +136,9 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
}
case QTYPE_QSTRING: {
QString *val = qobject_to_qstring(obj);
- const char *ptr;
- int cp;
- char buf[16];
- char *end;
-
- ptr = qstring_get_str(val);
- qstring_append(str, "\"");
-
- for (; *ptr; ptr = end) {
- cp = mod_utf8_codepoint(ptr, 6, &end);
- switch (cp) {
- case '\"':
- qstring_append(str, "\\\"");
- break;
- case '\\':
- qstring_append(str, "\\\\");
- break;
- case '\b':
- qstring_append(str, "\\b");
- break;
- case '\f':
- qstring_append(str, "\\f");
- break;
- case '\n':
- qstring_append(str, "\\n");
- break;
- case '\r':
- qstring_append(str, "\\r");
- break;
- case '\t':
- qstring_append(str, "\\t");
- break;
- default:
- if (cp < 0) {
- cp = 0xFFFD; /* replacement character */
- }
- if (cp > 0xFFFF) {
- /* beyond BMP; need a surrogate pair */
- snprintf(buf, sizeof(buf), "\\u%04X\\u%04X",
- 0xD800 + ((cp - 0x10000) >> 10),
- 0xDC00 + ((cp - 0x10000) & 0x3FF));
- } else if (cp < 0x20 || cp >= 0x7F) {
- snprintf(buf, sizeof(buf), "\\u%04X", cp);
- } else {
- buf[0] = cp;
- buf[1] = 0;
- }
- qstring_append(str, buf);
- }
- };
-
- qstring_append(str, "\"");
+ /* FIXME: no way inform user if we replaced invalid UTF-8
+ * sequences to avoid invalid JSON */
+ qstring_append_json_string(str, qstring_get_str(val));
break;
}
case QTYPE_QDICT: {
@@ -290,3 +238,69 @@ QString *qobject_to_json_pretty(const QObject *obj)
return str;
}
+
+/**
+ * Append a JSON string to @qstring that encodes the C string @str.
+ *
+ * @str is in UTF-8. The JSON string is enclosed in double quotes and
+ * has funny characters escaped. Invalid UTF-8 sequences are replaced
+ * by (properly escaped) U+0xFFFD replacement characters. Return -1 if
+ * invalid sequences were replaced, else 0.
+ */
+int qstring_append_json_string(QString *qstring, const char *str)
+{
+ const char *ptr;
+ int cp;
+ char buf[16];
+ char *end;
+ int result = 0;
+
+ qstring_append(qstring, "\"");
+
+ for (ptr = str; *ptr; ptr = end) {
+ cp = mod_utf8_codepoint(ptr, 6, &end);
+ switch (cp) {
+ case '\"':
+ qstring_append(qstring, "\\\"");
+ break;
+ case '\\':
+ qstring_append(qstring, "\\\\");
+ break;
+ case '\b':
+ qstring_append(qstring, "\\b");
+ break;
+ case '\f':
+ qstring_append(qstring, "\\f");
+ break;
+ case '\n':
+ qstring_append(qstring, "\\n");
+ break;
+ case '\r':
+ qstring_append(qstring, "\\r");
+ break;
+ case '\t':
+ qstring_append(qstring, "\\t");
+ break;
+ default:
+ if (cp < 0) {
+ cp = 0xFFFD; /* replacement character */
+ result = -1;
+ }
+ if (cp > 0xFFFF) {
+ /* beyond BMP; need a surrogate pair */
+ snprintf(buf, sizeof(buf), "\\u%04X\\u%04X",
+ 0xD800 + ((cp - 0x10000) >> 10),
+ 0xDC00 + ((cp - 0x10000) & 0x3FF));
+ } else if (cp < 0x20 || cp >= 0x7F) {
+ snprintf(buf, sizeof(buf), "\\u%04X", cp);
+ } else {
+ buf[0] = cp;
+ buf[1] = 0;
+ }
+ qstring_append(qstring, buf);
+ }
+ };
+
+ qstring_append(qstring, "\"");
+ return result;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 04/15] qapi: Factor out JSON number formatting
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (2 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 03/15] qapi: Factor out JSON string escaping Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 05/15] qapi: Add qstring_append_printf() Eric Blake
` (11 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
Pull out a new qstring_append_json_number() helper, so that all
JSON output producers can use a consistent style for printing
floating point without duplicating code (since we are doing more
data massaging than a simple printf format can handle). (For
now, there is only one client, but later patches will use it.)
Unlike qstring_append_json_string() which has a return value to
allow callers to detect when output was munged to produce valid
JSON, this code either succeeds or triggers an assertion failure.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: drop return value, now that we assert finite floats on any
QObject converted to JSON
[no v5 due to series split]
v4: keep helper in qobject-json.[ch], don't use Error **errp for
inf/NaN handling, drop R-b
v3: rebase to latest; even though the patch differs quite a bit
from v2, the changes are due to prior comment changes that are
just moving between files, so R-b kept
v2: minor wording tweaks
---
include/qapi/qmp/qjson.h | 1 +
qobject/qjson.c | 57 +++++++++++++++++++++++++++++-------------------
2 files changed, 36 insertions(+), 22 deletions(-)
diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index aa8ddd7..6fb912e 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -25,5 +25,6 @@ QString *qobject_to_json(const QObject *obj);
QString *qobject_to_json_pretty(const QObject *obj);
int qstring_append_json_string(QString *qstring, const char *str);
+void qstring_append_json_number(QString *qstring, double number);
#endif /* QJSON_H */
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 6810726..b47a361 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -181,29 +181,8 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
}
case QTYPE_QFLOAT: {
double val = qfloat_get_double(qobject_to_qfloat(obj));
- char buffer[1024];
- int len;
- assert(isfinite(val));
- /* FIXME: snprintf() is locale dependent; but JSON requires
- * numbers to be formatted as if in the C locale. Dependence
- * on C locale is a pervasive issue in QEMU. */
- /* FIXME: the default precision of 6 for %f often causes
- * rounding errors; we should be using DBL_DECIMAL_DIG (17),
- * and only rounding to a shorter number if the result would
- * still produce the same floating point value. */
- len = snprintf(buffer, sizeof(buffer), "%f", val);
- while (len > 0 && buffer[len - 1] == '0') {
- len--;
- }
-
- if (len && buffer[len - 1] == '.') {
- buffer[len - 1] = 0;
- } else {
- buffer[len] = 0;
- }
-
- qstring_append(str, buffer);
+ qstring_append_json_number(str, val);
break;
}
case QTYPE_QBOOL: {
@@ -304,3 +283,37 @@ int qstring_append_json_string(QString *qstring, const char *str)
qstring_append(qstring, "\"");
return result;
}
+
+/**
+ * Append a JSON representation of @number to @qstring.
+ *
+ * Requires @number to be finite, per RFC 7159.
+ */
+void qstring_append_json_number(QString *qstring, double number)
+{
+ char buffer[1024];
+ int len;
+
+ assert(isfinite(number));
+
+ /* FIXME: snprintf() is locale dependent; but JSON requires
+ * numbers to be formatted as if in the C locale. Dependence
+ * on C locale is a pervasive issue in QEMU. */
+ /* FIXME: the default precision of 6 for %f often causes
+ * rounding errors; we should be using DBL_DECIMAL_DIG (17),
+ * and only rounding to a shorter number if the result would
+ * still produce the same floating point value. */
+ len = snprintf(buffer, sizeof(buffer), "%f", number);
+ assert(len > 0 && len < sizeof(buffer));
+ while (len > 0 && buffer[len - 1] == '0') {
+ len--;
+ }
+
+ if (len && buffer[len - 1] == '.') {
+ buffer[len - 1] = 0;
+ } else {
+ buffer[len] = 0;
+ }
+
+ qstring_append(qstring, buffer);
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 05/15] qapi: Add qstring_append_printf()
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (3 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 04/15] qapi: Factor out JSON number formatting Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 06/15] qapi: Use qstring_append_chr() where appropriate Eric Blake
` (10 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Juan Quintela, Amit Shah
Back in commit 764c1ca (Nov 2009), we added qstring_append_int().
However, it did not see any use until commit 190c882 (Jan 2015).
Furthermore, it has a rather limited use case - to print anything
else, callers still have to format into a temporary buffer, unless
we want to introduce an explosion of new qstring_append_* methods
for each useful type to print.
A much better approach is to add a wrapper that merges printf
behavior onto qstring_append, via the new qstring_append_printf()
(and its vararg counterpart), with a name based on glib's
g_string_append_printf(). In fact, with our wrapper in place, we
no longer need qstring_append_int().
Other immediate uses for the new function include simplifying
two existing clients of qstring_append() on a just-formatted
buffer, and the fact that we can take advantage of printf width
manipulations for more efficient indentation.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: no change
[no v5 due to series split]
v4: retitle, s/_format/_printf/, drop R-b, move up in series
v3: rebase to master
v2: also simplify qstring_append_json_string(), add assertion that
format is well-formed
---
include/qapi/qmp/qstring.h | 7 +++++--
migration/qjson.c | 2 +-
qobject/qjson.c | 38 ++++++++++----------------------------
qobject/qstring.c | 26 +++++++++++++++++++++-----
4 files changed, 37 insertions(+), 36 deletions(-)
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 10076b7..a987f3b 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -1,7 +1,7 @@
/*
* QString Module
*
- * Copyright (C) 2009 Red Hat Inc.
+ * Copyright (C) 2009-2016 Red Hat Inc.
*
* Authors:
* Luiz Capitulino <lcapitulino@redhat.com>
@@ -27,9 +27,12 @@ QString *qstring_from_str(const char *str);
QString *qstring_from_substr(const char *str, int start, int end);
size_t qstring_get_length(const QString *qstring);
const char *qstring_get_str(const QString *qstring);
-void qstring_append_int(QString *qstring, int64_t value);
void qstring_append(QString *qstring, const char *str);
void qstring_append_chr(QString *qstring, int c);
+void qstring_append_printf(QString *qstring, const char *fmt, ...)
+ GCC_FMT_ATTR(2, 3);
+void qstring_append_vprintf(QString *qstring, const char *fmt, va_list ap)
+ GCC_FMT_ATTR(2, 0);
QString *qobject_to_qstring(const QObject *obj);
void qstring_destroy_obj(QObject *obj);
diff --git a/migration/qjson.c b/migration/qjson.c
index 741f9e6..f11effd 100644
--- a/migration/qjson.c
+++ b/migration/qjson.c
@@ -77,7 +77,7 @@ void json_end_array(QJSON *json)
void json_prop_int(QJSON *json, const char *name, int64_t val)
{
json_emit_element(json, name);
- qstring_append_int(json->str, val);
+ qstring_append_printf(json->str, "%" PRId64, val);
}
void json_prop_str(QJSON *json, const char *name, const char *str)
diff --git a/qobject/qjson.c b/qobject/qjson.c
index b47a361..7dc694c 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -82,16 +82,13 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent);
static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
{
ToJsonIterState *s = opaque;
- int j;
if (s->count) {
qstring_append(s->str, s->pretty ? "," : ", ");
}
if (s->pretty) {
- qstring_append(s->str, "\n");
- for (j = 0 ; j < s->indent ; j++)
- qstring_append(s->str, " ");
+ qstring_append_printf(s->str, "\n%*s", 4 * s->indent, "");
}
qstring_append_json_string(s->str, key);
@@ -104,16 +101,13 @@ static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
static void to_json_list_iter(QObject *obj, void *opaque)
{
ToJsonIterState *s = opaque;
- int j;
if (s->count) {
qstring_append(s->str, s->pretty ? "," : ", ");
}
if (s->pretty) {
- qstring_append(s->str, "\n");
- for (j = 0 ; j < s->indent ; j++)
- qstring_append(s->str, " ");
+ qstring_append_printf(s->str, "\n%*s", 4 * s->indent, "");
}
to_json(obj, s->str, s->pretty, s->indent);
@@ -128,10 +122,7 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
break;
case QTYPE_QINT: {
QInt *val = qobject_to_qint(obj);
- char buffer[1024];
-
- snprintf(buffer, sizeof(buffer), "%" PRId64, qint_get_int(val));
- qstring_append(str, buffer);
+ qstring_append_printf(str, "%" PRId64, qint_get_int(val));
break;
}
case QTYPE_QSTRING: {
@@ -152,10 +143,7 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
qstring_append(str, "{");
qdict_iter(val, to_json_dict_iter, &s);
if (pretty) {
- int j;
- qstring_append(str, "\n");
- for (j = 0 ; j < indent ; j++)
- qstring_append(str, " ");
+ qstring_append_printf(str, "\n%*s", 4 * indent, "");
}
qstring_append(str, "}");
break;
@@ -171,10 +159,7 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
qstring_append(str, "[");
qlist_iter(val, (void *)to_json_list_iter, &s);
if (pretty) {
- int j;
- qstring_append(str, "\n");
- for (j = 0 ; j < indent ; j++)
- qstring_append(str, " ");
+ qstring_append_printf(str, "\n%*s", 4 * indent, "");
}
qstring_append(str, "]");
break;
@@ -230,7 +215,6 @@ int qstring_append_json_string(QString *qstring, const char *str)
{
const char *ptr;
int cp;
- char buf[16];
char *end;
int result = 0;
@@ -267,16 +251,14 @@ int qstring_append_json_string(QString *qstring, const char *str)
}
if (cp > 0xFFFF) {
/* beyond BMP; need a surrogate pair */
- snprintf(buf, sizeof(buf), "\\u%04X\\u%04X",
- 0xD800 + ((cp - 0x10000) >> 10),
- 0xDC00 + ((cp - 0x10000) & 0x3FF));
+ qstring_append_printf(qstring, "\\u%04X\\u%04X",
+ 0xD800 + ((cp - 0x10000) >> 10),
+ 0xDc00 + ((cp - 0x10000) & 0x3FF));
} else if (cp < 0x20 || cp >= 0x7F) {
- snprintf(buf, sizeof(buf), "\\u%04X", cp);
+ qstring_append_printf(qstring, "\\u%04X", cp);
} else {
- buf[0] = cp;
- buf[1] = 0;
+ qstring_append_chr(qstring, cp);
}
- qstring_append(qstring, buf);
}
};
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 5da7b5f..fbfae27 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -1,7 +1,7 @@
/*
* QString Module
*
- * Copyright (C) 2009 Red Hat Inc.
+ * Copyright (C) 2009-2016 Red Hat Inc.
*
* Authors:
* Luiz Capitulino <lcapitulino@redhat.com>
@@ -88,12 +88,28 @@ void qstring_append(QString *qstring, const char *str)
qstring->string[qstring->length] = 0;
}
-void qstring_append_int(QString *qstring, int64_t value)
+void qstring_append_printf(QString *qstring, const char *fmt, ...)
{
- char num[32];
+ va_list ap;
- snprintf(num, sizeof(num), "%" PRId64, value);
- qstring_append(qstring, num);
+ va_start(ap, fmt);
+ qstring_append_vprintf(qstring, fmt, ap);
+ va_end(ap);
+}
+
+void qstring_append_vprintf(QString *qstring, const char *fmt, va_list ap)
+{
+ va_list ap2;
+ int len;
+
+ va_copy(ap2, ap);
+ len = vsnprintf(NULL, 0, fmt, ap2);
+ assert(len >= 0);
+ va_end(ap2);
+
+ capacity_increase(qstring, len);
+ vsnprintf(qstring->string + qstring->length, len + 1, fmt, ap);
+ qstring->length += len;
}
/**
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 06/15] qapi: Use qstring_append_chr() where appropriate
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (4 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 05/15] qapi: Add qstring_append_printf() Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 07/15] qstring: Add qstring_consume_str() Eric Blake
` (9 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
qstring_append_chr() is more efficient than qstring_append()
when dealing with a one-byte string (including the case of a
temporary 2-byte buffer just for creating a dynamic one-byte
string).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: no change
[no v5 due to series split]
v4: also convert qstring_append() of one-byte strings
v3: no change
v2: no change
---
qobject/json-parser.c | 25 ++++++++++---------------
qobject/qjson.c | 12 ++++++------
2 files changed, 16 insertions(+), 21 deletions(-)
diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index c18e48a..5c23740 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -143,39 +143,39 @@ static QString *qstring_from_escaped_str(JSONParserContext *ctxt,
switch (*ptr) {
case '"':
- qstring_append(str, "\"");
+ qstring_append_chr(str, '"');
ptr++;
break;
case '\'':
- qstring_append(str, "'");
+ qstring_append_chr(str, '\'');
ptr++;
break;
case '\\':
- qstring_append(str, "\\");
+ qstring_append_chr(str, '\\');
ptr++;
break;
case '/':
- qstring_append(str, "/");
+ qstring_append_chr(str, '/');
ptr++;
break;
case 'b':
- qstring_append(str, "\b");
+ qstring_append_chr(str, '\b');
ptr++;
break;
case 'f':
- qstring_append(str, "\f");
+ qstring_append_chr(str, '\f');
ptr++;
break;
case 'n':
- qstring_append(str, "\n");
+ qstring_append_chr(str, '\n');
ptr++;
break;
case 'r':
- qstring_append(str, "\r");
+ qstring_append_chr(str, '\r');
ptr++;
break;
case 't':
- qstring_append(str, "\t");
+ qstring_append_chr(str, '\t');
ptr++;
break;
case 'u': {
@@ -204,12 +204,7 @@ static QString *qstring_from_escaped_str(JSONParserContext *ctxt,
goto out;
}
} else {
- char dummy[2];
-
- dummy[0] = *ptr++;
- dummy[1] = 0;
-
- qstring_append(str, dummy);
+ qstring_append_chr(str, *ptr++);
}
}
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 7dc694c..a705aba 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -140,12 +140,12 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
s.str = str;
s.indent = indent + 1;
s.pretty = pretty;
- qstring_append(str, "{");
+ qstring_append_chr(str, '{');
qdict_iter(val, to_json_dict_iter, &s);
if (pretty) {
qstring_append_printf(str, "\n%*s", 4 * indent, "");
}
- qstring_append(str, "}");
+ qstring_append_chr(str, '}');
break;
}
case QTYPE_QLIST: {
@@ -156,12 +156,12 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
s.str = str;
s.indent = indent + 1;
s.pretty = pretty;
- qstring_append(str, "[");
+ qstring_append_chr(str, '[');
qlist_iter(val, (void *)to_json_list_iter, &s);
if (pretty) {
qstring_append_printf(str, "\n%*s", 4 * indent, "");
}
- qstring_append(str, "]");
+ qstring_append_chr(str, ']');
break;
}
case QTYPE_QFLOAT: {
@@ -218,7 +218,7 @@ int qstring_append_json_string(QString *qstring, const char *str)
char *end;
int result = 0;
- qstring_append(qstring, "\"");
+ qstring_append_chr(qstring, '"');
for (ptr = str; *ptr; ptr = end) {
cp = mod_utf8_codepoint(ptr, 6, &end);
@@ -262,7 +262,7 @@ int qstring_append_json_string(QString *qstring, const char *str)
}
};
- qstring_append(qstring, "\"");
+ qstring_append_chr(qstring, '"');
return result;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 07/15] qstring: Add qstring_consume_str()
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (5 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 06/15] qapi: Use qstring_append_chr() where appropriate Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 08/15] qstring: Add qstring_wrap_str() Eric Blake
` (8 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Dr. David Alan Gilbert, Andreas Färber
Similar to g_string_free(), there are cases where we want to
destroy our reference to a QString while grabbing its contents,
where we want to avoid use-after-free but also avoid a needless
strdup(). But unlike g_string_free(), we are at least sensible
enough to add this feature via a different function name,
instead of trying to overload two completely separate concepts
into a single function. Do this by introducing the new
qstring_consume_str(), then use it where it makes sense.
In the case of monitor.c, note that QString _always_ has a
non-NULL embedded string with at least one byte allocated for
a terminating NUL, so special-casing on length 0 was wasted code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: no change
[no v5 due to series split]
v4: new patch, suggested by Markus
---
include/qapi/qmp/qstring.h | 1 +
monitor.c | 6 +-----
qobject/qstring.c | 23 +++++++++++++++++++++++
qom/object.c | 3 +--
tests/check-qstring.c | 31 +++++++++++++++++++++++++++----
5 files changed, 53 insertions(+), 11 deletions(-)
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index a987f3b..2d55c87 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -27,6 +27,7 @@ QString *qstring_from_str(const char *str);
QString *qstring_from_substr(const char *str, int start, int end);
size_t qstring_get_length(const QString *qstring);
const char *qstring_get_str(const QString *qstring);
+char *qstring_consume_str(QString *qstring);
void qstring_append(QString *qstring, const char *str);
void qstring_append_chr(QString *qstring, int c);
void qstring_append_printf(QString *qstring, const char *fmt, ...)
diff --git a/monitor.c b/monitor.c
index 4ff74b7..aed0d0b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -624,11 +624,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
cur_mon = old_mon;
qemu_mutex_lock(&hmp.out_lock);
- if (qstring_get_length(hmp.outbuf) > 0) {
- output = g_strdup(qstring_get_str(hmp.outbuf));
- } else {
- output = g_strdup("");
- }
+ output = qstring_consume_str(hmp.outbuf);
qemu_mutex_unlock(&hmp.out_lock);
out:
diff --git a/qobject/qstring.c b/qobject/qstring.c
index fbfae27..7a438e9 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -145,6 +145,29 @@ const char *qstring_get_str(const QString *qstring)
}
/**
+ * qstring_consume_str(): Destructively convert a QString to string
+ *
+ * The reference count of @qstring is decremented by one, and the
+ * caller is responsible for calling g_free() on the result. In the
+ * common case where @qstring was not shared, this is faster than
+ * using strdup() on the result of qstring_get_str(); otherwise, the
+ * result is a copy and remaining users of @qstring are unaffected.
+ */
+char *qstring_consume_str(QString *qstring)
+{
+ char *result;
+
+ if (qstring->base.refcnt == 1) {
+ result = qstring->string;
+ qstring->string = NULL;
+ } else {
+ result = g_strdup(qstring->string);
+ }
+ qobject_decref(&qstring->base);
+ return result;
+}
+
+/**
* qstring_destroy_obj(): Free all memory allocated by a QString
* object
*/
diff --git a/qom/object.c b/qom/object.c
index 8166b7d..1a3c83e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1112,10 +1112,9 @@ char *object_property_get_str(Object *obj, const char *name,
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name, "string");
retval = NULL;
} else {
- retval = g_strdup(qstring_get_str(qstring));
+ retval = qstring_consume_str(qstring);
}
- QDECREF(qstring);
return retval;
}
diff --git a/tests/check-qstring.c b/tests/check-qstring.c
index 239e9d9..11823c2 100644
--- a/tests/check-qstring.c
+++ b/tests/check-qstring.c
@@ -1,7 +1,7 @@
/*
* QString unit-tests.
*
- * Copyright (C) 2009 Red Hat Inc.
+ * Copyright (C) 2009-2016 Red Hat Inc.
*
* Authors:
* Luiz Capitulino <lcapitulino@redhat.com>
@@ -31,9 +31,7 @@ static void qstring_from_str_test(void)
g_assert(strcmp(str, qstring->string) == 0);
g_assert(qobject_type(QOBJECT(qstring)) == QTYPE_QSTRING);
- // destroy doesn't exit yet
- g_free(qstring->string);
- g_free(qstring);
+ QDECREF(qstring);
}
static void qstring_destroy_test(void)
@@ -55,6 +53,30 @@ static void qstring_get_str_test(void)
QDECREF(qstring);
}
+static void qstring_consume_str_test(void)
+{
+ QString *qstring;
+ char *ret_str;
+ const char *str = "QEMU/KVM";
+ char *ptr;
+
+ qstring = qstring_from_str(str);
+ QINCREF(qstring);
+ g_assert_cmpint(qstring->base.refcnt, ==, 2);
+ ptr = qstring->string;
+
+ ret_str = qstring_consume_str(qstring);
+ g_assert_cmpint(qstring->base.refcnt, ==, 1);
+ g_assert(ret_str != ptr);
+ g_assert_cmpstr(ret_str, ==, str);
+ g_free(ret_str);
+
+ ret_str = qstring_consume_str(qstring);
+ g_assert(ret_str == ptr);
+ g_assert_cmpstr(ret_str, ==, str);
+ g_free(ret_str);
+}
+
static void qstring_append_chr_test(void)
{
int i;
@@ -102,6 +124,7 @@ int main(int argc, char **argv)
g_test_add_func("/public/append_chr", qstring_append_chr_test);
g_test_add_func("/public/from_substr", qstring_from_substr_test);
g_test_add_func("/public/to_qstring", qobject_to_qstring_test);
+ g_test_add_func("/public/consume_str", qstring_consume_str_test);
return g_test_run();
}
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 08/15] qstring: Add qstring_wrap_str()
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (6 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 07/15] qstring: Add qstring_consume_str() Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-11 11:08 ` Marc-André Lureau
2016-10-11 15:24 ` [Qemu-devel] [PATCH v6 08.5/15] fixup! " Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 09/15] qobject: Consolidate qobject_to_json() calls Eric Blake
` (7 subsequent siblings)
15 siblings, 2 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: armbru, Kevin Wolf, Max Reitz, Chrysostomos Nanakos, Jeff Cody,
open list:Block layer core
Several spots in the code malloc a string, then wrap it in a
QString, which has to duplicate the input. Adding a new
constructor with transfer semantics makes this pattern more
efficient, comparable to the just-added transfer semantics to
go from QString back to raw string. Use the new
qstring_wrap_str() where it makes sense.
The new test still passes under valgrind.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: don't auto-convert NULL into ""
[no v5 due to series split]
v4: new patch
---
include/qapi/qmp/qstring.h | 1 +
block.c | 3 ++-
block/archipelago.c | 6 ++----
blockdev.c | 3 +--
qobject/qstring.c | 20 ++++++++++++++++++++
tests/check-qstring.c | 15 +++++++++++++++
6 files changed, 41 insertions(+), 7 deletions(-)
diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 2d55c87..97cf776 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -25,6 +25,7 @@ typedef struct QString {
QString *qstring_new(void);
QString *qstring_from_str(const char *str);
QString *qstring_from_substr(const char *str, int start, int end);
+QString *qstring_wrap_str(char *str);
size_t qstring_get_length(const QString *qstring);
const char *qstring_get_str(const QString *qstring);
char *qstring_consume_str(QString *qstring);
diff --git a/block.c b/block.c
index bb1f1ec..8a2876f 100644
--- a/block.c
+++ b/block.c
@@ -1640,7 +1640,8 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
qdict_put(snapshot_options, "file.driver",
qstring_from_str("file"));
qdict_put(snapshot_options, "file.filename",
- qstring_from_str(tmp_filename));
+ qstring_wrap_str(tmp_filename));
+ tmp_filename = NULL;
qdict_put(snapshot_options, "driver",
qstring_from_str("qcow2"));
diff --git a/block/archipelago.c b/block/archipelago.c
index 37b8aca..ac047e4 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -426,13 +426,11 @@ static void archipelago_parse_filename(const char *filename, QDict *options,
parse_filename_opts(filename, errp, &volume, &segment_name, &mport, &vport);
if (volume) {
- qdict_put(options, ARCHIPELAGO_OPT_VOLUME, qstring_from_str(volume));
- g_free(volume);
+ qdict_put(options, ARCHIPELAGO_OPT_VOLUME, qstring_wrap_str(volume));
}
if (segment_name) {
qdict_put(options, ARCHIPELAGO_OPT_SEGMENT,
- qstring_from_str(segment_name));
- g_free(segment_name);
+ qstring_wrap_str(segment_name));
}
if (mport != NoPort) {
qdict_put(options, ARCHIPELAGO_OPT_MPORT, qint_from_int(mport));
diff --git a/blockdev.c b/blockdev.c
index 07ec733..35cd905 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1026,8 +1026,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
new_id = g_strdup_printf("%s%s%i", if_name[type],
mediastr, unit_id);
}
- qdict_put(bs_opts, "id", qstring_from_str(new_id));
- g_free(new_id);
+ qdict_put(bs_opts, "id", qstring_wrap_str(new_id));
}
/* Add virtio block device */
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 7a438e9..a64373a 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -66,6 +66,26 @@ QString *qstring_from_str(const char *str)
return qstring_from_substr(str, 0, strlen(str) - 1);
}
+/**
+ * qstring_wrap_str(): Create a new QString around a malloc'd C string
+ *
+ * Returns a strong reference, and caller must not use @str any more.
+ * @str must not be NULL.
+ */
+QString *qstring_wrap_str(char *str)
+{
+ QString *qstring;
+
+ qstring = g_malloc(sizeof(*qstring));
+ qobject_init(QOBJECT(qstring), QTYPE_QSTRING);
+
+ assert(str);
+ qstring->string = str;
+ qstring->capacity = qstring->length = strlen(str);
+
+ return qstring;
+}
+
static void capacity_increase(QString *qstring, size_t len)
{
if (qstring->capacity < (qstring->length + len)) {
diff --git a/tests/check-qstring.c b/tests/check-qstring.c
index 11823c2..0c427c7 100644
--- a/tests/check-qstring.c
+++ b/tests/check-qstring.c
@@ -34,6 +34,20 @@ static void qstring_from_str_test(void)
QDECREF(qstring);
}
+static void qstring_wrap_str_test(void)
+{
+ QString *qstring;
+ char *str = g_strdup("QEMU");
+
+ qstring = qstring_wrap_str(str);
+ g_assert(qstring != NULL);
+ g_assert(qstring->base.refcnt == 1);
+ g_assert(strcmp("QEMU", qstring->string) == 0);
+ g_assert(qobject_type(QOBJECT(qstring)) == QTYPE_QSTRING);
+
+ QDECREF(qstring);
+}
+
static void qstring_destroy_test(void)
{
QString *qstring = qstring_from_str("destroy test");
@@ -119,6 +133,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
g_test_add_func("/public/from_str", qstring_from_str_test);
+ g_test_add_func("/public/wrap_str", qstring_wrap_str_test);
g_test_add_func("/public/destroy", qstring_destroy_test);
g_test_add_func("/public/get_str", qstring_get_str_test);
g_test_add_func("/public/append_chr", qstring_append_chr_test);
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/15] qstring: Add qstring_wrap_str()
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 08/15] qstring: Add qstring_wrap_str() Eric Blake
@ 2016-10-11 11:08 ` Marc-André Lureau
2016-10-11 15:04 ` Eric Blake
2016-10-11 15:24 ` [Qemu-devel] [PATCH v6 08.5/15] fixup! " Eric Blake
1 sibling, 1 reply; 24+ messages in thread
From: Marc-André Lureau @ 2016-10-11 11:08 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: Kevin Wolf, Chrysostomos Nanakos, open list:Block layer core,
Jeff Cody, armbru, Max Reitz
Hi
On Mon, Oct 10, 2016 at 5:25 PM Eric Blake <eblake@redhat.com> wrote:
> Several spots in the code malloc a string, then wrap it in a
> QString, which has to duplicate the input. Adding a new
> constructor with transfer semantics makes this pattern more
> efficient, comparable to the just-added transfer semantics to
> go from QString back to raw string. Use the new
> qstring_wrap_str() where it makes sense.
>
> The new test still passes under valgrind.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: don't auto-convert NULL into ""
> [no v5 due to series split]
> v4: new patch
> ---
> include/qapi/qmp/qstring.h | 1 +
> block.c | 3 ++-
> block/archipelago.c | 6 ++----
> blockdev.c | 3 +--
> qobject/qstring.c | 20 ++++++++++++++++++++
> tests/check-qstring.c | 15 +++++++++++++++
> 6 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index 2d55c87..97cf776 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -25,6 +25,7 @@ typedef struct QString {
> QString *qstring_new(void);
> QString *qstring_from_str(const char *str);
> QString *qstring_from_substr(const char *str, int start, int end);
> +QString *qstring_wrap_str(char *str);
> size_t qstring_get_length(const QString *qstring);
> const char *qstring_get_str(const QString *qstring);
> char *qstring_consume_str(QString *qstring);
> diff --git a/block.c b/block.c
> index bb1f1ec..8a2876f 100644
> --- a/block.c
> +++ b/block.c
> @@ -1640,7 +1640,8 @@ static BlockDriverState
> *bdrv_append_temp_snapshot(BlockDriverState *bs,
> qdict_put(snapshot_options, "file.driver",
> qstring_from_str("file"));
> qdict_put(snapshot_options, "file.filename",
> - qstring_from_str(tmp_filename));
> + qstring_wrap_str(tmp_filename));
> + tmp_filename = NULL;
> qdict_put(snapshot_options, "driver",
> qstring_from_str("qcow2"));
>
>
You could also remove g_free(tmp_filename) from the normal return path
(this may please static analyzers).
diff --git a/block/archipelago.c b/block/archipelago.c
> index 37b8aca..ac047e4 100644
> --- a/block/archipelago.c
> +++ b/block/archipelago.c
> @@ -426,13 +426,11 @@ static void archipelago_parse_filename(const char
> *filename, QDict *options,
> parse_filename_opts(filename, errp, &volume, &segment_name, &mport,
> &vport);
>
> if (volume) {
> - qdict_put(options, ARCHIPELAGO_OPT_VOLUME,
> qstring_from_str(volume));
> - g_free(volume);
> + qdict_put(options, ARCHIPELAGO_OPT_VOLUME,
> qstring_wrap_str(volume));
> }
> if (segment_name) {
> qdict_put(options, ARCHIPELAGO_OPT_SEGMENT,
> - qstring_from_str(segment_name));
> - g_free(segment_name);
> + qstring_wrap_str(segment_name));
> }
> if (mport != NoPort) {
> qdict_put(options, ARCHIPELAGO_OPT_MPORT, qint_from_int(mport));
> diff --git a/blockdev.c b/blockdev.c
> index 07ec733..35cd905 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1026,8 +1026,7 @@ DriveInfo *drive_new(QemuOpts *all_opts,
> BlockInterfaceType block_default_type)
> new_id = g_strdup_printf("%s%s%i", if_name[type],
> mediastr, unit_id);
> }
> - qdict_put(bs_opts, "id", qstring_from_str(new_id));
> - g_free(new_id);
> + qdict_put(bs_opts, "id", qstring_wrap_str(new_id));
> }
>
> /* Add virtio block device */
> diff --git a/qobject/qstring.c b/qobject/qstring.c
> index 7a438e9..a64373a 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -66,6 +66,26 @@ QString *qstring_from_str(const char *str)
> return qstring_from_substr(str, 0, strlen(str) - 1);
> }
>
> +/**
> + * qstring_wrap_str(): Create a new QString around a malloc'd C string
> + *
> + * Returns a strong reference, and caller must not use @str any more.
> + * @str must not be NULL.
> + */
> +QString *qstring_wrap_str(char *str)
> +{
> + QString *qstring;
> +
> + qstring = g_malloc(sizeof(*qstring));
> + qobject_init(QOBJECT(qstring), QTYPE_QSTRING);
> +
> + assert(str);
> + qstring->string = str;
> + qstring->capacity = qstring->length = strlen(str);
> +
> + return qstring;
> +}
> +
> static void capacity_increase(QString *qstring, size_t len)
> {
> if (qstring->capacity < (qstring->length + len)) {
> diff --git a/tests/check-qstring.c b/tests/check-qstring.c
> index 11823c2..0c427c7 100644
> --- a/tests/check-qstring.c
> +++ b/tests/check-qstring.c
> @@ -34,6 +34,20 @@ static void qstring_from_str_test(void)
> QDECREF(qstring);
> }
>
> +static void qstring_wrap_str_test(void)
> +{
> + QString *qstring;
> + char *str = g_strdup("QEMU");
> +
> + qstring = qstring_wrap_str(str);
> + g_assert(qstring != NULL);
> + g_assert(qstring->base.refcnt == 1);
> + g_assert(strcmp("QEMU", qstring->string) == 0);
> + g_assert(qobject_type(QOBJECT(qstring)) == QTYPE_QSTRING);
> +
> + QDECREF(qstring);
> +}
> +
> static void qstring_destroy_test(void)
> {
> QString *qstring = qstring_from_str("destroy test");
> @@ -119,6 +133,7 @@ int main(int argc, char **argv)
> g_test_init(&argc, &argv, NULL);
>
> g_test_add_func("/public/from_str", qstring_from_str_test);
> + g_test_add_func("/public/wrap_str", qstring_wrap_str_test);
> g_test_add_func("/public/destroy", qstring_destroy_test);
> g_test_add_func("/public/get_str", qstring_get_str_test);
> g_test_add_func("/public/append_chr", qstring_append_chr_test);
> --
> 2.7.4
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/15] qstring: Add qstring_wrap_str()
2016-10-11 11:08 ` Marc-André Lureau
@ 2016-10-11 15:04 ` Eric Blake
2016-10-11 15:05 ` Eric Blake
0 siblings, 1 reply; 24+ messages in thread
From: Eric Blake @ 2016-10-11 15:04 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: Kevin Wolf, Chrysostomos Nanakos, open list:Block layer core,
Jeff Cody, armbru, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]
On 10/11/2016 06:08 AM, Marc-André Lureau wrote:
>> +++ b/block.c
>> @@ -1640,7 +1640,8 @@ static BlockDriverState
>> *bdrv_append_temp_snapshot(BlockDriverState *bs,
>> qdict_put(snapshot_options, "file.driver",
>> qstring_from_str("file"));
>> qdict_put(snapshot_options, "file.filename",
>> - qstring_from_str(tmp_filename));
>> + qstring_wrap_str(tmp_filename));
>> + tmp_filename = NULL;
>> qdict_put(snapshot_options, "driver",
>> qstring_from_str("qcow2"));
>>
>>
> You could also remove g_free(tmp_filename) from the normal return path
> (this may please static analyzers).
No. g_free(NULL) is safe, but we can also reach the 'out' label with
tmp_filename still malloc'd prior to the place where we transfer it
here, so the g_free() in the cleanup label is still required. The
assignment to NULL here prevents a double free. The patch is correct as-is.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/15] qstring: Add qstring_wrap_str()
2016-10-11 15:04 ` Eric Blake
@ 2016-10-11 15:05 ` Eric Blake
2016-10-11 15:25 ` Marc-André Lureau
0 siblings, 1 reply; 24+ messages in thread
From: Eric Blake @ 2016-10-11 15:05 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: Kevin Wolf, Chrysostomos Nanakos, open list:Block layer core,
Jeff Cody, armbru, Max Reitz
[-- Attachment #1: Type: text/plain, Size: 1410 bytes --]
On 10/11/2016 10:04 AM, Eric Blake wrote:
> On 10/11/2016 06:08 AM, Marc-André Lureau wrote:
>
>>> +++ b/block.c
>>> @@ -1640,7 +1640,8 @@ static BlockDriverState
>>> *bdrv_append_temp_snapshot(BlockDriverState *bs,
>>> qdict_put(snapshot_options, "file.driver",
>>> qstring_from_str("file"));
>>> qdict_put(snapshot_options, "file.filename",
>>> - qstring_from_str(tmp_filename));
>>> + qstring_wrap_str(tmp_filename));
>>> + tmp_filename = NULL;
>>> qdict_put(snapshot_options, "driver",
>>> qstring_from_str("qcow2"));
>>>
>>>
>> You could also remove g_free(tmp_filename) from the normal return path
>> (this may please static analyzers).
>
> No. g_free(NULL) is safe, but we can also reach the 'out' label with
> tmp_filename still malloc'd prior to the place where we transfer it
> here, so the g_free() in the cleanup label is still required. The
> assignment to NULL here prevents a double free. The patch is correct as-is.
Spoke too soon. I see what you're saying - the normal return path now
has a dead g_free(NULL). It won't cause any grief to the static
analyzers, but it is a useless no-op function call, so I can indeed trim
it (the one before the label, not the one after).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/15] qstring: Add qstring_wrap_str()
2016-10-11 15:05 ` Eric Blake
@ 2016-10-11 15:25 ` Marc-André Lureau
0 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2016-10-11 15:25 UTC (permalink / raw)
To: Eric Blake, qemu-devel
Cc: Kevin Wolf, Chrysostomos Nanakos, open list:Block layer core,
Jeff Cody, armbru, Max Reitz
Hi
On Tue, Oct 11, 2016 at 7:05 PM Eric Blake <eblake@redhat.com> wrote:
> On 10/11/2016 10:04 AM, Eric Blake wrote:
> > On 10/11/2016 06:08 AM, Marc-André Lureau wrote:
> >
> >>> +++ b/block.c
> >>> @@ -1640,7 +1640,8 @@ static BlockDriverState
> >>> *bdrv_append_temp_snapshot(BlockDriverState *bs,
> >>> qdict_put(snapshot_options, "file.driver",
> >>> qstring_from_str("file"));
> >>> qdict_put(snapshot_options, "file.filename",
> >>> - qstring_from_str(tmp_filename));
> >>> + qstring_wrap_str(tmp_filename));
> >>> + tmp_filename = NULL;
> >>> qdict_put(snapshot_options, "driver",
> >>> qstring_from_str("qcow2"));
> >>>
> >>>
> >> You could also remove g_free(tmp_filename) from the normal return path
> >> (this may please static analyzers).
> >
> > No. g_free(NULL) is safe, but we can also reach the 'out' label with
> > tmp_filename still malloc'd prior to the place where we transfer it
> > here, so the g_free() in the cleanup label is still required. The
> > assignment to NULL here prevents a double free. The patch is correct
> as-is.
>
> Spoke too soon. I see what you're saying - the normal return path now
> has a dead g_free(NULL). It won't cause any grief to the static
> analyzers, but it is a useless no-op function call, so I can indeed trim
> it (the one before the label, not the one after).
>
static analyzers could catch the fact that you always call g_free(NULL) in
this code path, please remove it.
> --
> Eric Blake eblake redhat com +1-919-301-3266 <+1%20919-301-3266>
> Libvirt virtualization library http://libvirt.org
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v6 08.5/15] fixup! qstring: Add qstring_wrap_str()
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 08/15] qstring: Add qstring_wrap_str() Eric Blake
2016-10-11 11:08 ` Marc-André Lureau
@ 2016-10-11 15:24 ` Eric Blake
1 sibling, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-11 15:24 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, armbru, marcandre.lureau, Kevin Wolf, Max Reitz
Signed-off-by: Eric Blake <eblake@redhat.com>
---
squash this in to address Marc-Andre's finding on a harmless
extra g_free.
---
block.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index 16d5981..6f00fd4 100644
--- a/block.c
+++ b/block.c
@@ -1605,7 +1605,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
char *tmp_filename = g_malloc0(PATH_MAX + 1);
int64_t total_size;
QemuOpts *opts = NULL;
- BlockDriverState *bs_snapshot;
+ BlockDriverState *bs_snapshot = NULL;
int ret;
/* if snapshot, we create a temporary backing file and open it
@@ -1658,13 +1658,10 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
bdrv_ref(bs_snapshot);
bdrv_append(bs_snapshot, bs);
+out:
+ QDECREF(snapshot_options);
g_free(tmp_filename);
return bs_snapshot;
-
-out:
- QDECREF(snapshot_options);
- g_free(tmp_filename);
- return NULL;
}
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v6 09/15] qobject: Consolidate qobject_to_json() calls
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (7 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 08/15] qstring: Add qstring_wrap_str() Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 10/15] tests: Test qobject_to_json() pretty formatting Eric Blake
` (6 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: armbru, Kevin Wolf, Max Reitz, Dr. David Alan Gilbert,
Michael Roth, open list:Block layer core
It's simpler to have a single conversion function that takes a
bool parameter, rather than two functions where the choice of
function determines an internal bool. Similar to commit fc471c18.
While at it, the conversion currently cannot fail (maybe it SHOULD
be possible to choose to fail, when encountering invalid UTF-8
encoding or an Infinity or NaN valued double, but that's a story
for another day), so clean up callers to avoid a needless assert.
And use bool rather than int for 'pretty'.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase
[no v5 due to series split]
v4: new patch
---
include/qapi/qmp/qjson.h | 4 ++--
block.c | 2 +-
monitor.c | 4 +---
qemu-img.c | 9 +++------
qga/main.c | 5 +----
qobject/qjson.c | 19 +++++--------------
tests/check-qjson.c | 27 +++++++++++----------------
tests/libqtest.c | 2 +-
tests/test-visitor-serialization.c | 2 +-
9 files changed, 26 insertions(+), 48 deletions(-)
diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 6fb912e..4b0e2b4 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -21,8 +21,8 @@ QObject *qobject_from_json(const char *string);
QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
QObject *qobject_from_jsonv(const char *string, va_list *ap) GCC_FMT_ATTR(1, 0);
-QString *qobject_to_json(const QObject *obj);
-QString *qobject_to_json_pretty(const QObject *obj);
+/* Convert the object to QString; does not fail. */
+QString *qobject_to_json(const QObject *obj, bool pretty);
int qstring_append_json_string(QString *qstring, const char *str);
void qstring_append_json_number(QString *qstring, double number);
diff --git a/block.c b/block.c
index 8a2876f..ad1422f 100644
--- a/block.c
+++ b/block.c
@@ -4021,7 +4021,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
if (bs->exact_filename[0]) {
pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
} else if (bs->full_open_options) {
- QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
+ QString *json = qobject_to_json(QOBJECT(bs->full_open_options), false);
snprintf(bs->filename, sizeof(bs->filename), "json:%s",
qstring_get_str(json));
QDECREF(json);
diff --git a/monitor.c b/monitor.c
index aed0d0b..cd51f8d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -388,9 +388,7 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
{
QString *json;
- json = mon->flags & MONITOR_USE_PRETTY ? qobject_to_json_pretty(data) :
- qobject_to_json(data);
- assert(json != NULL);
+ json = qobject_to_json(data, mon->flags & MONITOR_USE_PRETTY);
qstring_append_chr(json, '\n');
monitor_puts(mon, qstring_get_str(json));
diff --git a/qemu-img.c b/qemu-img.c
index ceffefe..626781d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -504,8 +504,7 @@ static void dump_json_image_check(ImageCheck *check, bool quiet)
visit_type_ImageCheck(v, NULL, &check, &error_abort);
visit_complete(v, &obj);
- str = qobject_to_json_pretty(obj);
- assert(str != NULL);
+ str = qobject_to_json(obj, true);
qprintf(quiet, "%s\n", qstring_get_str(str));
qobject_decref(obj);
visit_free(v);
@@ -2197,8 +2196,7 @@ static void dump_json_image_info_list(ImageInfoList *list)
visit_type_ImageInfoList(v, NULL, &list, &error_abort);
visit_complete(v, &obj);
- str = qobject_to_json_pretty(obj);
- assert(str != NULL);
+ str = qobject_to_json(obj, true);
printf("%s\n", qstring_get_str(str));
qobject_decref(obj);
visit_free(v);
@@ -2213,8 +2211,7 @@ static void dump_json_image_info(ImageInfo *info)
visit_type_ImageInfo(v, NULL, &info, &error_abort);
visit_complete(v, &obj);
- str = qobject_to_json_pretty(obj);
- assert(str != NULL);
+ str = qobject_to_json(obj, true);
printf("%s\n", qstring_get_str(str));
qobject_decref(obj);
visit_free(v);
diff --git a/qga/main.c b/qga/main.c
index 0b9d04e..2b04003 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -522,10 +522,7 @@ static int send_response(GAState *s, QObject *payload)
g_assert(payload && s->channel);
- payload_qstr = qobject_to_json(payload);
- if (!payload_qstr) {
- return -EINVAL;
- }
+ payload_qstr = qobject_to_json(payload, false);
if (s->delimit_response) {
s->delimit_response = false;
diff --git a/qobject/qjson.c b/qobject/qjson.c
index a705aba..d45048b 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -72,12 +72,12 @@ QObject *qobject_from_jsonf(const char *string, ...)
typedef struct ToJsonIterState
{
int indent;
- int pretty;
+ bool pretty;
int count;
QString *str;
} ToJsonIterState;
-static void to_json(const QObject *obj, QString *str, int pretty, int indent);
+static void to_json(const QObject *obj, QString *str, bool pretty, int indent);
static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
{
@@ -114,7 +114,7 @@ static void to_json_list_iter(QObject *obj, void *opaque)
s->count++;
}
-static void to_json(const QObject *obj, QString *str, int pretty, int indent)
+static void to_json(const QObject *obj, QString *str, bool pretty, int indent)
{
switch (qobject_type(obj)) {
case QTYPE_QNULL:
@@ -185,20 +185,11 @@ static void to_json(const QObject *obj, QString *str, int pretty, int indent)
}
}
-QString *qobject_to_json(const QObject *obj)
+QString *qobject_to_json(const QObject *obj, bool pretty)
{
QString *str = qstring_new();
- to_json(obj, str, 0, 0);
-
- return str;
-}
-
-QString *qobject_to_json_pretty(const QObject *obj)
-{
- QString *str = qstring_new();
-
- to_json(obj, str, 1, 0);
+ to_json(obj, str, pretty, 0);
return str;
}
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 8595574..3f02494 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -62,7 +62,7 @@ static void escaped_string(void)
g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].decoded);
if (test_cases[i].skip == 0) {
- str = qobject_to_json(obj);
+ str = qobject_to_json(obj, false);
g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].encoded);
qobject_decref(obj);
}
@@ -96,7 +96,7 @@ static void simple_string(void)
str = qobject_to_qstring(obj);
g_assert(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
- str = qobject_to_json(obj);
+ str = qobject_to_json(obj, false);
g_assert(strcmp(qstring_get_str(str), test_cases[i].encoded) == 0);
qobject_decref(obj);
@@ -830,13 +830,8 @@ static void utf8_string(void)
qobject_decref(obj);
obj = QOBJECT(qstring_from_str(utf8_in));
- str = qobject_to_json(obj);
- if (json_out) {
- g_assert(str);
- g_assert_cmpstr(qstring_get_str(str), ==, json_out);
- } else {
- g_assert(!str);
- }
+ str = qobject_to_json(obj, false);
+ g_assert_cmpstr(qstring_get_str(str), ==, json_out);
QDECREF(str);
qobject_decref(obj);
@@ -911,7 +906,7 @@ static void simple_number(void)
if (test_cases[i].skip == 0) {
QString *str;
- str = qobject_to_json(obj);
+ str = qobject_to_json(obj, false);
g_assert(strcmp(qstring_get_str(str), test_cases[i].encoded) == 0);
QDECREF(str);
}
@@ -949,7 +944,7 @@ static void float_number(void)
if (test_cases[i].skip == 0) {
QString *str;
- str = qobject_to_json(obj);
+ str = qobject_to_json(obj, false);
g_assert(strcmp(qstring_get_str(str), test_cases[i].encoded) == 0);
QDECREF(str);
}
@@ -1009,7 +1004,7 @@ static void keyword_literal(void)
qbool = qobject_to_qbool(obj);
g_assert(qbool_get_bool(qbool) == true);
- str = qobject_to_json(obj);
+ str = qobject_to_json(obj, false);
g_assert(strcmp(qstring_get_str(str), "true") == 0);
QDECREF(str);
@@ -1022,7 +1017,7 @@ static void keyword_literal(void)
qbool = qobject_to_qbool(obj);
g_assert(qbool_get_bool(qbool) == false);
- str = qobject_to_json(obj);
+ str = qobject_to_json(obj, false);
g_assert(strcmp(qstring_get_str(str), "false") == 0);
QDECREF(str);
@@ -1189,7 +1184,7 @@ static void simple_dict(void)
g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
- str = qobject_to_json(obj);
+ str = qobject_to_json(obj, false);
qobject_decref(obj);
obj = qobject_from_json(qstring_get_str(str));
@@ -1304,7 +1299,7 @@ static void simple_list(void)
g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
- str = qobject_to_json(obj);
+ str = qobject_to_json(obj, false);
qobject_decref(obj);
obj = qobject_from_json(qstring_get_str(str));
@@ -1372,7 +1367,7 @@ static void simple_whitespace(void)
g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
- str = qobject_to_json(obj);
+ str = qobject_to_json(obj, false);
qobject_decref(obj);
obj = qobject_from_json(qstring_get_str(str));
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 6f6bdf1..e1f8cb5 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -420,7 +420,7 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
/* No need to send anything for an empty QObject. */
if (qobj) {
int log = getenv("QTEST_LOG") != NULL;
- QString *qstr = qobject_to_json(qobj);
+ QString *qstr = qobject_to_json(qobj, false);
const char *str = qstring_get_str(qstr);
size_t size = qstring_get_length(qstr);
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index dba4670..13bb9ef 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -1036,7 +1036,7 @@ static void qmp_deserialize(void **native_out, void *datap,
visit_complete(d->qov, &d->obj);
obj_orig = d->obj;
- output_json = qobject_to_json(obj_orig);
+ output_json = qobject_to_json(obj_orig, false);
obj = qobject_from_json(qstring_get_str(output_json));
QDECREF(output_json);
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 10/15] tests: Test qobject_to_json() pretty formatting
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (8 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 09/15] qobject: Consolidate qobject_to_json() calls Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 11/15] qapi: Add JSON output visitor Eric Blake
` (5 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
It's risky to refactor qobject_to_json() without at least testing
that pretty output remains unchanged :)
Note that the new simple_pretty() test is a bit sensitive to our
current notion of prettiness, as well as to the hash ordering in
QDict (most of the tests in check-qobject-json intentionally do
not compare the original string to the round-trip string, because
we liberally accept more input forms than the canonical form that
we output).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: no change
[no v5 due to series split]
v4: new patch, split from v3 12/18
---
tests/check-qjson.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/tests/check-qjson.c b/tests/check-qjson.c
index 3f02494..2214906 100644
--- a/tests/check-qjson.c
+++ b/tests/check-qjson.c
@@ -1,6 +1,6 @@
/*
* Copyright IBM, Corp. 2009
- * Copyright (c) 2013, 2015 Red Hat Inc.
+ * Copyright (c) 2013-2016 Red Hat Inc.
*
* Authors:
* Anthony Liguori <aliguori@us.ibm.com>
@@ -1381,6 +1381,69 @@ static void simple_whitespace(void)
}
}
+static void simple_pretty(void)
+{
+ int i;
+ struct {
+ const char *encoded;
+ LiteralQObject decoded;
+ } test_cases[] = {
+ {
+ .encoded =
+ "[\n"
+ " 43,\n"
+ " 42\n"
+ "]",
+ .decoded = QLIT_QLIST(((LiteralQObject[]){
+ QLIT_QINT(43),
+ QLIT_QINT(42),
+ { }
+ })),
+ },
+ {
+ .encoded =
+ "[\n"
+ " 43,\n"
+ " {\n"
+ " \"a\": 32,\n"
+ " \"h\": \"b\"\n"
+ " },\n"
+ " [\n"
+ " ],\n"
+ " 42\n"
+ "]",
+ .decoded = QLIT_QLIST(((LiteralQObject[]){
+ QLIT_QINT(43),
+ QLIT_QDICT(((LiteralQDictEntry[]){
+ { "a", QLIT_QINT(32) },
+ { "h", QLIT_QSTR("b") },
+ { } })),
+ QLIT_QLIST(((LiteralQObject[]){
+ { } })),
+ QLIT_QINT(42),
+ { }
+ })),
+ },
+ { }
+ };
+
+ for (i = 0; test_cases[i].encoded; i++) {
+ QObject *obj;
+ QString *str;
+
+ obj = qobject_from_json(test_cases[i].encoded);
+ g_assert(obj != NULL);
+ g_assert(qobject_type(obj) == QTYPE_QLIST);
+
+ g_assert(compare_litqobj_to_qobj(&test_cases[i].decoded, obj) == 1);
+
+ str = qobject_to_json(obj, true);
+ qobject_decref(obj);
+ g_assert_cmpstr(qstring_get_str(str), ==, test_cases[i].encoded);
+ QDECREF(str);
+ }
+}
+
static void simple_varargs(void)
{
QObject *embedded_obj;
@@ -1518,6 +1581,7 @@ int main(int argc, char **argv)
g_test_add_func("/lists/simple_list", simple_list);
g_test_add_func("/whitespace/simple_whitespace", simple_whitespace);
+ g_test_add_func("/whitespace/simple_pretty", simple_pretty);
g_test_add_func("/varargs/simple_varargs", simple_varargs);
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 11/15] qapi: Add JSON output visitor
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (9 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 10/15] tests: Test qobject_to_json() pretty formatting Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 12/15] qapi: Support pretty printing in " Eric Blake
` (4 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We have several places that want to go from qapi to JSON; right now,
they have to create an intermediate QObject to do the work. That
also has the drawback that the JSON formatting of a QDict will
rearrange keys (according to a deterministic, but unpredictable,
hash), when humans have an easier time if dicts are produced in
the same order as the qapi type.
For these reasons, it is time to add a new JSON output visitor.
This patch just adds the basic visitor and tests that it works;
later patches will add pretty-printing, support for visit_type_any(),
and conversion of clients to use the visitor.
Design choices: Unlike the QMP output visitor, the JSON visitor
refuses to visit a required string with a NULL value, via abort().
Reusing QString to grow the contents means that we easily share
code with qjson.c; although it might be nice to enhance things to
take an optional output callback function so that the output can
truly be streamed instead of collected in memory. visit_complete()
can be called at most once, so that we can take advantage of
transfer semantics into the caller's pointer saved in jov->result.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: tweak commit message, adjust includes
[no v5 due to series split]
v4: retitle, split off inf/NaN handling, rebase to visit_complete
and visit_free changes, defer type_any, address other findings from
Markus
v3: retitle, rebase to master, minor cleanups
v2: rebase to qapi subset E v8; add test of error outputting
infinity; use unsigned depth
---
include/qapi/visitor.h | 22 +--
include/qapi/json-output-visitor.h | 28 +++
qapi/json-output-visitor.c | 193 +++++++++++++++++++
tests/test-json-output-visitor.c | 383 +++++++++++++++++++++++++++++++++++++
tests/test-qmp-output-visitor.c | 5 +
qapi/Makefile.objs | 2 +-
tests/.gitignore | 1 +
tests/Makefile.include | 5 +-
8 files changed, 626 insertions(+), 13 deletions(-)
create mode 100644 include/qapi/json-output-visitor.h
create mode 100644 qapi/json-output-visitor.c
create mode 100644 tests/test-json-output-visitor.c
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 4dd7532..d26e0e1 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -27,17 +27,17 @@
*
* There are four kinds of visitor classes: input visitors (QMP,
* string, and QemuOpts) parse an external representation and build
- * the corresponding QAPI graph, output visitors (QMP and string) take
- * a completed QAPI graph and generate an external representation, the
- * dealloc visitor can take a QAPI graph (possibly partially
- * constructed) and recursively free its resources, and the clone
- * visitor performs a deep clone of one QAPI object to another. While
- * the dealloc and QMP input/output visitors are general, the string,
- * QemuOpts, and clone visitors have some implementation limitations;
- * see the documentation for each visitor for more details on what it
- * supports. Also, see visitor-impl.h for the callback contracts
- * implemented by each visitor, and docs/qapi-code-gen.txt for more
- * about the QAPI code generator.
+ * the corresponding QAPI graph, output visitors (QMP, string, and
+ * JSON) take a completed QAPI graph and generate an external
+ * representation, the dealloc visitor can take a QAPI graph (possibly
+ * partially constructed) and recursively free its resources, and the
+ * clone visitor performs a deep clone of one QAPI object to another.
+ * While the dealloc, JSON output, and QMP input/output visitors are
+ * general, the string, QemuOpts, and clone visitors have some
+ * implementation limitations; see the documentation for each visitor
+ * for more details on what it supports. Also, see visitor-impl.h for
+ * the callback contracts implemented by each visitor, and
+ * docs/qapi-code-gen.txt for more about the QAPI code generator.
*
* All of the visitors are created via:
*
diff --git a/include/qapi/json-output-visitor.h b/include/qapi/json-output-visitor.h
new file mode 100644
index 0000000..41c79f4
--- /dev/null
+++ b/include/qapi/json-output-visitor.h
@@ -0,0 +1,28 @@
+/*
+ * JSON Output Visitor
+ *
+ * Copyright (C) 2015-2016 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef JSON_OUTPUT_VISITOR_H
+#define JSON_OUTPUT_VISITOR_H
+
+#include "qapi/visitor.h"
+
+typedef struct JsonOutputVisitor JsonOutputVisitor;
+
+/*
+ * Create a new JSON output visitor.
+ *
+ * If everything else succeeds, pass @result to visit_complete() to
+ * collect the result of the visit.
+ *
+ * For now, this cannot be used to visit the 'any' type.
+ */
+Visitor *json_output_visitor_new(char **result);
+
+#endif
diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
new file mode 100644
index 0000000..805154a
--- /dev/null
+++ b/qapi/json-output-visitor.c
@@ -0,0 +1,193 @@
+/*
+ * Convert QAPI to JSON
+ *
+ * Copyright (C) 2015-2016 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/json-output-visitor.h"
+#include "qapi/visitor-impl.h"
+#include "qapi/qmp/qstring.h"
+#include "qapi/qmp/qjson.h"
+
+struct JsonOutputVisitor {
+ Visitor visitor;
+ QString *str;
+ bool comma;
+ unsigned int depth;
+ char **result;
+};
+
+static JsonOutputVisitor *to_jov(Visitor *v)
+{
+ return container_of(v, JsonOutputVisitor, visitor);
+}
+
+static void json_output_name(JsonOutputVisitor *jov, const char *name)
+{
+ if (jov->comma) {
+ qstring_append(jov->str, ", ");
+ } else {
+ jov->comma = true;
+ }
+ if (name && jov->depth) {
+ qstring_append_json_string(jov->str, name);
+ qstring_append(jov->str, ": ");
+ }
+}
+
+static void json_output_start_struct(Visitor *v, const char *name, void **obj,
+ size_t unused, Error **errp)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ json_output_name(jov, name);
+ qstring_append(jov->str, "{");
+ jov->comma = false;
+ jov->depth++;
+}
+
+static void json_output_end_struct(Visitor *v, void **obj)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ assert(jov->depth);
+ jov->depth--;
+ qstring_append(jov->str, "}");
+ jov->comma = true;
+}
+
+static void json_output_start_list(Visitor *v, const char *name,
+ GenericList **listp, size_t size,
+ Error **errp)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ json_output_name(jov, name);
+ qstring_append(jov->str, "[");
+ jov->comma = false;
+ jov->depth++;
+}
+
+static GenericList *json_output_next_list(Visitor *v, GenericList *tail,
+ size_t size)
+{
+ return tail->next;
+}
+
+static void json_output_end_list(Visitor *v, void **obj)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ assert(jov->depth);
+ jov->depth--;
+ qstring_append(jov->str, "]");
+ jov->comma = true;
+}
+
+static void json_output_type_int64(Visitor *v, const char *name, int64_t *obj,
+ Error **errp)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ json_output_name(jov, name);
+ qstring_append_printf(jov->str, "%" PRId64, *obj);
+}
+
+static void json_output_type_uint64(Visitor *v, const char *name,
+ uint64_t *obj, Error **errp)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ json_output_name(jov, name);
+ qstring_append_printf(jov->str, "%" PRIu64, *obj);
+}
+
+static void json_output_type_bool(Visitor *v, const char *name, bool *obj,
+ Error **errp)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ json_output_name(jov, name);
+ qstring_append(jov->str, *obj ? "true" : "false");
+}
+
+static void json_output_type_str(Visitor *v, const char *name, char **obj,
+ Error **errp)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ assert(*obj);
+ json_output_name(jov, name);
+ /* FIXME: report invalid UTF-8 encoding */
+ qstring_append_json_string(jov->str, *obj);
+}
+
+static void json_output_type_number(Visitor *v, const char *name, double *obj,
+ Error **errp)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ json_output_name(jov, name);
+ /* FIXME: report Inf/NaN problems */
+ qstring_append_json_number(jov->str, *obj);
+}
+
+static void json_output_type_null(Visitor *v, const char *name, Error **errp)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ json_output_name(jov, name);
+ qstring_append(jov->str, "null");
+}
+
+static void json_output_complete(Visitor *v, void *result)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ assert(!jov->depth);
+ assert(qstring_get_length(jov->str));
+ assert(jov->result == result);
+ *jov->result = qstring_consume_str(jov->str);
+ jov->str = NULL;
+ jov->result = NULL;
+}
+
+static void json_output_free(Visitor *v)
+{
+ JsonOutputVisitor *jov = to_jov(v);
+
+ QDECREF(jov->str);
+ g_free(jov);
+}
+
+Visitor *json_output_visitor_new(char **result)
+{
+ JsonOutputVisitor *v;
+
+ v = g_malloc0(sizeof(*v));
+ v->result = result;
+ *result = NULL;
+ v->str = qstring_new();
+
+ v->visitor.type = VISITOR_OUTPUT;
+ v->visitor.start_struct = json_output_start_struct;
+ v->visitor.end_struct = json_output_end_struct;
+ v->visitor.start_list = json_output_start_list;
+ v->visitor.next_list = json_output_next_list;
+ v->visitor.end_list = json_output_end_list;
+ v->visitor.type_int64 = json_output_type_int64;
+ v->visitor.type_uint64 = json_output_type_uint64;
+ v->visitor.type_bool = json_output_type_bool;
+ v->visitor.type_str = json_output_type_str;
+ v->visitor.type_number = json_output_type_number;
+ v->visitor.type_null = json_output_type_null;
+ v->visitor.complete = json_output_complete;
+ v->visitor.free = json_output_free;
+
+ return &v->visitor;
+}
diff --git a/tests/test-json-output-visitor.c b/tests/test-json-output-visitor.c
new file mode 100644
index 0000000..3c77a61
--- /dev/null
+++ b/tests/test-json-output-visitor.c
@@ -0,0 +1,383 @@
+/*
+ * JSON Output Visitor unit-tests.
+ *
+ * Copyright (C) 2015-2016 Red Hat Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+/*
+ * Similar in design to test-qmp-output-visitor; if you add tests
+ * here, consider adding tests there as well.
+ */
+
+#include "qemu/osdep.h"
+#include <glib.h>
+
+#include "qemu-common.h"
+#include "qapi/json-output-visitor.h"
+#include "test-qapi-types.h"
+#include "test-qapi-visit.h"
+#include "qapi/qmp/types.h"
+#include "qapi/error.h"
+
+typedef struct TestOutputVisitorData {
+ Visitor *ov;
+ char *str;
+} TestOutputVisitorData;
+
+static void visitor_output_setup(TestOutputVisitorData *data,
+ const void *unused)
+{
+ data->ov = json_output_visitor_new(&data->str);
+ g_assert(data->ov);
+}
+
+static void visitor_output_teardown(TestOutputVisitorData *data,
+ const void *unused)
+{
+ visit_free(data->ov);
+ data->ov = NULL;
+ g_free(data->str);
+ data->str = NULL;
+}
+
+static const char *visitor_get(TestOutputVisitorData *data)
+{
+ visit_complete(data->ov, &data->str);
+ g_assert(data->str);
+ return data->str;
+}
+
+static void visitor_reset(TestOutputVisitorData *data)
+{
+ visitor_output_teardown(data, NULL);
+ visitor_output_setup(data, NULL);
+}
+
+static void test_visitor_out_int(TestOutputVisitorData *data,
+ const void *unused)
+{
+ int64_t value = -42;
+ const char *out;
+
+ visit_type_int(data->ov, NULL, &value, &error_abort);
+
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==, "-42");
+}
+
+static void test_visitor_out_bool(TestOutputVisitorData *data,
+ const void *unused)
+{
+ bool value = true;
+ const char *out;
+
+ visit_type_bool(data->ov, NULL, &value, &error_abort);
+
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==, "true");
+}
+
+static void test_visitor_out_number(TestOutputVisitorData *data,
+ const void *unused)
+{
+ double value = 3.14;
+ const char *out;
+
+ visit_type_number(data->ov, NULL, &value, &error_abort);
+
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==, "3.14");
+
+ /* FIXME: JSON requires finite numbers */
+}
+
+static void test_visitor_out_string(TestOutputVisitorData *data,
+ const void *unused)
+{
+ char *string = (char *) "Q E M U";
+ const char *out;
+
+ visit_type_str(data->ov, NULL, &string, &error_abort);
+
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==, "\"Q E M U\"");
+}
+
+static void test_visitor_out_enum(TestOutputVisitorData *data,
+ const void *unused)
+{
+ const char *out;
+ EnumOne i;
+ size_t len;
+
+ for (i = 0; i < ENUM_ONE__MAX; i++) {
+ visit_type_EnumOne(data->ov, "unused", &i, &error_abort);
+
+ out = visitor_get(data);
+ g_assert(*out == '"');
+ len = strlen(out);
+ g_assert_cmpint(len, >, 2);
+ g_assert(out[len - 1] == '"');
+ g_assert_cmpint(memcmp(out + 1, EnumOne_lookup[i], len - 2), ==, 0);
+ visitor_reset(data);
+ }
+}
+
+static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
+ const void *unused)
+{
+ EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
+ Error *err;
+
+ for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
+ err = NULL;
+ visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
+ error_free_or_abort(&err);
+ }
+}
+
+
+static void test_visitor_out_struct(TestOutputVisitorData *data,
+ const void *unused)
+{
+ TestStruct test_struct = { .integer = 42,
+ .boolean = false,
+ .string = (char *) "foo"};
+ TestStruct *p = &test_struct;
+ const char *out;
+
+ visit_type_TestStruct(data->ov, NULL, &p, &error_abort);
+
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==,
+ "{"
+ "\"integer\": 42, "
+ "\"boolean\": false, "
+ "\"string\": \"foo\""
+ "}");
+}
+
+static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
+ const void *unused)
+{
+ int64_t value = 42;
+ UserDefTwo *ud2;
+ const char *string = "user def string";
+ const char *strings[] = { "forty two", "forty three", "forty four",
+ "forty five" };
+ const char *out;
+
+ ud2 = g_malloc0(sizeof(*ud2));
+ ud2->string0 = g_strdup(strings[0]);
+
+ ud2->dict1 = g_malloc0(sizeof(*ud2->dict1));
+ ud2->dict1->string1 = g_strdup(strings[1]);
+
+ ud2->dict1->dict2 = g_malloc0(sizeof(*ud2->dict1->dict2));
+ ud2->dict1->dict2->userdef = g_new0(UserDefOne, 1);
+ ud2->dict1->dict2->userdef->string = g_strdup(string);
+ ud2->dict1->dict2->userdef->integer = value;
+ ud2->dict1->dict2->string = g_strdup(strings[2]);
+
+ ud2->dict1->dict3 = g_malloc0(sizeof(*ud2->dict1->dict3));
+ ud2->dict1->has_dict3 = true;
+ ud2->dict1->dict3->userdef = g_new0(UserDefOne, 1);
+ ud2->dict1->dict3->userdef->string = g_strdup(string);
+ ud2->dict1->dict3->userdef->integer = value;
+ ud2->dict1->dict3->string = g_strdup(strings[3]);
+
+ visit_type_UserDefTwo(data->ov, "unused", &ud2, &error_abort);
+
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==,
+ "{"
+ "\"string0\": \"forty two\", "
+ "\"dict1\": {"
+ "\"string1\": \"forty three\", "
+ "\"dict2\": {"
+ "\"userdef\": {"
+ "\"integer\": 42, "
+ "\"string\": \"user def string\""
+ "}, "
+ "\"string\": \"forty four\""
+ "}, "
+ "\"dict3\": {"
+ "\"userdef\": {"
+ "\"integer\": 42, "
+ "\"string\": \"user def string\""
+ "}, "
+ "\"string\": \"forty five\""
+ "}"
+ "}"
+ "}");
+ qapi_free_UserDefTwo(ud2);
+}
+
+static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
+ const void *unused)
+{
+ EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
+ UserDefOne u = { .string = (char *)"" };
+ UserDefOne *pu = &u;
+ Error *err;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(bad_values); i++) {
+ err = NULL;
+ u.has_enum1 = true;
+ u.enum1 = bad_values[i];
+ visit_type_UserDefOne(data->ov, "unused", &pu, &err);
+ error_free_or_abort(&err);
+ visitor_reset(data);
+ }
+}
+
+
+static void test_visitor_out_list(TestOutputVisitorData *data,
+ const void *unused)
+{
+ const char *value_str = "list value";
+ TestStructList *p, *head = NULL;
+ const int max_items = 10;
+ bool value_bool = true;
+ int value_int = 10;
+ int i;
+ const char *out;
+
+ for (i = 0; i < max_items; i++) {
+ p = g_malloc0(sizeof(*p));
+ p->value = g_malloc0(sizeof(*p->value));
+ p->value->integer = value_int + (max_items - i - 1);
+ p->value->boolean = value_bool;
+ p->value->string = g_strdup(value_str);
+
+ p->next = head;
+ head = p;
+ }
+
+ visit_type_TestStructList(data->ov, NULL, &head, &error_abort);
+
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==,
+ "["
+ "{\"integer\": 10, \"boolean\": true, "
+ "\"string\": \"list value\"}, "
+ "{\"integer\": 11, \"boolean\": true, "
+ "\"string\": \"list value\"}, "
+ "{\"integer\": 12, \"boolean\": true, "
+ "\"string\": \"list value\"}, "
+ "{\"integer\": 13, \"boolean\": true, "
+ "\"string\": \"list value\"}, "
+ "{\"integer\": 14, \"boolean\": true, "
+ "\"string\": \"list value\"}, "
+ "{\"integer\": 15, \"boolean\": true, "
+ "\"string\": \"list value\"}, "
+ "{\"integer\": 16, \"boolean\": true, "
+ "\"string\": \"list value\"}, "
+ "{\"integer\": 17, \"boolean\": true, "
+ "\"string\": \"list value\"}, "
+ "{\"integer\": 18, \"boolean\": true, "
+ "\"string\": \"list value\"}, "
+ "{\"integer\": 19, \"boolean\": true, "
+ "\"string\": \"list value\"}"
+ "]");
+ qapi_free_TestStructList(head);
+}
+
+static void test_visitor_out_union_flat(TestOutputVisitorData *data,
+ const void *unused)
+{
+ const char *out;
+ UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
+
+ tmp->enum1 = ENUM_ONE_VALUE1;
+ tmp->string = g_strdup("str");
+ tmp->integer = 41;
+ tmp->u.value1.boolean = true;
+
+ visit_type_UserDefFlatUnion(data->ov, NULL, &tmp, &error_abort);
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==,
+ "{"
+ "\"integer\": 41, "
+ "\"string\": \"str\", "
+ "\"enum1\": \"value1\", "
+ "\"boolean\": true"
+ "}");
+ qapi_free_UserDefFlatUnion(tmp);
+}
+
+static void test_visitor_out_alternate(TestOutputVisitorData *data,
+ const void *unused)
+{
+ UserDefAlternate *tmp;
+ const char *out;
+
+ tmp = g_new0(UserDefAlternate, 1);
+ tmp->type = QTYPE_QINT;
+ tmp->u.i = 42;
+
+ visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==, "42");
+ qapi_free_UserDefAlternate(tmp);
+
+ visitor_reset(data);
+ tmp = g_new0(UserDefAlternate, 1);
+ tmp->type = QTYPE_QSTRING;
+ tmp->u.s = g_strdup("hello");
+
+ visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==, "\"hello\"");
+ qapi_free_UserDefAlternate(tmp);
+}
+
+static void test_visitor_out_null(TestOutputVisitorData *data,
+ const void *unused)
+{
+ const char *out;
+
+ visit_type_null(data->ov, NULL, &error_abort);
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==, "null");
+}
+
+static void output_visitor_test_add(const char *testpath,
+ void (*test_func)(TestOutputVisitorData *,
+ const void *))
+{
+ g_test_add(testpath, TestOutputVisitorData, NULL, visitor_output_setup,
+ test_func, visitor_output_teardown);
+}
+
+int main(int argc, char **argv)
+{
+ g_test_init(&argc, &argv, NULL);
+
+ output_visitor_test_add("/visitor/json/int", test_visitor_out_int);
+ output_visitor_test_add("/visitor/json/bool", test_visitor_out_bool);
+ output_visitor_test_add("/visitor/json/number", test_visitor_out_number);
+ output_visitor_test_add("/visitor/json/string", test_visitor_out_string);
+ output_visitor_test_add("/visitor/json/enum", test_visitor_out_enum);
+ output_visitor_test_add("/visitor/json/enum-errors",
+ test_visitor_out_enum_errors);
+ output_visitor_test_add("/visitor/json/struct", test_visitor_out_struct);
+ output_visitor_test_add("/visitor/json/struct-nested",
+ test_visitor_out_struct_nested);
+ output_visitor_test_add("/visitor/json/struct-errors",
+ test_visitor_out_struct_errors);
+ output_visitor_test_add("/visitor/json/list", test_visitor_out_list);
+ output_visitor_test_add("/visitor/json/union-flat",
+ test_visitor_out_union_flat);
+ output_visitor_test_add("/visitor/json/alternate",
+ test_visitor_out_alternate);
+ output_visitor_test_add("/visitor/json/null", test_visitor_out_null);
+
+ g_test_run();
+
+ return 0;
+}
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 513d71f..7d23591 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -10,6 +10,11 @@
* See the COPYING file in the top-level directory.
*/
+/*
+ * Similar in design to test-json-output-visitor; if you add tests
+ * here, consider adding tests there as well.
+ */
+
#include "qemu/osdep.h"
#include "qemu-common.h"
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index 7ea4aeb..0a08492 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -1,6 +1,6 @@
util-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
util-obj-y += string-input-visitor.o string-output-visitor.o
-util-obj-y += opts-visitor.o qapi-clone-visitor.o
+util-obj-y += opts-visitor.o qapi-clone-visitor.o json-output-visitor.o
util-obj-y += qmp-event.o
util-obj-y += qapi-util.o
diff --git a/tests/.gitignore b/tests/.gitignore
index 0f0c79b..b54a91f 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -42,6 +42,7 @@ test-io-channel-file.txt
test-io-channel-socket
test-io-channel-tls
test-io-task
+test-json-output-visitor
test-logging
test-mul64
test-opts-visitor
diff --git a/tests/Makefile.include b/tests/Makefile.include
index a77777c..3b81dc3 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -24,6 +24,8 @@ check-unit-y += tests/test-qmp-output-visitor$(EXESUF)
gcov-files-test-qmp-output-visitor-y = qapi/qmp-output-visitor.c
check-unit-y += tests/test-clone-visitor$(EXESUF)
gcov-files-test-clone-visitor-y = qapi/qapi-clone-visitor.c
+check-unit-y += tests/test-json-output-visitor$(EXESUF)
+gcov-files-test-json-output-visitor-y = qapi/json-output-visitor.c
check-unit-y += tests/test-qmp-input-visitor$(EXESUF)
gcov-files-test-qmp-input-visitor-y = qapi/qmp-input-visitor.c
check-unit-y += tests/test-qmp-input-strict$(EXESUF)
@@ -447,7 +449,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
tests/check-qjson.o \
tests/test-coroutine.o tests/test-string-output-visitor.o \
tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
- tests/test-clone-visitor.o \
+ tests/test-clone-visitor.o tests/test-json-output-visitor.o \
tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
tests/test-qmp-commands.o tests/test-visitor-serialization.o \
tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
@@ -551,6 +553,7 @@ tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o $(te
tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o $(test-qapi-obj-y)
tests/test-qmp-output-visitor$(EXESUF): tests/test-qmp-output-visitor.o $(test-qapi-obj-y)
tests/test-clone-visitor$(EXESUF): tests/test-clone-visitor.o $(test-qapi-obj-y)
+tests/test-json-output-visitor$(EXESUF): tests/test-json-output-visitor.o $(test-qapi-obj-y)
tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qapi-obj-y)
tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y)
tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 12/15] qapi: Support pretty printing in JSON output visitor
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (10 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 11/15] qapi: Add JSON output visitor Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-11 11:20 ` Marc-André Lureau
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 13/15] qobject: Implement qobject_to_json() atop JSON visitor Eric Blake
` (3 subsequent siblings)
15 siblings, 1 reply; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Add pretty printing, where the format intentionally matches that of
qobject_to_json() (a later patch will then rework qobject-json.c to
work on top of the JSON visitor). The trickiest part is probably
that the testsuite now has to honor parameterization on whether
pretty printing is enabled.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: tweak commit message, use of 'bool pretty' in testsuite
[no v5 due to series split]
v4: rebase to earlier changes, defer type_any() to later
v3: rebase to earlier changes
v2: rebase to earlier changes
---
include/qapi/json-output-visitor.h | 5 +-
qapi/json-output-visitor.c | 15 ++++-
tests/test-json-output-visitor.c | 124 +++++++++++++++++++++++++------------
3 files changed, 101 insertions(+), 43 deletions(-)
diff --git a/include/qapi/json-output-visitor.h b/include/qapi/json-output-visitor.h
index 41c79f4..94c9e0f 100644
--- a/include/qapi/json-output-visitor.h
+++ b/include/qapi/json-output-visitor.h
@@ -21,8 +21,11 @@ typedef struct JsonOutputVisitor JsonOutputVisitor;
* If everything else succeeds, pass @result to visit_complete() to
* collect the result of the visit.
*
+ * If @pretty, make the output legible with newlines and indentation;
+ * otherwise the output uses a single line.
+ *
* For now, this cannot be used to visit the 'any' type.
*/
-Visitor *json_output_visitor_new(char **result);
+Visitor *json_output_visitor_new(bool pretty, char **result);
#endif
diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
index 805154a..7d12879 100644
--- a/qapi/json-output-visitor.c
+++ b/qapi/json-output-visitor.c
@@ -17,6 +17,7 @@
struct JsonOutputVisitor {
Visitor visitor;
QString *str;
+ bool pretty;
bool comma;
unsigned int depth;
char **result;
@@ -30,10 +31,13 @@ static JsonOutputVisitor *to_jov(Visitor *v)
static void json_output_name(JsonOutputVisitor *jov, const char *name)
{
if (jov->comma) {
- qstring_append(jov->str, ", ");
+ qstring_append(jov->str, jov->pretty ? "," : ", ");
} else {
jov->comma = true;
}
+ if (jov->pretty && jov->depth) {
+ qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, "");
+ }
if (name && jov->depth) {
qstring_append_json_string(jov->str, name);
qstring_append(jov->str, ": ");
@@ -57,6 +61,9 @@ static void json_output_end_struct(Visitor *v, void **obj)
assert(jov->depth);
jov->depth--;
+ if (jov->pretty) {
+ qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, "");
+ }
qstring_append(jov->str, "}");
jov->comma = true;
}
@@ -85,6 +92,9 @@ static void json_output_end_list(Visitor *v, void **obj)
assert(jov->depth);
jov->depth--;
+ if (jov->pretty) {
+ qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, "");
+ }
qstring_append(jov->str, "]");
jov->comma = true;
}
@@ -165,11 +175,12 @@ static void json_output_free(Visitor *v)
g_free(jov);
}
-Visitor *json_output_visitor_new(char **result)
+Visitor *json_output_visitor_new(bool pretty, char **result)
{
JsonOutputVisitor *v;
v = g_malloc0(sizeof(*v));
+ v->pretty = pretty;
v->result = result;
*result = NULL;
v->str = qstring_new();
diff --git a/tests/test-json-output-visitor.c b/tests/test-json-output-visitor.c
index 3c77a61..849cf2b 100644
--- a/tests/test-json-output-visitor.c
+++ b/tests/test-json-output-visitor.c
@@ -24,13 +24,17 @@
typedef struct TestOutputVisitorData {
Visitor *ov;
+ bool pretty;
char *str;
} TestOutputVisitorData;
static void visitor_output_setup(TestOutputVisitorData *data,
- const void *unused)
+ const void *arg)
{
- data->ov = json_output_visitor_new(&data->str);
+ bool pretty = *(bool *)arg;
+
+ data->pretty = pretty;
+ data->ov = json_output_visitor_new(pretty, &data->str);
g_assert(data->ov);
}
@@ -52,8 +56,10 @@ static const char *visitor_get(TestOutputVisitorData *data)
static void visitor_reset(TestOutputVisitorData *data)
{
+ bool pretty = data->pretty;
+
visitor_output_teardown(data, NULL);
- visitor_output_setup(data, NULL);
+ visitor_output_setup(data, &pretty);
}
static void test_visitor_out_int(TestOutputVisitorData *data,
@@ -161,8 +167,9 @@ static void test_visitor_out_struct(TestOutputVisitorData *data,
}
static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
- const void *unused)
+ const void *arg)
{
+ bool pretty = *(bool *)arg;
int64_t value = 42;
UserDefTwo *ud2;
const char *string = "user def string";
@@ -192,27 +199,51 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
visit_type_UserDefTwo(data->ov, "unused", &ud2, &error_abort);
out = visitor_get(data);
- g_assert_cmpstr(out, ==,
- "{"
- "\"string0\": \"forty two\", "
- "\"dict1\": {"
- "\"string1\": \"forty three\", "
- "\"dict2\": {"
- "\"userdef\": {"
- "\"integer\": 42, "
- "\"string\": \"user def string\""
- "}, "
- "\"string\": \"forty four\""
- "}, "
- "\"dict3\": {"
- "\"userdef\": {"
- "\"integer\": 42, "
- "\"string\": \"user def string\""
- "}, "
- "\"string\": \"forty five\""
- "}"
- "}"
- "}");
+ if (pretty) {
+ g_assert_cmpstr(out, ==,
+ "{\n"
+ " \"string0\": \"forty two\",\n"
+ " \"dict1\": {\n"
+ " \"string1\": \"forty three\",\n"
+ " \"dict2\": {\n"
+ " \"userdef\": {\n"
+ " \"integer\": 42,\n"
+ " \"string\": \"user def string\"\n"
+ " },\n"
+ " \"string\": \"forty four\"\n"
+ " },\n"
+ " \"dict3\": {\n"
+ " \"userdef\": {\n"
+ " \"integer\": 42,\n"
+ " \"string\": \"user def string\"\n"
+ " },\n"
+ " \"string\": \"forty five\"\n"
+ " }\n"
+ " }\n"
+ "}");
+ } else {
+ g_assert_cmpstr(out, ==,
+ "{"
+ "\"string0\": \"forty two\", "
+ "\"dict1\": {"
+ "\"string1\": \"forty three\", "
+ "\"dict2\": {"
+ "\"userdef\": {"
+ "\"integer\": 42, "
+ "\"string\": \"user def string\""
+ "}, "
+ "\"string\": \"forty four\""
+ "}, "
+ "\"dict3\": {"
+ "\"userdef\": {"
+ "\"integer\": 42, "
+ "\"string\": \"user def string\""
+ "}, "
+ "\"string\": \"forty five\""
+ "}"
+ "}"
+ "}");
+ }
qapi_free_UserDefTwo(ud2);
}
@@ -346,36 +377,49 @@ static void test_visitor_out_null(TestOutputVisitorData *data,
g_assert_cmpstr(out, ==, "null");
}
-static void output_visitor_test_add(const char *testpath,
+static void output_visitor_test_add(const char *testpath, bool *data,
void (*test_func)(TestOutputVisitorData *,
const void *))
{
- g_test_add(testpath, TestOutputVisitorData, NULL, visitor_output_setup,
+ g_test_add(testpath, TestOutputVisitorData, data, visitor_output_setup,
test_func, visitor_output_teardown);
}
int main(int argc, char **argv)
{
+ bool plain = false;
+ bool pretty = true;
+
g_test_init(&argc, &argv, NULL);
- output_visitor_test_add("/visitor/json/int", test_visitor_out_int);
- output_visitor_test_add("/visitor/json/bool", test_visitor_out_bool);
- output_visitor_test_add("/visitor/json/number", test_visitor_out_number);
- output_visitor_test_add("/visitor/json/string", test_visitor_out_string);
- output_visitor_test_add("/visitor/json/enum", test_visitor_out_enum);
- output_visitor_test_add("/visitor/json/enum-errors",
+ output_visitor_test_add("/visitor/json/int", &plain,
+ test_visitor_out_int);
+ output_visitor_test_add("/visitor/json/bool", &plain,
+ test_visitor_out_bool);
+ output_visitor_test_add("/visitor/json/number", &plain,
+ test_visitor_out_number);
+ output_visitor_test_add("/visitor/json/string", &plain,
+ test_visitor_out_string);
+ output_visitor_test_add("/visitor/json/enum", &plain,
+ test_visitor_out_enum);
+ output_visitor_test_add("/visitor/json/enum-errors", &plain,
test_visitor_out_enum_errors);
- output_visitor_test_add("/visitor/json/struct", test_visitor_out_struct);
- output_visitor_test_add("/visitor/json/struct-nested",
+ output_visitor_test_add("/visitor/json/struct", &plain,
+ test_visitor_out_struct);
+ output_visitor_test_add("/visitor/json/struct-nested", &plain,
test_visitor_out_struct_nested);
- output_visitor_test_add("/visitor/json/struct-errors",
+ output_visitor_test_add("/visitor/json-pretty/struct-nested", &pretty,
+ test_visitor_out_struct_nested);
+ output_visitor_test_add("/visitor/json/struct-errors", &plain,
test_visitor_out_struct_errors);
- output_visitor_test_add("/visitor/json/list", test_visitor_out_list);
- output_visitor_test_add("/visitor/json/union-flat",
+ output_visitor_test_add("/visitor/json/list", &plain,
+ test_visitor_out_list);
+ output_visitor_test_add("/visitor/json/union-flat", &plain,
test_visitor_out_union_flat);
- output_visitor_test_add("/visitor/json/alternate",
+ output_visitor_test_add("/visitor/json/alternate", &plain,
test_visitor_out_alternate);
- output_visitor_test_add("/visitor/json/null", test_visitor_out_null);
+ output_visitor_test_add("/visitor/json/null", &plain,
+ test_visitor_out_null);
g_test_run();
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v6 12/15] qapi: Support pretty printing in JSON output visitor
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 12/15] qapi: Support pretty printing in " Eric Blake
@ 2016-10-11 11:20 ` Marc-André Lureau
2016-10-11 15:09 ` Eric Blake
0 siblings, 1 reply; 24+ messages in thread
From: Marc-André Lureau @ 2016-10-11 11:20 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: armbru, Michael Roth
On Mon, Oct 10, 2016 at 5:44 PM Eric Blake <eblake@redhat.com> wrote:
> Add pretty printing, where the format intentionally matches that of
> qobject_to_json() (a later patch will then rework qobject-json.c to
> work on top of the JSON visitor). The trickiest part is probably
> that the testsuite now has to honor parameterization on whether
> pretty printing is enabled.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: tweak commit message, use of 'bool pretty' in testsuite
> [no v5 due to series split]
> v4: rebase to earlier changes, defer type_any() to later
> v3: rebase to earlier changes
> v2: rebase to earlier changes
> ---
> include/qapi/json-output-visitor.h | 5 +-
> qapi/json-output-visitor.c | 15 ++++-
> tests/test-json-output-visitor.c | 124
> +++++++++++++++++++++++++------------
> 3 files changed, 101 insertions(+), 43 deletions(-)
>
> diff --git a/include/qapi/json-output-visitor.h
> b/include/qapi/json-output-visitor.h
> index 41c79f4..94c9e0f 100644
> --- a/include/qapi/json-output-visitor.h
> +++ b/include/qapi/json-output-visitor.h
> @@ -21,8 +21,11 @@ typedef struct JsonOutputVisitor JsonOutputVisitor;
> * If everything else succeeds, pass @result to visit_complete() to
> * collect the result of the visit.
> *
> + * If @pretty, make the output legible with newlines and indentation;
> + * otherwise the output uses a single line.
> + *
> * For now, this cannot be used to visit the 'any' type.
> */
> -Visitor *json_output_visitor_new(char **result);
> +Visitor *json_output_visitor_new(bool pretty, char **result);
>
> #endif
> diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
> index 805154a..7d12879 100644
> --- a/qapi/json-output-visitor.c
> +++ b/qapi/json-output-visitor.c
> @@ -17,6 +17,7 @@
> struct JsonOutputVisitor {
> Visitor visitor;
> QString *str;
> + bool pretty;
> bool comma;
> unsigned int depth;
> char **result;
> @@ -30,10 +31,13 @@ static JsonOutputVisitor *to_jov(Visitor *v)
> static void json_output_name(JsonOutputVisitor *jov, const char *name)
> {
> if (jov->comma) {
> - qstring_append(jov->str, ", ");
> + qstring_append(jov->str, jov->pretty ? "," : ", ");
> } else {
> jov->comma = true;
> }
> + if (jov->pretty && jov->depth) {
> + qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, "");
> + }
> if (name && jov->depth) {
> qstring_append_json_string(jov->str, name);
> qstring_append(jov->str, ": ");
> @@ -57,6 +61,9 @@ static void json_output_end_struct(Visitor *v, void
> **obj)
>
> assert(jov->depth);
> jov->depth--;
> + if (jov->pretty) {
> + qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, "");
> + }
> qstring_append(jov->str, "}");
> jov->comma = true;
> }
> @@ -85,6 +92,9 @@ static void json_output_end_list(Visitor *v, void **obj)
>
> assert(jov->depth);
> jov->depth--;
> + if (jov->pretty) {
> + qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, "");
> + }
> qstring_append(jov->str, "]");
> jov->comma = true;
> }
> @@ -165,11 +175,12 @@ static void json_output_free(Visitor *v)
> g_free(jov);
> }
>
> -Visitor *json_output_visitor_new(char **result)
> +Visitor *json_output_visitor_new(bool pretty, char **result)
> {
> JsonOutputVisitor *v;
>
> v = g_malloc0(sizeof(*v));
> + v->pretty = pretty;
> v->result = result;
> *result = NULL;
> v->str = qstring_new();
> diff --git a/tests/test-json-output-visitor.c
> b/tests/test-json-output-visitor.c
> index 3c77a61..849cf2b 100644
> --- a/tests/test-json-output-visitor.c
> +++ b/tests/test-json-output-visitor.c
> @@ -24,13 +24,17 @@
>
> typedef struct TestOutputVisitorData {
> Visitor *ov;
> + bool pretty;
> char *str;
> } TestOutputVisitorData;
>
> static void visitor_output_setup(TestOutputVisitorData *data,
> - const void *unused)
> + const void *arg)
> {
> - data->ov = json_output_visitor_new(&data->str);
> + bool pretty = *(bool *)arg;
> +
>
Have you considered GPOINTER_TO_INT/GINT_TO_POINTER instead of pointers to
the stack frame?
either way, it's fine.
> + data->pretty = pretty;
> + data->ov = json_output_visitor_new(pretty, &data->str);
> g_assert(data->ov);
> }
>
> @@ -52,8 +56,10 @@ static const char *visitor_get(TestOutputVisitorData
> *data)
>
> static void visitor_reset(TestOutputVisitorData *data)
> {
> + bool pretty = data->pretty;
> +
> visitor_output_teardown(data, NULL);
> - visitor_output_setup(data, NULL);
> + visitor_output_setup(data, &pretty);
> }
>
> static void test_visitor_out_int(TestOutputVisitorData *data,
> @@ -161,8 +167,9 @@ static void
> test_visitor_out_struct(TestOutputVisitorData *data,
> }
>
> static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
> - const void *unused)
> + const void *arg)
> {
> + bool pretty = *(bool *)arg;
> int64_t value = 42;
> UserDefTwo *ud2;
> const char *string = "user def string";
> @@ -192,27 +199,51 @@ static void
> test_visitor_out_struct_nested(TestOutputVisitorData *data,
> visit_type_UserDefTwo(data->ov, "unused", &ud2, &error_abort);
>
> out = visitor_get(data);
> - g_assert_cmpstr(out, ==,
> - "{"
> - "\"string0\": \"forty two\", "
> - "\"dict1\": {"
> - "\"string1\": \"forty three\", "
> - "\"dict2\": {"
> - "\"userdef\": {"
> - "\"integer\": 42, "
> - "\"string\": \"user def string\""
> - "}, "
> - "\"string\": \"forty four\""
> - "}, "
> - "\"dict3\": {"
> - "\"userdef\": {"
> - "\"integer\": 42, "
> - "\"string\": \"user def string\""
> - "}, "
> - "\"string\": \"forty five\""
> - "}"
> - "}"
> - "}");
> + if (pretty) {
> + g_assert_cmpstr(out, ==,
> + "{\n"
> + " \"string0\": \"forty two\",\n"
> + " \"dict1\": {\n"
> + " \"string1\": \"forty three\",\n"
> + " \"dict2\": {\n"
> + " \"userdef\": {\n"
> + " \"integer\": 42,\n"
> + " \"string\": \"user def
> string\"\n"
> + " },\n"
> + " \"string\": \"forty four\"\n"
> + " },\n"
> + " \"dict3\": {\n"
> + " \"userdef\": {\n"
> + " \"integer\": 42,\n"
> + " \"string\": \"user def
> string\"\n"
> + " },\n"
> + " \"string\": \"forty five\"\n"
> + " }\n"
> + " }\n"
> + "}");
> + } else {
> + g_assert_cmpstr(out, ==,
> + "{"
> + "\"string0\": \"forty two\", "
> + "\"dict1\": {"
> + "\"string1\": \"forty three\", "
> + "\"dict2\": {"
> + "\"userdef\": {"
> + "\"integer\": 42, "
> + "\"string\": \"user def string\""
> + "}, "
> + "\"string\": \"forty four\""
> + "}, "
> + "\"dict3\": {"
> + "\"userdef\": {"
> + "\"integer\": 42, "
> + "\"string\": \"user def string\""
> + "}, "
> + "\"string\": \"forty five\""
> + "}"
> + "}"
> + "}");
> + }
> qapi_free_UserDefTwo(ud2);
> }
>
> @@ -346,36 +377,49 @@ static void
> test_visitor_out_null(TestOutputVisitorData *data,
> g_assert_cmpstr(out, ==, "null");
> }
>
> -static void output_visitor_test_add(const char *testpath,
> +static void output_visitor_test_add(const char *testpath, bool *data,
> void
> (*test_func)(TestOutputVisitorData *,
> const void *))
> {
> - g_test_add(testpath, TestOutputVisitorData, NULL,
> visitor_output_setup,
> + g_test_add(testpath, TestOutputVisitorData, data,
> visitor_output_setup,
> test_func, visitor_output_teardown);
> }
>
> int main(int argc, char **argv)
> {
> + bool plain = false;
> + bool pretty = true;
> +
> g_test_init(&argc, &argv, NULL);
>
> - output_visitor_test_add("/visitor/json/int", test_visitor_out_int);
> - output_visitor_test_add("/visitor/json/bool", test_visitor_out_bool);
> - output_visitor_test_add("/visitor/json/number",
> test_visitor_out_number);
> - output_visitor_test_add("/visitor/json/string",
> test_visitor_out_string);
> - output_visitor_test_add("/visitor/json/enum", test_visitor_out_enum);
> - output_visitor_test_add("/visitor/json/enum-errors",
> + output_visitor_test_add("/visitor/json/int", &plain,
> + test_visitor_out_int);
> + output_visitor_test_add("/visitor/json/bool", &plain,
> + test_visitor_out_bool);
> + output_visitor_test_add("/visitor/json/number", &plain,
> + test_visitor_out_number);
> + output_visitor_test_add("/visitor/json/string", &plain,
> + test_visitor_out_string);
> + output_visitor_test_add("/visitor/json/enum", &plain,
> + test_visitor_out_enum);
> + output_visitor_test_add("/visitor/json/enum-errors", &plain,
> test_visitor_out_enum_errors);
> - output_visitor_test_add("/visitor/json/struct",
> test_visitor_out_struct);
> - output_visitor_test_add("/visitor/json/struct-nested",
> + output_visitor_test_add("/visitor/json/struct", &plain,
> + test_visitor_out_struct);
> + output_visitor_test_add("/visitor/json/struct-nested", &plain,
> test_visitor_out_struct_nested);
> - output_visitor_test_add("/visitor/json/struct-errors",
> + output_visitor_test_add("/visitor/json-pretty/struct-nested", &pretty,
> + test_visitor_out_struct_nested);
> + output_visitor_test_add("/visitor/json/struct-errors", &plain,
> test_visitor_out_struct_errors);
> - output_visitor_test_add("/visitor/json/list", test_visitor_out_list);
> - output_visitor_test_add("/visitor/json/union-flat",
> + output_visitor_test_add("/visitor/json/list", &plain,
> + test_visitor_out_list);
> + output_visitor_test_add("/visitor/json/union-flat", &plain,
> test_visitor_out_union_flat);
> - output_visitor_test_add("/visitor/json/alternate",
> + output_visitor_test_add("/visitor/json/alternate", &plain,
> test_visitor_out_alternate);
> - output_visitor_test_add("/visitor/json/null", test_visitor_out_null);
> + output_visitor_test_add("/visitor/json/null", &plain,
> + test_visitor_out_null);
>
> g_test_run();
>
> --
> 2.7.4
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v6 12/15] qapi: Support pretty printing in JSON output visitor
2016-10-11 11:20 ` Marc-André Lureau
@ 2016-10-11 15:09 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-11 15:09 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: armbru, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]
On 10/11/2016 06:20 AM, Marc-André Lureau wrote:
> On Mon, Oct 10, 2016 at 5:44 PM Eric Blake <eblake@redhat.com> wrote:
>
>> Add pretty printing, where the format intentionally matches that of
>> qobject_to_json() (a later patch will then rework qobject-json.c to
>> work on top of the JSON visitor). The trickiest part is probably
>> that the testsuite now has to honor parameterization on whether
>> pretty printing is enabled.
>>
>>
>> static void visitor_output_setup(TestOutputVisitorData *data,
>> - const void *unused)
>> + const void *arg)
>> {
>> - data->ov = json_output_visitor_new(&data->str);
>> + bool pretty = *(bool *)arg;
>> +
>>
>
> Have you considered GPOINTER_TO_INT/GINT_TO_POINTER instead of pointers to
> the stack frame?
Casting integer values to void* and back requires a lot of typing to
pacify all compilers (either explicit casts, or casts hidden inside the
lengthy macro names from glib); and fails to work for 64-bit values when
void* is only 32 bits. Dereferencing a stack pointer is a lot less
thought and less typing, and works regardless of the integer size.
>
> either way, it's fine.
>
Good, I won't bother changing it.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v6 13/15] qobject: Implement qobject_to_json() atop JSON visitor
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (11 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 12/15] qapi: Support pretty printing in " Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 14/15] qapi: Add 'any' support to JSON output Eric Blake
` (2 subsequent siblings)
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
Rather than open-code two different JSON visitors, it's nicer to
make qobject_to_json() reuse the JSON output visitor. The JSON
output visitor allowed us to separate formatting from walking
the tree, where the tree walk is normally done by the
QAPI-generated code for a "real" visit. Now we can add another
tree walk of QObject, while reusing the formatting of the JSON
visitor, in a "virtual" visit.
Note that this virtual QObject tree walk can also be paired with
other visitors. The string output visitor isn't useful (it would
fail if structs are involved), but the (misnamed) QMP output
visitor could be used to perform a QObject deep clone. However,
I did not find any obvious QObject clones in the current code
base that needed to use this trick.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase, improve commit message, simplify iterators
[no v5 due to series split]
v4: new patch, inspired by discussion on v3 12/18
---
include/qapi/qmp/qjson.h | 9 ++++
qobject/qjson.c | 113 +++++++++++++----------------------------------
2 files changed, 40 insertions(+), 82 deletions(-)
diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 4b0e2b4..f8f4fcd 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -24,6 +24,15 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap) GCC_FMT_ATTR(1, 0);
/* Convert the object to QString; does not fail. */
QString *qobject_to_json(const QObject *obj, bool pretty);
+/*
+ * Visit the object with an output visitor @v.
+ *
+ * @name represents the relationship of this object to any parent
+ * (ignored for a top-level visit, must be set if part of a
+ * dictionary, should be NULL if part of a list).
+ */
+void qobject_visit_output(Visitor *v, const char *name, const QObject *obj);
+
int qstring_append_json_string(QString *qstring, const char *str);
void qstring_append_json_number(QString *qstring, double number);
diff --git a/qobject/qjson.c b/qobject/qjson.c
index d45048b..4ce4b9a 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -19,6 +19,9 @@
#include "qapi/qmp/qjson.h"
#include "qapi/qmp/types.h"
#include "qemu/unicode.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "qapi/json-output-visitor.h"
typedef struct JSONParsingState
{
@@ -69,115 +72,59 @@ QObject *qobject_from_jsonf(const char *string, ...)
return obj;
}
-typedef struct ToJsonIterState
-{
- int indent;
- bool pretty;
- int count;
- QString *str;
-} ToJsonIterState;
-
-static void to_json(const QObject *obj, QString *str, bool pretty, int indent);
-
-static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
-{
- ToJsonIterState *s = opaque;
-
- if (s->count) {
- qstring_append(s->str, s->pretty ? "," : ", ");
- }
-
- if (s->pretty) {
- qstring_append_printf(s->str, "\n%*s", 4 * s->indent, "");
- }
-
- qstring_append_json_string(s->str, key);
-
- qstring_append(s->str, ": ");
- to_json(obj, s->str, s->pretty, s->indent);
- s->count++;
-}
-
-static void to_json_list_iter(QObject *obj, void *opaque)
-{
- ToJsonIterState *s = opaque;
-
- if (s->count) {
- qstring_append(s->str, s->pretty ? "," : ", ");
- }
-
- if (s->pretty) {
- qstring_append_printf(s->str, "\n%*s", 4 * s->indent, "");
- }
-
- to_json(obj, s->str, s->pretty, s->indent);
- s->count++;
-}
-
-static void to_json(const QObject *obj, QString *str, bool pretty, int indent)
+void qobject_visit_output(Visitor *v, const char *name, const QObject *obj)
{
switch (qobject_type(obj)) {
case QTYPE_QNULL:
- qstring_append(str, "null");
+ visit_type_null(v, name, &error_abort);
break;
case QTYPE_QINT: {
- QInt *val = qobject_to_qint(obj);
- qstring_append_printf(str, "%" PRId64, qint_get_int(val));
+ int64_t val = qint_get_int(qobject_to_qint(obj));
+
+ visit_type_int64(v, name, &val, &error_abort);
break;
}
case QTYPE_QSTRING: {
- QString *val = qobject_to_qstring(obj);
+ const char *str = qstring_get_str(qobject_to_qstring(obj));
+
/* FIXME: no way inform user if we replaced invalid UTF-8
* sequences to avoid invalid JSON */
- qstring_append_json_string(str, qstring_get_str(val));
+ visit_type_str(v, name, (char **)&str, &error_abort);
break;
}
case QTYPE_QDICT: {
- ToJsonIterState s;
QDict *val = qobject_to_qdict(obj);
+ const QDictEntry *ent;
- s.count = 0;
- s.str = str;
- s.indent = indent + 1;
- s.pretty = pretty;
- qstring_append_chr(str, '{');
- qdict_iter(val, to_json_dict_iter, &s);
- if (pretty) {
- qstring_append_printf(str, "\n%*s", 4 * indent, "");
+ visit_start_struct(v, name, NULL, 0, &error_abort);
+ for (ent = qdict_first(val); ent; ent = qdict_next(val, ent)) {
+ qobject_visit_output(v, ent->key, ent->value);
}
- qstring_append_chr(str, '}');
+ visit_check_struct(v, &error_abort);
+ visit_end_struct(v, NULL);
break;
}
case QTYPE_QLIST: {
- ToJsonIterState s;
QList *val = qobject_to_qlist(obj);
+ const QListEntry *ent;
- s.count = 0;
- s.str = str;
- s.indent = indent + 1;
- s.pretty = pretty;
- qstring_append_chr(str, '[');
- qlist_iter(val, (void *)to_json_list_iter, &s);
- if (pretty) {
- qstring_append_printf(str, "\n%*s", 4 * indent, "");
+ visit_start_list(v, name, NULL, 0, &error_abort);
+ QTAILQ_FOREACH(ent, &val->head, next) {
+ qobject_visit_output(v, NULL, ent->value);
}
- qstring_append_chr(str, ']');
+ visit_end_list(v, NULL);
break;
}
case QTYPE_QFLOAT: {
double val = qfloat_get_double(qobject_to_qfloat(obj));
- qstring_append_json_number(str, val);
+ visit_type_number(v, name, &val, &error_abort);
break;
}
case QTYPE_QBOOL: {
- QBool *val = qobject_to_qbool(obj);
+ bool val = qbool_get_bool(qobject_to_qbool(obj));
- if (qbool_get_bool(val)) {
- qstring_append(str, "true");
- } else {
- qstring_append(str, "false");
- }
+ visit_type_bool(v, name, &val, &error_abort);
break;
}
default:
@@ -187,11 +134,13 @@ static void to_json(const QObject *obj, QString *str, bool pretty, int indent)
QString *qobject_to_json(const QObject *obj, bool pretty)
{
- QString *str = qstring_new();
+ char *str;
+ Visitor *v = json_output_visitor_new(pretty, &str);
- to_json(obj, str, pretty, 0);
-
- return str;
+ qobject_visit_output(v, NULL, obj);
+ visit_complete(v, &str);
+ visit_free(v);
+ return qstring_wrap_str(str);
}
/**
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 14/15] qapi: Add 'any' support to JSON output
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (12 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 13/15] qobject: Implement qobject_to_json() atop JSON visitor Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 15/15] qemu-img: Use new JSON output formatter Eric Blake
2016-10-11 12:14 ` [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Marc-André Lureau
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Now that we can visit any QObject, it's easy to add support
for visit_type_any() in the JSON output visitor.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: no change
[no v5 due to series split]
v4: new patch, split out of v3 7/18, but made simpler by
not requiring v3 12/18
---
include/qapi/json-output-visitor.h | 2 --
qapi/json-output-visitor.c | 7 ++++++
tests/test-json-output-visitor.c | 50 ++++++++++++++++++++++++++++++++++++++
3 files changed, 57 insertions(+), 2 deletions(-)
diff --git a/include/qapi/json-output-visitor.h b/include/qapi/json-output-visitor.h
index 94c9e0f..414f91b 100644
--- a/include/qapi/json-output-visitor.h
+++ b/include/qapi/json-output-visitor.h
@@ -23,8 +23,6 @@ typedef struct JsonOutputVisitor JsonOutputVisitor;
*
* If @pretty, make the output legible with newlines and indentation;
* otherwise the output uses a single line.
- *
- * For now, this cannot be used to visit the 'any' type.
*/
Visitor *json_output_visitor_new(bool pretty, char **result);
diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
index 7d12879..5f229d2 100644
--- a/qapi/json-output-visitor.c
+++ b/qapi/json-output-visitor.c
@@ -147,6 +147,12 @@ static void json_output_type_number(Visitor *v, const char *name, double *obj,
qstring_append_json_number(jov->str, *obj);
}
+static void json_output_type_any(Visitor *v, const char *name, QObject **obj,
+ Error **errp)
+{
+ qobject_visit_output(v, name, *obj);
+}
+
static void json_output_type_null(Visitor *v, const char *name, Error **errp)
{
JsonOutputVisitor *jov = to_jov(v);
@@ -196,6 +202,7 @@ Visitor *json_output_visitor_new(bool pretty, char **result)
v->visitor.type_bool = json_output_type_bool;
v->visitor.type_str = json_output_type_str;
v->visitor.type_number = json_output_type_number;
+ v->visitor.type_any = json_output_type_any;
v->visitor.type_null = json_output_type_null;
v->visitor.complete = json_output_complete;
v->visitor.free = json_output_free;
diff --git a/tests/test-json-output-visitor.c b/tests/test-json-output-visitor.c
index 849cf2b..a802a12 100644
--- a/tests/test-json-output-visitor.c
+++ b/tests/test-json-output-visitor.c
@@ -318,6 +318,52 @@ static void test_visitor_out_list(TestOutputVisitorData *data,
qapi_free_TestStructList(head);
}
+static void test_visitor_out_any(TestOutputVisitorData *data,
+ const void *arg)
+{
+ const bool *pretty = arg;
+ QObject *qobj;
+ QDict *qdict;
+ const char *out;
+
+ qobj = QOBJECT(qint_from_int(-42));
+ visit_type_any(data->ov, NULL, &qobj, &error_abort);
+ out = visitor_get(data);
+ g_assert_cmpstr(out, ==, "-42");
+ qobject_decref(qobj);
+
+ visitor_reset(data);
+ qdict = qdict_new();
+ /* Ordering here is sensitive to the hashing chosen by QDict. */
+ qdict_put(qdict, "integer", qint_from_int(-42));
+ qdict_put(qdict, "list", qlist_new());
+ qdict_put(qdict, "boolean", qbool_from_bool(true));
+ qdict_put(qdict, "string", qstring_from_str("foo"));
+ qobj = QOBJECT(qdict);
+ visit_type_any(data->ov, NULL, &qobj, &error_abort);
+ qobject_decref(qobj);
+ out = visitor_get(data);
+
+ if (*pretty) {
+ g_assert_cmpstr(out, ==,
+ "{\n"
+ " \"integer\": -42,\n"
+ " \"list\": [\n"
+ " ],\n"
+ " \"boolean\": true,\n"
+ " \"string\": \"foo\"\n"
+ "}");
+ } else {
+ g_assert_cmpstr(out, ==,
+ "{"
+ "\"integer\": -42, "
+ "\"list\": [], "
+ "\"boolean\": true, "
+ "\"string\": \"foo\""
+ "}");
+ }
+}
+
static void test_visitor_out_union_flat(TestOutputVisitorData *data,
const void *unused)
{
@@ -414,6 +460,10 @@ int main(int argc, char **argv)
test_visitor_out_struct_errors);
output_visitor_test_add("/visitor/json/list", &plain,
test_visitor_out_list);
+ output_visitor_test_add("/visitor/json/any", &plain,
+ test_visitor_out_any);
+ output_visitor_test_add("/visitor/json-pretty/any", &pretty,
+ test_visitor_out_any);
output_visitor_test_add("/visitor/json/union-flat", &plain,
test_visitor_out_union_flat);
output_visitor_test_add("/visitor/json/alternate", &plain,
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Qemu-devel] [PATCH v6 15/15] qemu-img: Use new JSON output formatter
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (13 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 14/15] qapi: Add 'any' support to JSON output Eric Blake
@ 2016-10-10 13:23 ` Eric Blake
2016-10-11 12:14 ` [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Marc-André Lureau
15 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2016-10-10 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Kevin Wolf, Max Reitz, open list:Block layer core
Now that we can pretty-print straight to JSON from a visitor,
we can eliminate the temporary conversion into QObject inside
qemu-img.
The changes to qemu-iotests 043 expected output demonstrates
the fact that output is now done in qapi declaration order,
rather than QDict hash order.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase
[no v5 due to series split]
v4: rebase to earlier changes
v3: rebase to master
v2: Drop RFC, adjust expected output of 043; rebase to 'name' motion
Overlaps with Fam's qemu-img edits, although he has expressed
interest in getting this one in first.
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01756.html
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01806.html
---
qemu-img.c | 43 +++++++++++++++++--------------------------
tests/qemu-iotests/043.out | 22 +++++++++++-----------
2 files changed, 28 insertions(+), 37 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 626781d..97a5401 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -25,9 +25,9 @@
#include "qemu-version.h"
#include "qapi/error.h"
#include "qapi-visit.h"
-#include "qapi/qmp-output-visitor.h"
+#include "qapi/json-output-visitor.h"
#include "qapi/qmp/qerror.h"
-#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/types.h"
#include "qemu/cutils.h"
#include "qemu/config-file.h"
#include "qemu/option.h"
@@ -498,17 +498,14 @@ fail:
static void dump_json_image_check(ImageCheck *check, bool quiet)
{
- QString *str;
- QObject *obj;
- Visitor *v = qmp_output_visitor_new(&obj);
+ char *str;
+ Visitor *v = json_output_visitor_new(true, &str);
visit_type_ImageCheck(v, NULL, &check, &error_abort);
- visit_complete(v, &obj);
- str = qobject_to_json(obj, true);
- qprintf(quiet, "%s\n", qstring_get_str(str));
- qobject_decref(obj);
+ visit_complete(v, &str);
+ qprintf(quiet, "%s\n", str);
+ g_free(str);
visit_free(v);
- QDECREF(str);
}
static void dump_human_image_check(ImageCheck *check, bool quiet)
@@ -2190,32 +2187,26 @@ static void dump_snapshots(BlockDriverState *bs)
static void dump_json_image_info_list(ImageInfoList *list)
{
- QString *str;
- QObject *obj;
- Visitor *v = qmp_output_visitor_new(&obj);
+ char *str;
+ Visitor *v = json_output_visitor_new(true, &str);
visit_type_ImageInfoList(v, NULL, &list, &error_abort);
- visit_complete(v, &obj);
- str = qobject_to_json(obj, true);
- printf("%s\n", qstring_get_str(str));
- qobject_decref(obj);
+ visit_complete(v, &str);
+ printf("%s\n", str);
visit_free(v);
- QDECREF(str);
+ g_free(str);
}
static void dump_json_image_info(ImageInfo *info)
{
- QString *str;
- QObject *obj;
- Visitor *v = qmp_output_visitor_new(&obj);
+ char *str;
+ Visitor *v = json_output_visitor_new(true, &str);
visit_type_ImageInfo(v, NULL, &info, &error_abort);
- visit_complete(v, &obj);
- str = qobject_to_json(obj, true);
- printf("%s\n", qstring_get_str(str));
- qobject_decref(obj);
+ visit_complete(v, &str);
+ printf("%s\n", str);
visit_free(v);
- QDECREF(str);
+ g_free(str);
}
static void dump_human_image_info_list(ImageInfoList *list)
diff --git a/tests/qemu-iotests/043.out b/tests/qemu-iotests/043.out
index b37d2a3..41697a2 100644
--- a/tests/qemu-iotests/043.out
+++ b/tests/qemu-iotests/043.out
@@ -40,29 +40,29 @@ cluster_size: 65536
== finite chain of length 3 (json) ==
[
{
- "virtual-size": 134217728,
"filename": "TEST_DIR/t.IMGFMT",
- "cluster-size": 65536,
"format": "IMGFMT",
- "full-backing-filename": "TEST_DIR/t.IMGFMT.2.base",
+ "dirty-flag": false,
+ "virtual-size": 134217728,
+ "cluster-size": 65536,
"backing-filename": "TEST_DIR/t.IMGFMT.2.base",
- "dirty-flag": false
+ "full-backing-filename": "TEST_DIR/t.IMGFMT.2.base",
},
{
- "virtual-size": 134217728,
"filename": "TEST_DIR/t.IMGFMT.2.base",
- "cluster-size": 65536,
"format": "IMGFMT",
- "full-backing-filename": "TEST_DIR/t.IMGFMT.1.base",
+ "dirty-flag": false,
+ "virtual-size": 134217728,
+ "cluster-size": 65536,
"backing-filename": "TEST_DIR/t.IMGFMT.1.base",
- "dirty-flag": false
+ "full-backing-filename": "TEST_DIR/t.IMGFMT.1.base",
},
{
- "virtual-size": 134217728,
"filename": "TEST_DIR/t.IMGFMT.1.base",
- "cluster-size": 65536,
"format": "IMGFMT",
- "dirty-flag": false
+ "dirty-flag": false,
+ "virtual-size": 134217728,
+ "cluster-size": 65536,
}
]
*** done
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor
2016-10-10 13:23 [Qemu-devel] [PATCH v6 00/15] Add qapi-to-JSON visitor Eric Blake
` (14 preceding siblings ...)
2016-10-10 13:23 ` [Qemu-devel] [PATCH v6 15/15] qemu-img: Use new JSON output formatter Eric Blake
@ 2016-10-11 12:14 ` Marc-André Lureau
15 siblings, 0 replies; 24+ messages in thread
From: Marc-André Lureau @ 2016-10-11 12:14 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: armbru
Hi
On Mon, Oct 10, 2016 at 5:29 PM Eric Blake <eblake@redhat.com> wrote:
> [5 months later...]
> Going from qapi to QObject to JSON is wasteful compared to going
> straight from qapi to JSON. What's more, having QObject in the
> middle means that dict keys are shuffled according to hash ordering,
> while going straight from qapi means we match the .json QAPI file
> declaration order. Factoring out JSON visits to a common central
> file will make it easier to make any further tweaks, such as
> patching floating point output to be unambiguous.
>
> Based on Markus' qapi-next branch, but should apply to current
> qemu.git master.
>
> Diff from v4, which was at:
> https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg03220.html
> (v4 was both JSON and clone visitors; v5 split out
> just the clone visitor, leaving just the JSON visitor and hence
> calling this v6):
>
> - assert up front that we are no longer willing to receive or
> pass non-finite numbers through QMP
> - drop experiment related to string-izing nonfinite numbers
> - rebase to recent changes
> - address other minor review comments from Markus
>
> Available as a tag at:
> git fetch git://repo.or.cz/qemu/ericb.git qapi-jsonv6
>
> 001/15:[down] 'qapi: Visitor documentation tweak'
> 002/15:[down] 'qapi: Assert finite use of 'number''
> 003/15:[0013] [FC] 'qapi: Factor out JSON string escaping'
> 004/15:[0022] [FC] 'qapi: Factor out JSON number formatting'
> 005/15:[----] [--] 'qapi: Add qstring_append_printf()'
> 006/15:[----] [--] 'qapi: Use qstring_append_chr() where appropriate'
> 007/15:[----] [--] 'qstring: Add qstring_consume_str()'
> 008/15:[0022] [FC] 'qstring: Add qstring_wrap_str()'
> 009/15:[----] [-C] 'qobject: Consolidate qobject_to_json() calls'
> 010/15:[----] [--] 'tests: Test qobject_to_json() pretty formatting'
> 011/15:[0013] [FC] 'qapi: Add JSON output visitor'
> 012/15:[0012] [FC] 'qapi: Support pretty printing in JSON output visitor'
> 013/15:[0041] [FC] 'qobject: Implement qobject_to_json() atop JSON visitor'
> 014/15:[----] [--] 'qapi: Add 'any' support to JSON output'
> 015/15:[0002] [FC] 'qemu-img: Use new JSON output formatter'
>
>
> Eric Blake (15):
> qapi: Visitor documentation tweak
> qapi: Assert finite use of 'number'
> qapi: Factor out JSON string escaping
> qapi: Factor out JSON number formatting
> qapi: Add qstring_append_printf()
> qapi: Use qstring_append_chr() where appropriate
> qstring: Add qstring_consume_str()
> qstring: Add qstring_wrap_str()
> qobject: Consolidate qobject_to_json() calls
> tests: Test qobject_to_json() pretty formatting
> qapi: Add JSON output visitor
> qapi: Support pretty printing in JSON output visitor
> qobject: Implement qobject_to_json() atop JSON visitor
> qapi: Add 'any' support to JSON output
> qemu-img: Use new JSON output formatter
>
Except the minor comments I left in patch 8 and 12, the whole series is
good to me:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> include/qapi/visitor.h | 33 +--
> include/qapi/json-output-visitor.h | 29 +++
> include/qapi/qmp/qjson.h | 16 +-
> include/qapi/qmp/qstring.h | 9 +-
> block.c | 5 +-
> block/accounting.c | 6 +-
> block/archipelago.c | 6 +-
> blockdev.c | 3 +-
> migration/migration.c | 4 +
> migration/qjson.c | 12 +-
> monitor.c | 10 +-
> qapi/json-output-visitor.c | 211 ++++++++++++++++
> qemu-img.c | 46 ++--
> qga/main.c | 5 +-
> qobject/json-parser.c | 25 +-
> qobject/qjson.c | 301 ++++++++++-------------
> qobject/qstring.c | 69 +++++-
> qom/object.c | 3 +-
> tests/check-qjson.c | 93 ++++++--
> tests/check-qstring.c | 46 +++-
> tests/libqtest.c | 2 +-
> tests/test-json-output-visitor.c | 477
> +++++++++++++++++++++++++++++++++++++
> tests/test-qmp-output-visitor.c | 5 +
> tests/test-visitor-serialization.c | 2 +-
> qapi/Makefile.objs | 2 +-
> tests/.gitignore | 1 +
> tests/Makefile.include | 5 +-
> tests/qemu-iotests/043.out | 22 +-
> 28 files changed, 1137 insertions(+), 311 deletions(-)
> create mode 100644 include/qapi/json-output-visitor.h
> create mode 100644 qapi/json-output-visitor.c
> create mode 100644 tests/test-json-output-visitor.c
>
> --
> 2.7.4
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 24+ messages in thread