[Qemu-devel] [PATCH 0/4] block: Allow JSON filenames

[Qemu-devel] [PATCH 0/4] block: Allow JSON filenames

[Max Reitz]

This series acts as some kind of alternative or v4 to the "block/json:
Add JSON protocol driver" series. It makes bdrv_open() parse filenames
prefixed by "json:" as JSON objects (discarding the prefix beforehand)
and then use the resulting QDict as the options for the block device to
be opened with a NULL filename.

The purpose of this is that it may sometimes be desirable to specify
options for a block device where only a filename can be given, e.g., for
backing files. Using this should obviously be the exception, but it is
nice to have if actually needed.


After having written this series, I do indeed agree that it is much
nicer than the old version of having a dedicated "virtual" block driver
for this purpose.

Patches 1, 2 and 4 are taken directly from the block/json v3 (see
backport-diff output).


git-backport-diff against [PATCH v3 00/12] block/json:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/4:[----] [--] 'qdict: Add qdict_join()'
002/4:[----] [--] 'check-qdict: Add test for qdict_join()'
003/4:[down] 'block: Allow JSON filenames'
004/4:[----] [-C] 'iotests: Add test for the JSON protocol'


Max Reitz (4):
  qdict: Add qdict_join()
  check-qdict: Add test for qdict_join()
  block: Allow JSON filenames
  iotests: Add test for the JSON protocol

 block.c                    |  41 +++++++++++++++
 include/qapi/qmp/qdict.h   |   3 ++
 qobject/qdict.c            |  32 ++++++++++++
 tests/check-qdict.c        |  87 ++++++++++++++++++++++++++++++++
 tests/qemu-iotests/089     | 123 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/089.out |  39 ++++++++++++++
 tests/qemu-iotests/group   |   1 +
 7 files changed, 326 insertions(+)
 create mode 100755 tests/qemu-iotests/089
 create mode 100644 tests/qemu-iotests/089.out

-- 
1.9.2

[Qemu-devel] [PATCH 1/4] qdict: Add qdict_join()

[Max Reitz]

This function joins two QDicts by absorbing one into the other.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/qmp/qdict.h |  3 +++
 qobject/qdict.c          | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 1ddf97b..d68f4eb 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -16,6 +16,7 @@
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qlist.h"
 #include "qemu/queue.h"
+#include <stdbool.h>
 #include <stdint.h>
 
 #define QDICT_BUCKET_MAX 512
@@ -70,4 +71,6 @@ void qdict_flatten(QDict *qdict);
 void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
 void qdict_array_split(QDict *src, QList **dst);
 
+void qdict_join(QDict *dest, QDict *src, bool overwrite);
+
 #endif /* QDICT_H */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 42ec4c0..ea239f0 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -665,3 +665,35 @@ void qdict_array_split(QDict *src, QList **dst)
         qlist_append_obj(*dst, subqobj ?: QOBJECT(subqdict));
     }
 }
+
+/**
+ * qdict_join(): Absorb the src QDict into the dest QDict, that is, move all
+ * elements from src to dest.
+ *
+ * If an element from src has a key already present in dest, it will not be
+ * moved unless overwrite is true.
+ *
+ * If overwrite is true, the conflicting values in dest will be discarded and
+ * replaced by the corresponding values from src.
+ *
+ * Therefore, with overwrite being true, the src QDict will always be empty when
+ * this function returns. If overwrite is false, the src QDict will be empty
+ * iff there were no conflicts.
+ */
+void qdict_join(QDict *dest, QDict *src, bool overwrite)
+{
+    const QDictEntry *entry, *next;
+
+    entry = qdict_first(src);
+    while (entry) {
+        next = qdict_next(src, entry);
+
+        if (overwrite || !qdict_haskey(dest, entry->key)) {
+            qobject_incref(entry->value);
+            qdict_put_obj(dest, entry->key, entry->value);
+            qdict_del(src, entry->key);
+        }
+
+        entry = next;
+    }
+}
-- 
1.9.2

[Qemu-devel] [PATCH 2/4] check-qdict: Add test for qdict_join()

[Max Reitz]

Add some test cases for qdict_join().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 tests/check-qdict.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/tests/check-qdict.c b/tests/check-qdict.c
index 2ad0f78..a9296f0 100644
--- a/tests/check-qdict.c
+++ b/tests/check-qdict.c
@@ -444,6 +444,92 @@ static void qdict_array_split_test(void)
     QDECREF(test_dict);
 }
 
