[libvirt] [PATCH 0/9] Properly implement json pseudo-protocol string deflattening
In qemu's new syntax starting from 2.9 it's possible to have nested objects in the json pseudo protocol string specifying a backing store for an image. Libvirt would not be able to handle some of them, since the json deflattener was meant to deflatten only 1 layer. Peter Krempa (9): tests: Rename jsontest to virjsontest util: json: Add virJSONValueIsObject util: Move JSON object deflattening code to json utility file util: json: Don't remove the 'file' subobject when deflattening tests: json: Add test for the deflattening function util: json: Properly implement JSON deflattening util: json: Recursively deflatten objects virJSONValueObjectDeflatten util: storage: Always deflatten JSON pseudo-protocol objects tests: Validate that JSON deflattening fixed nested json pseudo-protocol strings src/libvirt_private.syms | 2 + src/util/virjson.c | 115 +++++++++++++++++++++ src/util/virjson.h | 4 + src/util/virstoragefile.c | 79 ++------------ tests/Makefile.am | 8 +- tests/virjsondata/deflatten-basic-file-in.json | 8 ++ tests/virjsondata/deflatten-basic-file-out.json | 10 ++ tests/virjsondata/deflatten-basic-generic-in.json | 14 +++ tests/virjsondata/deflatten-basic-generic-out.json | 20 ++++ .../deflatten-concat-double-key-in.json | 7 ++ tests/virjsondata/deflatten-concat-in.json | 5 + tests/virjsondata/deflatten-concat-out.json | 8 ++ tests/virjsondata/deflatten-deep-file-in.json | 9 ++ tests/virjsondata/deflatten-deep-file-out.json | 23 +++++ tests/virjsondata/deflatten-deep-generic-in.json | 9 ++ tests/virjsondata/deflatten-deep-generic-out.json | 27 +++++ tests/virjsondata/deflatten-double-key-in.json | 4 + tests/virjsondata/deflatten-nested-in.json | 16 +++ tests/virjsondata/deflatten-nested-out.json | 28 +++++ tests/virjsondata/deflatten-qemu-sheepdog-in.json | 11 ++ tests/virjsondata/deflatten-qemu-sheepdog-out.json | 13 +++ tests/virjsondata/deflatten-unflattened-in.json | 12 +++ tests/virjsondata/deflatten-unflattened-out.json | 14 +++ tests/{jsontest.c => virjsontest.c} | 75 ++++++++++++++ tests/virstoragetest.c | 10 ++ 25 files changed, 458 insertions(+), 73 deletions(-) create mode 100644 tests/virjsondata/deflatten-basic-file-in.json create mode 100644 tests/virjsondata/deflatten-basic-file-out.json create mode 100644 tests/virjsondata/deflatten-basic-generic-in.json create mode 100644 tests/virjsondata/deflatten-basic-generic-out.json create mode 100644 tests/virjsondata/deflatten-concat-double-key-in.json create mode 100644 tests/virjsondata/deflatten-concat-in.json create mode 100644 tests/virjsondata/deflatten-concat-out.json create mode 100644 tests/virjsondata/deflatten-deep-file-in.json create mode 100644 tests/virjsondata/deflatten-deep-file-out.json create mode 100644 tests/virjsondata/deflatten-deep-generic-in.json create mode 100644 tests/virjsondata/deflatten-deep-generic-out.json create mode 100644 tests/virjsondata/deflatten-double-key-in.json create mode 100644 tests/virjsondata/deflatten-nested-in.json create mode 100644 tests/virjsondata/deflatten-nested-out.json create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-in.json create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-out.json create mode 100644 tests/virjsondata/deflatten-unflattened-in.json create mode 100644 tests/virjsondata/deflatten-unflattened-out.json rename tests/{jsontest.c => virjsontest.c} (89%) -- 2.12.2
--- tests/Makefile.am | 8 ++++---- tests/{jsontest.c => virjsontest.c} | 0 2 files changed, 4 insertions(+), 4 deletions(-) rename tests/{jsontest.c => virjsontest.c} (100%) diff --git a/tests/Makefile.am b/tests/Makefile.am index 19986dc99..3596b5ff1 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -328,7 +328,7 @@ test_programs += objectlocking endif WITH_CIL if WITH_YAJL -test_programs += jsontest +test_programs += virjsontest endif WITH_YAJL test_programs += \ @@ -1375,9 +1375,9 @@ virfirewalltest_SOURCES = \ virfirewalltest_LDADD = $(LDADDS) $(DBUS_LIBS) virfirewalltest_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS) -jsontest_SOURCES = \ - jsontest.c testutils.h testutils.c -jsontest_LDADD = $(LDADDS) +virjsontest_SOURCES = \ + virjsontest.c testutils.h testutils.c +virjsontest_LDADD = $(LDADDS) utiltest_SOURCES = \ utiltest.c testutils.h testutils.c diff --git a/tests/jsontest.c b/tests/virjsontest.c similarity index 100% rename from tests/jsontest.c rename to tests/virjsontest.c -- 2.12.2
On 06/27/2017 08:46 AM, Peter Krempa wrote:
--- tests/Makefile.am | 8 ++++---- tests/{jsontest.c => virjsontest.c} | 0 2 files changed, 4 insertions(+), 4 deletions(-) rename tests/{jsontest.c => virjsontest.c} (100%)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Allows testing whether a virJSONValue is an object. --- src/libvirt_private.syms | 1 + src/util/virjson.c | 10 ++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 13 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c1e9471c5..d487b1f43 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1905,6 +1905,7 @@ virJSONValueGetString; virJSONValueHashFree; virJSONValueIsArray; virJSONValueIsNull; +virJSONValueIsObject; virJSONValueNewArray; virJSONValueNewArrayFromBitmap; virJSONValueNewBoolean; diff --git a/src/util/virjson.c b/src/util/virjson.c index b49b29b0f..efd6c3a0e 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -905,6 +905,16 @@ virJSONValueObjectGetValue(virJSONValuePtr object, bool +virJSONValueIsObject(virJSONValuePtr object) +{ + if (object) + return object->type == VIR_JSON_TYPE_OBJECT; + else + return NULL; +} + + +bool virJSONValueIsArray(virJSONValuePtr array) { return array->type == VIR_JSON_TYPE_ARRAY; diff --git a/src/util/virjson.h b/src/util/virjson.h index 14b74c061..c9d9752de 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -114,6 +114,8 @@ virJSONValuePtr virJSONValueObjectGet(virJSONValuePtr object, const char *key); virJSONValuePtr virJSONValueObjectGetByType(virJSONValuePtr object, const char *key, virJSONType type); +bool virJSONValueIsObject(virJSONValuePtr object); + bool virJSONValueIsArray(virJSONValuePtr array); ssize_t virJSONValueArraySize(const virJSONValue *array); virJSONValuePtr virJSONValueArrayGet(virJSONValuePtr object, unsigned int element); -- 2.12.2
Peter Krempa <pkrempa@redhat.com> [2017-06-27, 02:46PM +0200]:
Allows testing whether a virJSONValue is an object. --- src/libvirt_private.syms | 1 + src/util/virjson.c | 10 ++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 13 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c1e9471c5..d487b1f43 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1905,6 +1905,7 @@ virJSONValueGetString; virJSONValueHashFree; virJSONValueIsArray; virJSONValueIsNull; +virJSONValueIsObject; virJSONValueNewArray; virJSONValueNewArrayFromBitmap; virJSONValueNewBoolean; diff --git a/src/util/virjson.c b/src/util/virjson.c index b49b29b0f..efd6c3a0e 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -905,6 +905,16 @@ virJSONValueObjectGetValue(virJSONValuePtr object,
bool +virJSONValueIsObject(virJSONValuePtr object) +{ + if (object) + return object->type == VIR_JSON_TYPE_OBJECT; + else + return NULL;
s/NULL/false/
+} + + +bool virJSONValueIsArray(virJSONValuePtr array) { return array->type == VIR_JSON_TYPE_ARRAY; diff --git a/src/util/virjson.h b/src/util/virjson.h index 14b74c061..c9d9752de 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -114,6 +114,8 @@ virJSONValuePtr virJSONValueObjectGet(virJSONValuePtr object, const char *key); virJSONValuePtr virJSONValueObjectGetByType(virJSONValuePtr object, const char *key, virJSONType type);
+bool virJSONValueIsObject(virJSONValuePtr object); + bool virJSONValueIsArray(virJSONValuePtr array); ssize_t virJSONValueArraySize(const virJSONValue *array); virJSONValuePtr virJSONValueArrayGet(virJSONValuePtr object, unsigned int element); -- 2.12.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- IBM Systems Linux on z Systems & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Schönaicher Str. 220 71032 Böblingen Phone: +49 7031 16 1819 E-Mail: bwalk@de.ibm.com ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Tue, Jun 27, 2017 at 17:55:40 +0200, Bjoern Walk wrote:
Peter Krempa <pkrempa@redhat.com> [2017-06-27, 02:46PM +0200]:
Allows testing whether a virJSONValue is an object. --- src/libvirt_private.syms | 1 + src/util/virjson.c | 10 ++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 13 insertions(+)
[...]
diff --git a/src/util/virjson.c b/src/util/virjson.c index b49b29b0f..efd6c3a0e 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -905,6 +905,16 @@ virJSONValueObjectGetValue(virJSONValuePtr object,
bool +virJSONValueIsObject(virJSONValuePtr object) +{ + if (object) + return object->type == VIR_JSON_TYPE_OBJECT; + else + return NULL;
s/NULL/false/
Thanks! It's a shame we don't have 'nullptr' like functionality in C.
On 06/27/2017 08:46 AM, Peter Krempa wrote:
Allows testing whether a virJSONValue is an object. --- src/libvirt_private.syms | 1 + src/util/virjson.c | 10 ++++++++++ src/util/virjson.h | 2 ++ 3 files changed, 13 insertions(+)
As long as you've made the change Bjoern points out... Reviewed-by: John Ferlan <jferlan@redhat.com> John
The code will become more universal so it makes more sense for it to live with the rest of the JSON functions. --- src/libvirt_private.syms | 1 + src/util/virjson.c | 61 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 2 ++ src/util/virstoragefile.c | 65 +---------------------------------------------- 4 files changed, 65 insertions(+), 64 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d487b1f43..9f64b8d93 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1931,6 +1931,7 @@ virJSONValueObjectAppendNumberUlong; virJSONValueObjectAppendString; virJSONValueObjectCreate; virJSONValueObjectCreateVArgs; +virJSONValueObjectDeflatten; virJSONValueObjectForeachKeyValue; virJSONValueObjectGet; virJSONValueObjectGetArray; diff --git a/src/util/virjson.c b/src/util/virjson.c index efd6c3a0e..8ab542432 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1965,3 +1965,64 @@ virJSONStringReformat(const char *jsonstr, virJSONValueFree(json); return ret; } + + +static int +virJSONValueObjectDeflattenWorker(const char *key, + virJSONValuePtr value, + void *opaque) +{ + virJSONValuePtr retobj = opaque; + virJSONValuePtr newval = NULL; + const char *newkey; + + if (!(newkey = STRSKIP(key, "file."))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("JSON object is neither nested nor flattened")); + return -1; + } + + if (!(newval = virJSONValueCopy(value))) + return -1; + + if (virJSONValueObjectAppend(retobj, newkey, newval) < 0) { + virJSONValueFree(newval); + return -1; + } + + return 0; +} + + +/** + * virJSONValueObjectDeflatten: + * + * In some cases it's possible to nest JSON objects by prefixing object members + * with the parent object name followed by the dot and then the attribute name + * rather than directly using a nested value object (e.g qemu's JSON + * pseudo-protocol in backing file definition). + * + * This function will attempt to reverse the process and provide a nested json + * hierarchy so that the parsers can be kept simple and we still can use the + * weird syntax some users might use. + * + * Currently this function will flatten out just the 'file.' prefix into a new + * tree. Any other syntax will be rejected. + */ +virJSONValuePtr +virJSONValueObjectDeflatten(virJSONValuePtr json) +{ + virJSONValuePtr ret; + + if (!(ret = virJSONValueNewObject())) + return NULL; + + if (virJSONValueObjectForeachKeyValue(json, + virJSONValueObjectDeflattenWorker, + ret) < 0) { + virJSONValueFree(ret); + return NULL; + } + + return ret; +} diff --git a/src/util/virjson.h b/src/util/virjson.h index c9d9752de..e89a776ab 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -186,4 +186,6 @@ virJSONValuePtr virJSONValueCopy(const virJSONValue *in); char *virJSONStringReformat(const char *jsonstr, bool pretty); +virJSONValuePtr virJSONValueObjectDeflatten(virJSONValuePtr json); + #endif /* __VIR_JSON_H_ */ diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f0ed5c6bd..52c5301ff 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3244,69 +3244,6 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = { }; -static int -virStorageSourceParseBackingJSONDeflattenWorker(const char *key, - virJSONValuePtr value, - void *opaque) -{ - virJSONValuePtr retobj = opaque; - virJSONValuePtr newval = NULL; - const char *newkey; - - if (!(newkey = STRSKIP(key, "file."))) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("JSON backing file syntax is neither nested nor " - "flattened")); - return -1; - } - - if (!(newval = virJSONValueCopy(value))) - return -1; - - if (virJSONValueObjectAppend(retobj, newkey, newval) < 0) { - virJSONValueFree(newval); - return -1; - } - - return 0; -} - - -/** - * virStorageSourceParseBackingJSONDeflatten: - * - * The json: pseudo-protocol syntax in qemu allows multiple approaches to - * describe nesting of the values. This is due to the lax handling of the string - * in qemu and the fact that internally qemu is flattening the values using '.'. - * - * This allows to specify nested json strings either using nested json objects - * or prefixing object members with the parent object name followed by the dot. - * - * This function will attempt to reverse the process and provide a nested json - * hierarchy so that the parsers can be kept simple and we still can use the - * weird syntax some users might use. - * - * Currently this function will flatten out just the 'file.' prefix into a new - * tree. Any other syntax will be rejected. - */ -static virJSONValuePtr -virStorageSourceParseBackingJSONDeflatten(virJSONValuePtr json) -{ - virJSONValuePtr ret; - - if (!(ret = virJSONValueNewObject())) - return NULL; - - if (virJSONValueObjectForeachKeyValue(json, - virStorageSourceParseBackingJSONDeflattenWorker, - ret) < 0) { - virJSONValueFree(ret); - return NULL; - } - - return ret; -} - static int virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, @@ -3320,7 +3257,7 @@ virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, int ret = -1; if (!(file = virJSONValueObjectGetObject(json, "file"))) { - if (!(fixedroot = virStorageSourceParseBackingJSONDeflatten(json))) + if (!(fixedroot = virJSONValueObjectDeflatten(json))) goto cleanup; file = fixedroot; -- 2.12.2
On 06/27/2017 08:46 AM, Peter Krempa wrote:
The code will become more universal so it makes more sense for it to live with the rest of the JSON functions. --- src/libvirt_private.syms | 1 + src/util/virjson.c | 61 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 2 ++ src/util/virstoragefile.c | 65 +---------------------------------------------- 4 files changed, 65 insertions(+), 64 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Currently the function would deflatten the object by dropping the 'file' prefix from the attributes. This does not really scale well or adhere to the documentation. Until we refactor the worker to properly deflatten everything we at least simulate it by adding the "file" wrapper object back. --- src/util/virjson.c | 19 +++++++++++++------ src/util/virstoragefile.c | 8 +++++++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index 8ab542432..a8e28cd1b 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -2012,17 +2012,24 @@ virJSONValueObjectDeflattenWorker(const char *key, virJSONValuePtr virJSONValueObjectDeflatten(virJSONValuePtr json) { - virJSONValuePtr ret; + virJSONValuePtr deflattened; + virJSONValuePtr ret = NULL; - if (!(ret = virJSONValueNewObject())) + if (!(deflattened = virJSONValueNewObject())) return NULL; if (virJSONValueObjectForeachKeyValue(json, virJSONValueObjectDeflattenWorker, - ret) < 0) { - virJSONValueFree(ret); - return NULL; - } + deflattened) < 0) + goto cleanup; + + if (virJSONValueObjectCreate(&ret, "a:file", deflattened, NULL) < 0) + goto cleanup; + + deflattened = NULL; + + cleanup: + virJSONValueFree(deflattened); return ret; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 52c5301ff..d24502fbf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3260,7 +3260,13 @@ virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, if (!(fixedroot = virJSONValueObjectDeflatten(json))) goto cleanup; - file = fixedroot; + if (!(file = virJSONValueObjectGetObject(fixedroot, "file"))) { + str = virJSONValueToString(json, false); + virReportError(VIR_ERR_INVALID_ARG, + _("JSON backing volume defintion '%s' lacks 'file' object"), + str); + goto cleanup; + } } if (!(drvname = virJSONValueObjectGetString(file, "driver"))) { -- 2.12.2
On 06/27/2017 08:46 AM, Peter Krempa wrote:
Currently the function would deflatten the object by dropping the 'file' prefix from the attributes. This does not really scale well or adhere to the documentation.
Until we refactor the worker to properly deflatten everything we at least simulate it by adding the "file" wrapper object back. --- src/util/virjson.c | 19 +++++++++++++------ src/util/virstoragefile.c | 8 +++++++- 2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c index 8ab542432..a8e28cd1b 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -2012,17 +2012,24 @@ virJSONValueObjectDeflattenWorker(const char *key, virJSONValuePtr virJSONValueObjectDeflatten(virJSONValuePtr json) { - virJSONValuePtr ret; + virJSONValuePtr deflattened; + virJSONValuePtr ret = NULL;
- if (!(ret = virJSONValueNewObject())) + if (!(deflattened = virJSONValueNewObject())) return NULL;
if (virJSONValueObjectForeachKeyValue(json, virJSONValueObjectDeflattenWorker, - ret) < 0) { - virJSONValueFree(ret); - return NULL; - } + deflattened) < 0) + goto cleanup; + + if (virJSONValueObjectCreate(&ret, "a:file", deflattened, NULL) < 0) + goto cleanup; + + deflattened = NULL; + + cleanup: + virJSONValueFree(deflattened);
return ret; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 52c5301ff..d24502fbf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3260,7 +3260,13 @@ virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, if (!(fixedroot = virJSONValueObjectDeflatten(json))) goto cleanup;
- file = fixedroot; + if (!(file = virJSONValueObjectGetObject(fixedroot, "file"))) { + str = virJSONValueToString(json, false); + virReportError(VIR_ERR_INVALID_ARG, + _("JSON backing volume defintion '%s' lacks 'file' object"), + str);
Not aligned properly under VIR_ERR_INVALID_ARG Similar to driver below, this should be NULLSTR(str); Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ goto cleanup; + } }
if (!(drvname = virJSONValueObjectGetString(file, "driver"))) {
Add a few test cases to verify that the old behaviour does not break and that new one behaves sanely. --- tests/virjsondata/deflatten-basic-file-in.json | 8 +++ tests/virjsondata/deflatten-basic-file-out.json | 10 +++ tests/virjsondata/deflatten-basic-generic-in.json | 14 ++++ .../deflatten-concat-double-key-in.json | 7 ++ .../deflatten-concat-double-key-out.json | 9 +++ tests/virjsondata/deflatten-concat-in.json | 5 ++ tests/virjsondata/deflatten-concat-out.json | 9 +++ tests/virjsondata/deflatten-deep-file-in.json | 9 +++ tests/virjsondata/deflatten-deep-file-out.json | 11 ++++ tests/virjsondata/deflatten-deep-generic-in.json | 9 +++ tests/virjsondata/deflatten-double-key-in.json | 4 ++ tests/virjsondata/deflatten-double-key-out.json | 6 ++ tests/virjsondata/deflatten-nested-in.json | 16 +++++ tests/virjsondata/deflatten-nested-out.json | 18 ++++++ tests/virjsondata/deflatten-unflattened-in.json | 12 ++++ tests/virjsondata/deflatten-unflattened-out.json | 14 ++++ tests/virjsontest.c | 74 ++++++++++++++++++++++ 17 files changed, 235 insertions(+) create mode 100644 tests/virjsondata/deflatten-basic-file-in.json create mode 100644 tests/virjsondata/deflatten-basic-file-out.json create mode 100644 tests/virjsondata/deflatten-basic-generic-in.json create mode 100644 tests/virjsondata/deflatten-concat-double-key-in.json create mode 100644 tests/virjsondata/deflatten-concat-double-key-out.json create mode 100644 tests/virjsondata/deflatten-concat-in.json create mode 100644 tests/virjsondata/deflatten-concat-out.json create mode 100644 tests/virjsondata/deflatten-deep-file-in.json create mode 100644 tests/virjsondata/deflatten-deep-file-out.json create mode 100644 tests/virjsondata/deflatten-deep-generic-in.json create mode 100644 tests/virjsondata/deflatten-double-key-in.json create mode 100644 tests/virjsondata/deflatten-double-key-out.json create mode 100644 tests/virjsondata/deflatten-nested-in.json create mode 100644 tests/virjsondata/deflatten-nested-out.json create mode 100644 tests/virjsondata/deflatten-unflattened-in.json create mode 100644 tests/virjsondata/deflatten-unflattened-out.json diff --git a/tests/virjsondata/deflatten-basic-file-in.json b/tests/virjsondata/deflatten-basic-file-in.json new file mode 100644 index 000000000..54f8bcd88 --- /dev/null +++ b/tests/virjsondata/deflatten-basic-file-in.json @@ -0,0 +1,8 @@ +{ + "file.int": 1, + "file.string": "string", + "file.object": { + "data":"value", + "foo":"bar" + } +} diff --git a/tests/virjsondata/deflatten-basic-file-out.json b/tests/virjsondata/deflatten-basic-file-out.json new file mode 100644 index 000000000..1a48eda54 --- /dev/null +++ b/tests/virjsondata/deflatten-basic-file-out.json @@ -0,0 +1,10 @@ +{ + "file": { + "int": 1, + "string": "string", + "object": { + "data": "value", + "foo": "bar" + } + } +} diff --git a/tests/virjsondata/deflatten-basic-generic-in.json b/tests/virjsondata/deflatten-basic-generic-in.json new file mode 100644 index 000000000..08304287d --- /dev/null +++ b/tests/virjsondata/deflatten-basic-generic-in.json @@ -0,0 +1,14 @@ +{ + "foo.int": 1, + "bar.int": 1, + "foo.string": "string", + "foo.object": { + "data":"value", + "foo":"bar" + }, + "blurb.string": "string", + "blurb.object": { + "data":"value", + "foo":"bar" + } +} diff --git a/tests/virjsondata/deflatten-concat-double-key-in.json b/tests/virjsondata/deflatten-concat-double-key-in.json new file mode 100644 index 000000000..5bb020ca9 --- /dev/null +++ b/tests/virjsondata/deflatten-concat-double-key-in.json @@ -0,0 +1,7 @@ +{ + "file.nest":{ + "into":"is already here" + }, + "file.nest.into":2, + "file.nest.there":"too" +} diff --git a/tests/virjsondata/deflatten-concat-double-key-out.json b/tests/virjsondata/deflatten-concat-double-key-out.json new file mode 100644 index 000000000..5624ef123 --- /dev/null +++ b/tests/virjsondata/deflatten-concat-double-key-out.json @@ -0,0 +1,9 @@ +{ + "file": { + "nest": { + "into": "is already here" + }, + "nest.into": 2, + "nest.there": "too" + } +} diff --git a/tests/virjsondata/deflatten-concat-in.json b/tests/virjsondata/deflatten-concat-in.json new file mode 100644 index 000000000..91a824375 --- /dev/null +++ b/tests/virjsondata/deflatten-concat-in.json @@ -0,0 +1,5 @@ +{ + "file.nest":{}, + "file.nest.into":2, + "file.nest.there":"too" +} diff --git a/tests/virjsondata/deflatten-concat-out.json b/tests/virjsondata/deflatten-concat-out.json new file mode 100644 index 000000000..539d2cc30 --- /dev/null +++ b/tests/virjsondata/deflatten-concat-out.json @@ -0,0 +1,9 @@ +{ + "file": { + "nest": { + + }, + "nest.into": 2, + "nest.there": "too" + } +} diff --git a/tests/virjsondata/deflatten-deep-file-in.json b/tests/virjsondata/deflatten-deep-file-in.json new file mode 100644 index 000000000..f1b1586c0 --- /dev/null +++ b/tests/virjsondata/deflatten-deep-file-in.json @@ -0,0 +1,9 @@ +{ + "file.double.nest1":"some", + "file.double.nest2":"more", + "file.double.nest3": { + "even":"objects" + }, + "file.very.deeply.nested.object.chains.nest1":"some", + "file.very.deeply.nested.object.chains.nest2":"stuff" +} diff --git a/tests/virjsondata/deflatten-deep-file-out.json b/tests/virjsondata/deflatten-deep-file-out.json new file mode 100644 index 000000000..a5910c9f7 --- /dev/null +++ b/tests/virjsondata/deflatten-deep-file-out.json @@ -0,0 +1,11 @@ +{ + "file": { + "double.nest1": "some", + "double.nest2": "more", + "double.nest3": { + "even": "objects" + }, + "very.deeply.nested.object.chains.nest1": "some", + "very.deeply.nested.object.chains.nest2": "stuff" + } +} diff --git a/tests/virjsondata/deflatten-deep-generic-in.json b/tests/virjsondata/deflatten-deep-generic-in.json new file mode 100644 index 000000000..6a4edfb9b --- /dev/null +++ b/tests/virjsondata/deflatten-deep-generic-in.json @@ -0,0 +1,9 @@ +{ + "foo.double.nest1":"some", + "foo.double.nest2":"more", + "bar.double.nest3": { + "even":"objects" + }, + "foo.very.deeply.nested.object.chains.nest1":"some", + "foo.very.deeply.nested.object.chains.nest2":"stuff" +} diff --git a/tests/virjsondata/deflatten-double-key-in.json b/tests/virjsondata/deflatten-double-key-in.json new file mode 100644 index 000000000..7bca2921b --- /dev/null +++ b/tests/virjsondata/deflatten-double-key-in.json @@ -0,0 +1,4 @@ +{ + "file.nest":1, + "file.nest.into":2 +} diff --git a/tests/virjsondata/deflatten-double-key-out.json b/tests/virjsondata/deflatten-double-key-out.json new file mode 100644 index 000000000..ca6766e5b --- /dev/null +++ b/tests/virjsondata/deflatten-double-key-out.json @@ -0,0 +1,6 @@ +{ + "file": { + "nest": 1, + "nest.into": 2 + } +} diff --git a/tests/virjsondata/deflatten-nested-in.json b/tests/virjsondata/deflatten-nested-in.json new file mode 100644 index 000000000..38e69683e --- /dev/null +++ b/tests/virjsondata/deflatten-nested-in.json @@ -0,0 +1,16 @@ +{ + "file.nest": { + "even.objects":"can", + "even.contain":"some", + "even.more":{ + "deeply.nested":"objects", + "deeply.needing":"deflattening", + "deeply.some.even":"more", + "deeply.some.than":{ + "others.thought.was":"even", + "others.thought.completely":"necessary" + }, + "perhaps":"flat value" + } + } +} diff --git a/tests/virjsondata/deflatten-nested-out.json b/tests/virjsondata/deflatten-nested-out.json new file mode 100644 index 000000000..acdcd1fc8 --- /dev/null +++ b/tests/virjsondata/deflatten-nested-out.json @@ -0,0 +1,18 @@ +{ + "file": { + "nest": { + "even.objects": "can", + "even.contain": "some", + "even.more": { + "deeply.nested": "objects", + "deeply.needing": "deflattening", + "deeply.some.even": "more", + "deeply.some.than": { + "others.thought.was": "even", + "others.thought.completely": "necessary" + }, + "perhaps": "flat value" + } + } + } +} diff --git a/tests/virjsondata/deflatten-unflattened-in.json b/tests/virjsondata/deflatten-unflattened-in.json new file mode 100644 index 000000000..e8a68fe17 --- /dev/null +++ b/tests/virjsondata/deflatten-unflattened-in.json @@ -0,0 +1,12 @@ +{ + "file.blah": { + "any":"possible", + "combination":{ + "of":"json", + "nested":{ + "objects":"should", + "be":"possible" + } + } + } +} diff --git a/tests/virjsondata/deflatten-unflattened-out.json b/tests/virjsondata/deflatten-unflattened-out.json new file mode 100644 index 000000000..c02cb4864 --- /dev/null +++ b/tests/virjsondata/deflatten-unflattened-out.json @@ -0,0 +1,14 @@ +{ + "file": { + "blah": { + "any": "possible", + "combination": { + "of": "json", + "nested": { + "objects": "should", + "be": "possible" + } + } + } + } +} diff --git a/tests/virjsontest.c b/tests/virjsontest.c index b67f68ccb..d69b22bf3 100644 --- a/tests/virjsontest.c +++ b/tests/virjsontest.c @@ -9,6 +9,8 @@ #include "virjson.h" #include "testutils.h" +#define VIR_FROM_THIS VIR_FROM_NONE + struct testInfo { const char *doc; const char *expect; @@ -312,6 +314,65 @@ testJSONCopy(const void *data) static int +testJSONDeflatten(const void *data) +{ + const struct testInfo *info = data; + virJSONValuePtr injson = NULL; + virJSONValuePtr deflattened = NULL; + char *infile = NULL; + char *indata = NULL; + char *outfile = NULL; + char *actual = NULL; + int ret = -1; + + if (virAsprintf(&infile, "%s/virjsondata/deflatten-%s-in.json", + abs_srcdir, info->doc) < 0 || + virAsprintf(&outfile, "%s/virjsondata/deflatten-%s-out.json", + abs_srcdir, info->doc) < 0) + goto cleanup; + + if (virTestLoadFile(infile, &indata) < 0) + goto cleanup; + + if (!(injson = virJSONValueFromString(indata))) + goto cleanup; + + if ((deflattened = virJSONValueObjectDeflatten(injson))) { + if (!info->pass) { + if (virTestGetVerbose()) + fprintf(stderr, "deflattening should have failed\n"); + + goto cleanup; + } + } else { + if (!info->pass) + ret = 0; + + goto cleanup; + } + + if (!(actual = virJSONValueToString(deflattened, true))) + goto cleanup; + + if (virTestCompareToFile(actual, outfile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virJSONValueFree(injson); + virJSONValueFree(deflattened); + VIR_FREE(infile); + VIR_FREE(outfile); + VIR_FREE(outfile); + VIR_FREE(actual); + + return ret; + +} + + +static int mymain(void) { int ret = 0; @@ -448,6 +509,19 @@ mymain(void) "{ \"a\": {}, \"b\": 1, \"c\": \"str\", \"d\": [] }", NULL, true); +#define DO_TEST_DEFLATTEN(name, pass) \ + DO_TEST_FULL(name, Deflatten, name, NULL, pass) + + DO_TEST_DEFLATTEN("unflattened", true); + DO_TEST_DEFLATTEN("basic-file", true); + DO_TEST_DEFLATTEN("basic-generic", false); + DO_TEST_DEFLATTEN("deep-file", true); + DO_TEST_DEFLATTEN("deep-generic", false); + DO_TEST_DEFLATTEN("nested", true); + DO_TEST_DEFLATTEN("double-key", true); + DO_TEST_DEFLATTEN("concat", true); + DO_TEST_DEFLATTEN("concat-double-key", true); + return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.12.2
On 06/27/2017 08:46 AM, Peter Krempa wrote:
Add a few test cases to verify that the old behaviour does not break and that new one behaves sanely. --- tests/virjsondata/deflatten-basic-file-in.json | 8 +++ tests/virjsondata/deflatten-basic-file-out.json | 10 +++ tests/virjsondata/deflatten-basic-generic-in.json | 14 ++++ .../deflatten-concat-double-key-in.json | 7 ++ .../deflatten-concat-double-key-out.json | 9 +++ tests/virjsondata/deflatten-concat-in.json | 5 ++ tests/virjsondata/deflatten-concat-out.json | 9 +++ tests/virjsondata/deflatten-deep-file-in.json | 9 +++ tests/virjsondata/deflatten-deep-file-out.json | 11 ++++ tests/virjsondata/deflatten-deep-generic-in.json | 9 +++ tests/virjsondata/deflatten-double-key-in.json | 4 ++ tests/virjsondata/deflatten-double-key-out.json | 6 ++ tests/virjsondata/deflatten-nested-in.json | 16 +++++ tests/virjsondata/deflatten-nested-out.json | 18 ++++++ tests/virjsondata/deflatten-unflattened-in.json | 12 ++++ tests/virjsondata/deflatten-unflattened-out.json | 14 ++++ tests/virjsontest.c | 74 ++++++++++++++++++++++ 17 files changed, 235 insertions(+) create mode 100644 tests/virjsondata/deflatten-basic-file-in.json create mode 100644 tests/virjsondata/deflatten-basic-file-out.json create mode 100644 tests/virjsondata/deflatten-basic-generic-in.json create mode 100644 tests/virjsondata/deflatten-concat-double-key-in.json create mode 100644 tests/virjsondata/deflatten-concat-double-key-out.json create mode 100644 tests/virjsondata/deflatten-concat-in.json create mode 100644 tests/virjsondata/deflatten-concat-out.json create mode 100644 tests/virjsondata/deflatten-deep-file-in.json create mode 100644 tests/virjsondata/deflatten-deep-file-out.json create mode 100644 tests/virjsondata/deflatten-deep-generic-in.json create mode 100644 tests/virjsondata/deflatten-double-key-in.json create mode 100644 tests/virjsondata/deflatten-double-key-out.json create mode 100644 tests/virjsondata/deflatten-nested-in.json create mode 100644 tests/virjsondata/deflatten-nested-out.json create mode 100644 tests/virjsondata/deflatten-unflattened-in.json create mode 100644 tests/virjsondata/deflatten-unflattened-out.json
[...]
static int +testJSONDeflatten(const void *data) +{ + const struct testInfo *info = data; + virJSONValuePtr injson = NULL; + virJSONValuePtr deflattened = NULL; + char *infile = NULL; + char *indata = NULL; + char *outfile = NULL; + char *actual = NULL; + int ret = -1; + + if (virAsprintf(&infile, "%s/virjsondata/deflatten-%s-in.json", + abs_srcdir, info->doc) < 0 || + virAsprintf(&outfile, "%s/virjsondata/deflatten-%s-out.json", + abs_srcdir, info->doc) < 0) + goto cleanup; + + if (virTestLoadFile(infile, &indata) < 0) + goto cleanup; + + if (!(injson = virJSONValueFromString(indata))) + goto cleanup; + + if ((deflattened = virJSONValueObjectDeflatten(injson))) { + if (!info->pass) { + if (virTestGetVerbose()) + fprintf(stderr, "deflattening should have failed\n");
This (and Copy) should probably use VIR_TEST_VERBOSE, right? At least fix this one - and consider fixing Copy in separate patch which could be considered trivial if you'd like ... For extra, extra credit you could prefix with %s: and use info->doc to further specify which test tailed.
+ + goto cleanup; + } + } else { + if (!info->pass) + ret = 0; + + goto cleanup; + } + + if (!(actual = virJSONValueToString(deflattened, true))) + goto cleanup; + + if (virTestCompareToFile(actual, outfile) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virJSONValueFree(injson); + virJSONValueFree(deflattened); + VIR_FREE(infile); + VIR_FREE(outfile); + VIR_FREE(outfile);
DRD - only need to free once, but the first one could be replaced w/ indata Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ VIR_FREE(actual); + + return ret; + +} + + +static int mymain(void) { int ret = 0; @@ -448,6 +509,19 @@ mymain(void) "{ \"a\": {}, \"b\": 1, \"c\": \"str\", \"d\": [] }", NULL, true);
+#define DO_TEST_DEFLATTEN(name, pass) \ + DO_TEST_FULL(name, Deflatten, name, NULL, pass) + + DO_TEST_DEFLATTEN("unflattened", true); + DO_TEST_DEFLATTEN("basic-file", true); + DO_TEST_DEFLATTEN("basic-generic", false); + DO_TEST_DEFLATTEN("deep-file", true); + DO_TEST_DEFLATTEN("deep-generic", false); + DO_TEST_DEFLATTEN("nested", true); + DO_TEST_DEFLATTEN("double-key", true); + DO_TEST_DEFLATTEN("concat", true); + DO_TEST_DEFLATTEN("concat-double-key", true); + return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; }
As it turns out sometimes users pass in an arbitrarily nested structure e.g. for the qemu backing chains JSON pseudo protocol. This new implementation deflatens now a single object fully even with nested keys. Additionally it's not necessary now to stick with the "file." prefix for the properties. --- src/util/virjson.c | 67 ++++++++++++++++------ tests/virjsondata/deflatten-basic-generic-out.json | 20 +++++++ .../deflatten-concat-double-key-out.json | 9 --- tests/virjsondata/deflatten-concat-out.json | 7 +-- tests/virjsondata/deflatten-deep-file-out.json | 24 ++++++-- tests/virjsondata/deflatten-deep-generic-out.json | 27 +++++++++ tests/virjsondata/deflatten-double-key-out.json | 6 -- tests/virjsontest.c | 8 +-- 8 files changed, 121 insertions(+), 47 deletions(-) create mode 100644 tests/virjsondata/deflatten-basic-generic-out.json delete mode 100644 tests/virjsondata/deflatten-concat-double-key-out.json create mode 100644 tests/virjsondata/deflatten-deep-generic-out.json delete mode 100644 tests/virjsondata/deflatten-double-key-out.json diff --git a/src/util/virjson.c b/src/util/virjson.c index a8e28cd1b..635b78e3a 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1974,23 +1974,60 @@ virJSONValueObjectDeflattenWorker(const char *key, { virJSONValuePtr retobj = opaque; virJSONValuePtr newval = NULL; - const char *newkey; + virJSONValuePtr existobj; + char **tokens = NULL; + size_t ntokens = 0; + int ret = -1; - if (!(newkey = STRSKIP(key, "file."))) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("JSON object is neither nested nor flattened")); - return -1; + /* non-nested keys only need to be copied */ + if (!strchr(key, '.')) { + if (!(newval = virJSONValueCopy(value))) + return -1; + + if (virJSONValueObjectHasKey(retobj, key)) { + virReportError(VIR_ERR_INVALID_ARG, + _("can't deflatten colliding key '%s'"), key); + goto cleanup; + } + + if (virJSONValueObjectAppend(retobj, key, newval) < 0) + goto cleanup; + + return 0; } - if (!(newval = virJSONValueCopy(value))) - return -1; + if (!(tokens = virStringSplitCount(key, ".", 2, &ntokens))) + goto cleanup; - if (virJSONValueObjectAppend(retobj, newkey, newval) < 0) { - virJSONValueFree(newval); - return -1; + if (ntokens != 2) { + virReportError(VIR_ERR_INVALID_ARG, + _("invalid nested value key '%s'"), key); + goto cleanup; } - return 0; + if (!(existobj = virJSONValueObjectGet(retobj, tokens[0]))) { + if (!(existobj = virJSONValueNewObject())) + goto cleanup; + + if (virJSONValueObjectAppend(retobj, tokens[0], existobj) < 0) + goto cleanup; + + } else { + if (!virJSONValueIsObject(existobj)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("mixing nested objects and values is forbidden in " + "JSON deflattening")); + goto cleanup; + } + } + + ret = virJSONValueObjectDeflattenWorker(tokens[1], value, existobj); + + cleanup: + virStringListFreeCount(tokens, ntokens); + virJSONValueFree(newval); + + return ret; } @@ -2005,9 +2042,6 @@ virJSONValueObjectDeflattenWorker(const char *key, * This function will attempt to reverse the process and provide a nested json * hierarchy so that the parsers can be kept simple and we still can use the * weird syntax some users might use. - * - * Currently this function will flatten out just the 'file.' prefix into a new - * tree. Any other syntax will be rejected. */ virJSONValuePtr virJSONValueObjectDeflatten(virJSONValuePtr json) @@ -2023,10 +2057,7 @@ virJSONValueObjectDeflatten(virJSONValuePtr json) deflattened) < 0) goto cleanup; - if (virJSONValueObjectCreate(&ret, "a:file", deflattened, NULL) < 0) - goto cleanup; - - deflattened = NULL; + VIR_STEAL_PTR(ret, deflattened); cleanup: virJSONValueFree(deflattened); diff --git a/tests/virjsondata/deflatten-basic-generic-out.json b/tests/virjsondata/deflatten-basic-generic-out.json new file mode 100644 index 000000000..ab639aa48 --- /dev/null +++ b/tests/virjsondata/deflatten-basic-generic-out.json @@ -0,0 +1,20 @@ +{ + "foo": { + "int": 1, + "string": "string", + "object": { + "data": "value", + "foo": "bar" + } + }, + "bar": { + "int": 1 + }, + "blurb": { + "string": "string", + "object": { + "data": "value", + "foo": "bar" + } + } +} diff --git a/tests/virjsondata/deflatten-concat-double-key-out.json b/tests/virjsondata/deflatten-concat-double-key-out.json deleted file mode 100644 index 5624ef123..000000000 --- a/tests/virjsondata/deflatten-concat-double-key-out.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "file": { - "nest": { - "into": "is already here" - }, - "nest.into": 2, - "nest.there": "too" - } -} diff --git a/tests/virjsondata/deflatten-concat-out.json b/tests/virjsondata/deflatten-concat-out.json index 539d2cc30..be417e53a 100644 --- a/tests/virjsondata/deflatten-concat-out.json +++ b/tests/virjsondata/deflatten-concat-out.json @@ -1,9 +1,8 @@ { "file": { "nest": { - - }, - "nest.into": 2, - "nest.there": "too" + "into": 2, + "there": "too" + } } } diff --git a/tests/virjsondata/deflatten-deep-file-out.json b/tests/virjsondata/deflatten-deep-file-out.json index a5910c9f7..d4614eeaf 100644 --- a/tests/virjsondata/deflatten-deep-file-out.json +++ b/tests/virjsondata/deflatten-deep-file-out.json @@ -1,11 +1,23 @@ { "file": { - "double.nest1": "some", - "double.nest2": "more", - "double.nest3": { - "even": "objects" + "double": { + "nest1": "some", + "nest2": "more", + "nest3": { + "even": "objects" + } }, - "very.deeply.nested.object.chains.nest1": "some", - "very.deeply.nested.object.chains.nest2": "stuff" + "very": { + "deeply": { + "nested": { + "object": { + "chains": { + "nest1": "some", + "nest2": "stuff" + } + } + } + } + } } } diff --git a/tests/virjsondata/deflatten-deep-generic-out.json b/tests/virjsondata/deflatten-deep-generic-out.json new file mode 100644 index 000000000..7ea521a8f --- /dev/null +++ b/tests/virjsondata/deflatten-deep-generic-out.json @@ -0,0 +1,27 @@ +{ + "foo": { + "double": { + "nest1": "some", + "nest2": "more" + }, + "very": { + "deeply": { + "nested": { + "object": { + "chains": { + "nest1": "some", + "nest2": "stuff" + } + } + } + } + } + }, + "bar": { + "double": { + "nest3": { + "even": "objects" + } + } + } +} diff --git a/tests/virjsondata/deflatten-double-key-out.json b/tests/virjsondata/deflatten-double-key-out.json deleted file mode 100644 index ca6766e5b..000000000 --- a/tests/virjsondata/deflatten-double-key-out.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "file": { - "nest": 1, - "nest.into": 2 - } -} diff --git a/tests/virjsontest.c b/tests/virjsontest.c index d69b22bf3..b3ce6591a 100644 --- a/tests/virjsontest.c +++ b/tests/virjsontest.c @@ -514,13 +514,13 @@ mymain(void) DO_TEST_DEFLATTEN("unflattened", true); DO_TEST_DEFLATTEN("basic-file", true); - DO_TEST_DEFLATTEN("basic-generic", false); + DO_TEST_DEFLATTEN("basic-generic", true); DO_TEST_DEFLATTEN("deep-file", true); - DO_TEST_DEFLATTEN("deep-generic", false); + DO_TEST_DEFLATTEN("deep-generic", true); DO_TEST_DEFLATTEN("nested", true); - DO_TEST_DEFLATTEN("double-key", true); + DO_TEST_DEFLATTEN("double-key", false); DO_TEST_DEFLATTEN("concat", true); - DO_TEST_DEFLATTEN("concat-double-key", true); + DO_TEST_DEFLATTEN("concat-double-key", false); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.12.2
On 06/27/2017 08:46 AM, Peter Krempa wrote:
As it turns out sometimes users pass in an arbitrarily nested structure e.g. for the qemu backing chains JSON pseudo protocol. This new implementation deflatens now a single object fully even with nested
deflattens
keys.
Additionally it's not necessary now to stick with the "file." prefix for the properties. --- src/util/virjson.c | 67 ++++++++++++++++------ tests/virjsondata/deflatten-basic-generic-out.json | 20 +++++++ .../deflatten-concat-double-key-out.json | 9 --- tests/virjsondata/deflatten-concat-out.json | 7 +-- tests/virjsondata/deflatten-deep-file-out.json | 24 ++++++-- tests/virjsondata/deflatten-deep-generic-out.json | 27 +++++++++ tests/virjsondata/deflatten-double-key-out.json | 6 -- tests/virjsontest.c | 8 +-- 8 files changed, 121 insertions(+), 47 deletions(-) create mode 100644 tests/virjsondata/deflatten-basic-generic-out.json delete mode 100644 tests/virjsondata/deflatten-concat-double-key-out.json create mode 100644 tests/virjsondata/deflatten-deep-generic-out.json delete mode 100644 tests/virjsondata/deflatten-double-key-out.json
Reviewed-by: John Ferlan <jferlan@redhat.com> John
If a value of the first level object contains more objects needing deflattening which would be wrapped in an actual object the function would not recurse into them. By this simple addition we can fully deflatten the objects. --- src/util/virjson.c | 8 +++++++- tests/virjsondata/deflatten-nested-out.json | 32 +++++++++++++++++++---------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index 635b78e3a..0c7beafab 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1981,7 +1981,13 @@ virJSONValueObjectDeflattenWorker(const char *key, /* non-nested keys only need to be copied */ if (!strchr(key, '.')) { - if (!(newval = virJSONValueCopy(value))) + + if (virJSONValueIsObject(value)) + newval = virJSONValueObjectDeflatten(value); + else + newval = virJSONValueCopy(value); + + if (!newval) return -1; if (virJSONValueObjectHasKey(retobj, key)) { diff --git a/tests/virjsondata/deflatten-nested-out.json b/tests/virjsondata/deflatten-nested-out.json index acdcd1fc8..f23ed8fd5 100644 --- a/tests/virjsondata/deflatten-nested-out.json +++ b/tests/virjsondata/deflatten-nested-out.json @@ -1,17 +1,27 @@ { "file": { "nest": { - "even.objects": "can", - "even.contain": "some", - "even.more": { - "deeply.nested": "objects", - "deeply.needing": "deflattening", - "deeply.some.even": "more", - "deeply.some.than": { - "others.thought.was": "even", - "others.thought.completely": "necessary" - }, - "perhaps": "flat value" + "even": { + "objects": "can", + "contain": "some", + "more": { + "deeply": { + "nested": "objects", + "needing": "deflattening", + "some": { + "even": "more", + "than": { + "others": { + "thought": { + "was": "even", + "completely": "necessary" + } + } + } + } + }, + "perhaps": "flat value" + } } } } -- 2.12.2
On 06/27/2017 08:46 AM, Peter Krempa wrote:
If a value of the first level object contains more objects needing deflattening which would be wrapped in an actual object the function would not recurse into them.
By this simple addition we can fully deflatten the objects. --- src/util/virjson.c | 8 +++++++- tests/virjsondata/deflatten-nested-out.json | 32 +++++++++++++++++++---------- 2 files changed, 28 insertions(+), 12 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Now that the JSON deflattener is working sanely we can always attempt the deflattening so that we can then parse the tree as expected. --- src/util/virstoragefile.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d24502fbf..f3dc860ae 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3249,24 +3249,22 @@ static int virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, virJSONValuePtr json) { - virJSONValuePtr fixedroot = NULL; + virJSONValuePtr deflattened = NULL; virJSONValuePtr file; const char *drvname; char *str = NULL; size_t i; int ret = -1; - if (!(file = virJSONValueObjectGetObject(json, "file"))) { - if (!(fixedroot = virJSONValueObjectDeflatten(json))) - goto cleanup; + if (!(deflattened = virJSONValueObjectDeflatten(json))) + goto cleanup; - if (!(file = virJSONValueObjectGetObject(fixedroot, "file"))) { - str = virJSONValueToString(json, false); - virReportError(VIR_ERR_INVALID_ARG, - _("JSON backing volume defintion '%s' lacks 'file' object"), - str); - goto cleanup; - } + if (!(file = virJSONValueObjectGetObject(deflattened, "file"))) { + str = virJSONValueToString(json, false); + virReportError(VIR_ERR_INVALID_ARG, + _("JSON backing volume defintion '%s' lacks 'file' object"), + str); + goto cleanup; } if (!(drvname = virJSONValueObjectGetString(file, "driver"))) { @@ -3290,7 +3288,7 @@ virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, cleanup: VIR_FREE(str); - virJSONValueFree(fixedroot); + virJSONValueFree(deflattened); return ret; } -- 2.12.2
On 06/27/2017 08:46 AM, Peter Krempa wrote:
Now that the JSON deflattener is working sanely we can always attempt the deflattening so that we can then parse the tree as expected. --- src/util/virstoragefile.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
I'm assuming you'll end up with an adjustment in this patch too since patch 4 will have aligned the error properly... [1] Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d24502fbf..f3dc860ae 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3249,24 +3249,22 @@ static int virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src, virJSONValuePtr json) { - virJSONValuePtr fixedroot = NULL; + virJSONValuePtr deflattened = NULL; virJSONValuePtr file; const char *drvname; char *str = NULL; size_t i; int ret = -1;
- if (!(file = virJSONValueObjectGetObject(json, "file"))) { - if (!(fixedroot = virJSONValueObjectDeflatten(json))) - goto cleanup; + if (!(deflattened = virJSONValueObjectDeflatten(json))) + goto cleanup;
- if (!(file = virJSONValueObjectGetObject(fixedroot, "file"))) { - str = virJSONValueToString(json, false); - virReportError(VIR_ERR_INVALID_ARG, - _("JSON backing volume defintion '%s' lacks 'file' object"), - str); - goto cleanup; - } + if (!(file = virJSONValueObjectGetObject(deflattened, "file"))) { + str = virJSONValueToString(json, false); + virReportError(VIR_ERR_INVALID_ARG, + _("JSON backing volume defintion '%s' lacks 'file' object"), + str);
[1]
+ goto cleanup; }
if (!(drvname = virJSONValueObjectGetString(file, "driver"))) { @@ -3290,7 +3288,7 @@ virStorageSourceParseBackingJSONInternal(virStorageSourcePtr src,
cleanup: VIR_FREE(str); - virJSONValueFree(fixedroot); + virJSONValueFree(deflattened); return ret; }
Sheepdog and possibly others use nested objects for network server and thus could be specified in a way that libvirt would not parse. Validates that https://bugzilla.redhat.com/show_bug.cgi?id=1464821 is fixed properly. --- tests/virjsondata/deflatten-qemu-sheepdog-in.json | 11 +++++++++++ tests/virjsondata/deflatten-qemu-sheepdog-out.json | 13 +++++++++++++ tests/virjsontest.c | 1 + tests/virstoragetest.c | 10 ++++++++++ 4 files changed, 35 insertions(+) create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-in.json create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-out.json diff --git a/tests/virjsondata/deflatten-qemu-sheepdog-in.json b/tests/virjsondata/deflatten-qemu-sheepdog-in.json new file mode 100644 index 000000000..7c0286300 --- /dev/null +++ b/tests/virjsondata/deflatten-qemu-sheepdog-in.json @@ -0,0 +1,11 @@ +{ + "driver": "raw", + "file": { + "server.host": "10.10.10.10", + "server.port": "7000", + "tag": "", + "driver": "sheepdog", + "server.type": "inet", + "vdi": "Alice" + } +} diff --git a/tests/virjsondata/deflatten-qemu-sheepdog-out.json b/tests/virjsondata/deflatten-qemu-sheepdog-out.json new file mode 100644 index 000000000..258b44a76 --- /dev/null +++ b/tests/virjsondata/deflatten-qemu-sheepdog-out.json @@ -0,0 +1,13 @@ +{ + "driver": "raw", + "file": { + "server": { + "host": "10.10.10.10", + "port": "7000", + "type": "inet" + }, + "tag": "", + "driver": "sheepdog", + "vdi": "Alice" + } +} diff --git a/tests/virjsontest.c b/tests/virjsontest.c index b3ce6591a..5f89a3e1c 100644 --- a/tests/virjsontest.c +++ b/tests/virjsontest.c @@ -521,6 +521,7 @@ mymain(void) DO_TEST_DEFLATTEN("double-key", false); DO_TEST_DEFLATTEN("concat", true); DO_TEST_DEFLATTEN("concat-double-key", false); + DO_TEST_DEFLATTEN("qemu-sheepdog", true); return (ret == 0) ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f34408395..90fcf36ca 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1575,6 +1575,16 @@ mymain(void) "<source protocol='sheepdog' name='test'>\n" " <host name='example.com' port='321'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{\"driver\": \"raw\"," + "\"file\": {\"server.host\": \"10.10.10.10\"," + "\"server.port\": \"7000\"," + "\"tag\": \"\"," + "\"driver\": \"sheepdog\"," + "\"server.type\": \"inet\"," + "\"vdi\": \"Alice\"}}", + "<source protocol='sheepdog' name='Alice'>\n" + " <host name='10.10.10.10' port='7000'/>\n" + "</source>\n"); #endif /* WITH_YAJL */ cleanup: -- 2.12.2
On 06/27/2017 08:46 AM, Peter Krempa wrote:
Sheepdog and possibly others use nested objects for network server and thus could be specified in a way that libvirt would not parse.
Validates that https://bugzilla.redhat.com/show_bug.cgi?id=1464821 is fixed properly. --- tests/virjsondata/deflatten-qemu-sheepdog-in.json | 11 +++++++++++ tests/virjsondata/deflatten-qemu-sheepdog-out.json | 13 +++++++++++++ tests/virjsontest.c | 1 + tests/virstoragetest.c | 10 ++++++++++ 4 files changed, 35 insertions(+) create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-in.json create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-out.json
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Mon, Jul 10, 2017 at 10:01:30 -0400, John Ferlan wrote:
On 06/27/2017 08:46 AM, Peter Krempa wrote:
Sheepdog and possibly others use nested objects for network server and thus could be specified in a way that libvirt would not parse.
Validates that https://bugzilla.redhat.com/show_bug.cgi?id=1464821 is fixed properly. --- tests/virjsondata/deflatten-qemu-sheepdog-in.json | 11 +++++++++++ tests/virjsondata/deflatten-qemu-sheepdog-out.json | 13 +++++++++++++ tests/virjsontest.c | 1 + tests/virstoragetest.c | 10 ++++++++++ 4 files changed, 35 insertions(+) create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-in.json create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-out.json
Reviewed-by: John Ferlan <jferlan@redhat.com>
Thanks for the review, I've implemented your suggestions and pushed the patches.
participants (3)
-
Bjoern Walk -
John Ferlan -
Peter Krempa