+static void qdict_join_test(void)
+{
+    QDict *dict1, *dict2;
+    bool overwrite = false;
+    int i;
+
+    dict1 = qdict_new();
+    dict2 = qdict_new();
+
+
+    /* Test everything once without overwrite and once with */
+    do
+    {
+        /* Test empty dicts */
+        qdict_join(dict1, dict2, overwrite);
+
+        g_assert(qdict_size(dict1) == 0);
+        g_assert(qdict_size(dict2) == 0);
+
+
+        /* First iteration: Test movement */
+        /* Second iteration: Test empty source and non-empty destination */
+        qdict_put(dict2, "foo", qint_from_int(42));
+
+        for (i = 0; i < 2; i++) {
+            qdict_join(dict1, dict2, overwrite);
+
+            g_assert(qdict_size(dict1) == 1);
+            g_assert(qdict_size(dict2) == 0);
+
+            g_assert(qdict_get_int(dict1, "foo") == 42);
+        }
+
+
+        /* Test non-empty source and destination without conflict */
+        qdict_put(dict2, "bar", qint_from_int(23));
+
+        qdict_join(dict1, dict2, overwrite);
+
+        g_assert(qdict_size(dict1) == 2);
+        g_assert(qdict_size(dict2) == 0);
+
+        g_assert(qdict_get_int(dict1, "foo") == 42);
+        g_assert(qdict_get_int(dict1, "bar") == 23);
+
+
+        /* Test conflict */
+        qdict_put(dict2, "foo", qint_from_int(84));
+
+        qdict_join(dict1, dict2, overwrite);
+
+        g_assert(qdict_size(dict1) == 2);
+        g_assert(qdict_size(dict2) == !overwrite);
+
+        g_assert(qdict_get_int(dict1, "foo") == overwrite ? 84 : 42);
+        g_assert(qdict_get_int(dict1, "bar") == 23);
+
+        if (!overwrite) {
+            g_assert(qdict_get_int(dict2, "foo") == 84);
+        }
+
+
+        /* Check the references */
+        g_assert(qdict_get(dict1, "foo")->refcnt == 1);
+        g_assert(qdict_get(dict1, "bar")->refcnt == 1);
+
+        if (!overwrite) {
+            g_assert(qdict_get(dict2, "foo")->refcnt == 1);
+        }
+
+
+        /* Clean up */
+        qdict_del(dict1, "foo");
+        qdict_del(dict1, "bar");
+
+        if (!overwrite) {
+            qdict_del(dict2, "foo");
+        }
+    }
+    while (overwrite ^= true);
+
+
+    QDECREF(dict1);
+    QDECREF(dict2);
+}
+
 /*
  * Errors test-cases
  */
@@ -584,6 +670,7 @@ int main(int argc, char **argv)
     g_test_add_func("/public/iterapi", qdict_iterapi_test);
     g_test_add_func("/public/flatten", qdict_flatten_test);
     g_test_add_func("/public/array_split", qdict_array_split_test);
+    g_test_add_func("/public/join", qdict_join_test);
 
     g_test_add_func("/errors/put_exists", qdict_put_exists_test);
     g_test_add_func("/errors/get_not_exists", qdict_get_not_exists_test);
-- 
1.9.2

[Qemu-devel] [PATCH 3/4] block: Allow JSON filenames

[Max Reitz]

If the filename given to bdrv_open() is prefixed with "json:", parse the
rest as a JSON object and use the result as the options QDict.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/block.c b/block.c
index b749d31..2555e34 100644
--- a/block.c
+++ b/block.c
@@ -1275,6 +1275,33 @@ out:
     g_free(tmp_filename);
 }
 
+static QDict *parse_json_filename(const char *filename, Error **errp)
+{
+    QObject *options_obj;
+    QDict *options;
+    int ret;
+
+    ret = strstart(filename, "json:", &filename);
+    assert(ret);
+
+    options_obj = qobject_from_json(filename);
+    if (!options_obj) {
+        error_setg(errp, "Could not parse the JSON options");
+        return NULL;
+    }
+
+    if (qobject_type(options_obj) != QTYPE_QDICT) {
+        qobject_decref(options_obj);
+        error_setg(errp, "Invalid JSON object given");
+        return NULL;
+    }
+
+    options = qobject_to_qdict(options_obj);
+    qdict_flatten(options);
+
+    return options;
+}
+
 /*
  * Opens a disk image (raw, qcow2, vmdk, ...)
  *
@@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
+    if (filename && g_str_has_prefix(filename, "json:")) {
+        QDict *json_options = parse_json_filename(filename, &local_err);
+        if (local_err) {
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        qdict_join(options, json_options, true);
+        assert(qdict_size(json_options) == 0);
+        QDECREF(json_options);
+
+        filename = NULL;
+    }
+
     bs->options = options;
     options = qdict_clone_shallow(options);
 
-- 
1.9.2

[Qemu-devel] [PATCH 4/4] iotests: Add test for the JSON protocol

[Max Reitz]

Add a test for the JSON protocol driver.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 tests/qemu-iotests/089     | 123 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/089.out |  39 ++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 163 insertions(+)
 create mode 100755 tests/qemu-iotests/089
 create mode 100644 tests/qemu-iotests/089.out

diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
new file mode 100755
index 0000000..75cb0c2
--- /dev/null
+++ b/tests/qemu-iotests/089
@@ -0,0 +1,123 @@
+#!/bin/bash
+#
+# Test case for the JSON block protocol
+#
+# Copyright (C) 2014 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# Using an image filename containing quotation marks will render the JSON data
+# below invalid. In that case, we have little choice but simply not to run this
+# test.
+case $TEST_IMG in
+    *'"'*)
+        _notrun "image filename may not contain quotation marks"
+        ;;
+esac
+
+IMG_SIZE=64M
+
+# Taken from test 072
+echo
+echo "=== Testing nested image formats ==="
+echo
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $IMG_SIZE
+
+$QEMU_IO -c 'write -P 42 0 512' -c 'write -P 23 512 512' \
+         -c 'write -P 66 1024 512' "$TEST_IMG.base" | _filter_qemu_io
+
+$QEMU_IMG convert -f raw -O $IMGFMT "$TEST_IMG.base" "$TEST_IMG"
+
+$QEMU_IO -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
+         -c 'read -P 66 1024 512' "json:{
+    \"driver\": \"$IMGFMT\",
+    \"file\": {
+        \"driver\": \"$IMGFMT\",
+        \"file\": {
+            \"filename\": \"$TEST_IMG\"
+        }
+    }
+}" | _filter_qemu_io
+
+# This should fail (see test 072)
+$QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
+
+
+# Taken from test 071
+echo
+echo "=== Testing blkdebug ==="
+echo
+
+_make_test_img $IMG_SIZE
+
+$QEMU_IO -c 'write -P 42 0x38000 512' "$TEST_IMG" | _filter_qemu_io
+
+# The "image.filename" part tests whether "a": { "b": "c" } and "a.b": "c" do
+# the same (which they should).
+# This has to use -g to force qemu-io to use BDRV_O_PROTOCOL, since it will try
+# to determine the format of the file otherwise; due to the complexity of the
+# filename, only raw (or json itself) will work and this will then result in an
+# error because of the blkdebug part. Thus, use -g.
+$QEMU_IO -c 'read -P 42 0x38000 512' -g "json:{
+    \"driver\": \"$IMGFMT\",
+    \"file\": {
+        \"driver\": \"blkdebug\",
+        \"inject-error\": [{
+            \"event\": \"l2_load\"
+        }],
+        \"image.filename\": \"$TEST_IMG\"
+    }
+}" | _filter_qemu_io
+
+
+echo
+echo "=== Testing qemu-img info output ==="
+echo
+
+# This should output information about the image itself, not about the JSON
+# block device.
+$QEMU_IMG info "json:{\"driver\":\"qcow2\",\"file.filename\":\"$TEST_IMG\"}" \
+    | _filter_testdir | _filter_imgfmt
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
new file mode 100644
index 0000000..4804ff0
--- /dev/null
+++ b/tests/qemu-iotests/089.out
@@ -0,0 +1,39 @@
+QA output created by 089
+
+=== Testing nested image formats ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 1024
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 1024
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Pattern verification failed at offset 0, 512 bytes
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing blkdebug ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+wrote 512/512 bytes at offset 229376
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read failed: Input/output error
+
+=== Testing qemu-img info output ===
+
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+disk size: 324K
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index ae09663..7f4b56b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -95,4 +95,5 @@
 086 rw auto quick
 087 rw auto
 088 rw auto
+089 rw auto quick
 090 rw auto quick
-- 
1.9.2

Re: [Qemu-devel] [PATCH 3/4] block: Allow JSON filenames

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]

On 05/06/2014 01:30 PM, Max Reitz wrote:
> If the filename given to bdrv_open() is prefixed with "json:", parse the
> rest as a JSON object and use the result as the options QDict.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 

>  /*
>   * Opens a disk image (raw, qcow2, vmdk, ...)
>   *
> @@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>          options = qdict_new();
>      }
>  
> +    if (filename && g_str_has_prefix(filename, "json:")) {
> +        QDict *json_options = parse_json_filename(filename, &local_err);
> +        if (local_err) {
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +
> +        qdict_join(options, json_options, true);
> +        assert(qdict_size(json_options) == 0);

Would it be better to pass false to qdict_join(), and then raise an
error if the user specified conflicting options?  For example (untested,
just typing off the top of my head here),

-drive
file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2

looks like it specifies conflicting backing.file.driver options.
Passing true means that qdict_join silently overwrites the value in
options to instead be the value in the json string; passing false means
you could flag the user error.

> +        QDECREF(json_options);
> +
> +        filename = NULL;
> +    }
> +
>      bs->options = options;
>      options = qdict_clone_shallow(options);
>  
> 

-- 
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 --]

Re: [Qemu-devel] [PATCH 3/4] block: Allow JSON filenames

[Max Reitz]

On 06.05.2014 21:57, Eric Blake wrote:
> On 05/06/2014 01:30 PM, Max Reitz wrote:
>> If the filename given to bdrv_open() is prefixed with "json:", parse the
>> rest as a JSON object and use the result as the options QDict.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 41 insertions(+)
>>
>>   /*
>>    * Opens a disk image (raw, qcow2, vmdk, ...)
>>    *
>> @@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>>           options = qdict_new();
>>       }
>>   
>> +    if (filename && g_str_has_prefix(filename, "json:")) {
>> +        QDict *json_options = parse_json_filename(filename, &local_err);
>> +        if (local_err) {
>> +            ret = -EINVAL;
>> +            goto fail;
>> +        }
>> +
>> +        qdict_join(options, json_options, true);
>> +        assert(qdict_size(json_options) == 0);
> Would it be better to pass false to qdict_join(), and then raise an
> error if the user specified conflicting options?  For example (untested,
> just typing off the top of my head here),
>
> -drive
> file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2
>
> looks like it specifies conflicting backing.file.driver options.
> Passing true means that qdict_join silently overwrites the value in
> options to instead be the value in the json string; passing false means
> you could flag the user error.

Yes, you're right; I'll change it.

Max

>> +        QDECREF(json_options);
>> +
>> +        filename = NULL;
>> +    }
>> +
>>       bs->options = options;
>>       options = qdict_clone_shallow(options);
>>

Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for the JSON protocol

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

On 05/06/2014 01:30 PM, Max Reitz wrote:
> Add a test for the JSON protocol driver.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> ---


> +echo
> +echo "=== Testing qemu-img info output ==="
> +echo
> +
> +# This should output information about the image itself, not about the JSON
> +# block device.

Is this comment a bit stale, now that there is no separate JSON block
device?

-- 
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 --]

Re: [Qemu-devel] [PATCH 4/4] iotests: Add test for the JSON protocol

[Max Reitz]

On 06.05.2014 22:04, Eric Blake wrote:
> On 05/06/2014 01:30 PM, Max Reitz wrote:
>> Add a test for the JSON protocol driver.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Benoit Canet <benoit@irqsave.net>
>> ---
>
>> +echo
>> +echo "=== Testing qemu-img info output ==="
>> +echo
>> +
>> +# This should output information about the image itself, not about the JSON
>> +# block device.
> Is this comment a bit stale, now that there is no separate JSON block
> device?

Probably, yes, I just took it from the old series and was glad it just 
worked. :-)

Max

Re: [Qemu-devel] [PATCH 3/4] block: Allow JSON filenames

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

On 05/06/2014 02:00 PM, Max Reitz wrote:

>>
>> -drive
>> file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2
>>
>>
>> looks like it specifies conflicting backing.file.driver options.
>> Passing true means that qdict_join silently overwrites the value in
>> options to instead be the value in the json string; passing false means
>> you could flag the user error.
> 
> Yes, you're right; I'll change it.

And of course, bonus points if you enhance the testsuite to provoke the
error and prove we detect 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 --]

Re: [Qemu-devel] [PATCH 3/4] block: Allow JSON filenames

[Max Reitz]

On 06.05.2014 22:28, Eric Blake wrote:
> On 05/06/2014 02:00 PM, Max Reitz wrote:
>
>>> -drive
>>> file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2
>>>
>>>
>>> looks like it specifies conflicting backing.file.driver options.
>>> Passing true means that qdict_join silently overwrites the value in
>>> options to instead be the value in the json string; passing false means
>>> you could flag the user error.
>> Yes, you're right; I'll change it.
> And of course, bonus points if you enhance the testsuite to provoke the
> error and prove we detect it :)

Already done. ;-)

Max

Re: [Qemu-devel] [PATCH 3/4] block: Allow JSON filenames

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]

Am 06.05.2014 um 21:57 hat Eric Blake geschrieben:
> On 05/06/2014 01:30 PM, Max Reitz wrote:
> > If the filename given to bdrv_open() is prefixed with "json:", parse the
> > rest as a JSON object and use the result as the options QDict.
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> 
> >  /*
> >   * Opens a disk image (raw, qcow2, vmdk, ...)
> >   *
> > @@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
> >          options = qdict_new();
> >      }
> >  
> > +    if (filename && g_str_has_prefix(filename, "json:")) {
> > +        QDict *json_options = parse_json_filename(filename, &local_err);
> > +        if (local_err) {
> > +            ret = -EINVAL;
> > +            goto fail;
> > +        }
> > +
> > +        qdict_join(options, json_options, true);
> > +        assert(qdict_size(json_options) == 0);
> 
> Would it be better to pass false to qdict_join(), and then raise an
> error if the user specified conflicting options?  For example (untested,
> just typing off the top of my head here),
> 
> -drive
> file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2
> 
> looks like it specifies conflicting backing.file.driver options.
> Passing true means that qdict_join silently overwrites the value in
> options to instead be the value in the json string; passing false means
> you could flag the user error.

Isn't the more realistic case, that 'file' is actually the backing file
string stored in an image, and the overwriting option comes from the
command line? In this case, I think we want to allow overriding the
option stored in the qcow2 file.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 3/4] block: Allow JSON filenames

[Max Reitz]

On 07.05.2014 10:39, Kevin Wolf wrote:
> Am 06.05.2014 um 21:57 hat Eric Blake geschrieben:
>> On 05/06/2014 01:30 PM, Max Reitz wrote:
>>> If the filename given to bdrv_open() is prefixed with "json:", parse the
>>> rest as a JSON object and use the result as the options QDict.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   block.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 41 insertions(+)
>>>
>>>   /*
>>>    * Opens a disk image (raw, qcow2, vmdk, ...)
>>>    *
>>> @@ -1337,6 +1364,20 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>>>           options = qdict_new();
>>>       }
>>>   
>>> +    if (filename && g_str_has_prefix(filename, "json:")) {
>>> +        QDict *json_options = parse_json_filename(filename, &local_err);
>>> +        if (local_err) {
>>> +            ret = -EINVAL;
>>> +            goto fail;
>>> +        }
>>> +
>>> +        qdict_join(options, json_options, true);
>>> +        assert(qdict_size(json_options) == 0);
>> Would it be better to pass false to qdict_join(), and then raise an
>> error if the user specified conflicting options?  For example (untested,
>> just typing off the top of my head here),
>>
>> -drive
>> file='json:{"driver":"qcow2","file.filename":"foo","backing.file.driver":"raw"}',backing.file.driver=qcow2
>>
>> looks like it specifies conflicting backing.file.driver options.
>> Passing true means that qdict_join silently overwrites the value in
>> options to instead be the value in the json string; passing false means
>> you could flag the user error.
> Isn't the more realistic case, that 'file' is actually the backing file
> string stored in an image, and the overwriting option comes from the
> command line? In this case, I think we want to allow overriding the
> option stored in the qcow2 file.

Yes, you're probably right. I'll drop outputting an error message for 
conflicting entries from v2 in v3.

Max