[Qemu-devel] [PATCH for-2.12 0/3] block: Handle null backing link

[Qemu-devel] [PATCH for-2.12 0/3] block: Handle null backing link

[Max Reitz]

Currently, we try to rewrite every occurrence of "backing": null into
"backing": "" in qmp_blockdev_add().  However, that breaks using the
same "backing": null construction in json:{} file names (which do not go
through qmp_blockdev_add()).  Currently, these then just behave as if
the option has not been specified.

Since there is actually only one place where we evaluate the @backing
option to find out whether not to use a backing file, we can instead
just check for null there.  It doesn't matter that this changes the
runtime state of the option from "" to null, because nobody really does
anything with that runtime state anyway (except put it into qemu again,
but qemu doesn't care whether it's "" or null).

And in the future, it's much better if we get it to be null in that
runtime state sooner than later -- see patch 3. :-)


Max Reitz (3):
  qapi: Add qdict_is_null()
  block: Handle null backing link
  block: Deprecate "backing": ""

 qapi/block-core.json       |  4 ++--
 include/qapi/qmp/qdict.h   |  1 +
 block.c                    |  6 +++++-
 blockdev.c                 | 14 --------------
 qobject/qdict.c            | 10 ++++++++++
 qemu-doc.texi              |  7 +++++++
 qemu-options.hx            |  4 ++--
 tests/qemu-iotests/089     | 20 ++++++++++++++++++++
 tests/qemu-iotests/089.out |  8 ++++++++
 9 files changed, 55 insertions(+), 19 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qapi/qmp/qdict.h |  1 +
 qobject/qdict.c          | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index fc218e7be6..c65ebfc748 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
                           int64_t def_value);
 bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
 const char *qdict_get_try_str(const QDict *qdict, const char *key);
+bool qdict_is_qnull(const QDict *qdict, const char *key);
 
 void qdict_copy_default(QDict *dst, QDict *src, const char *key);
 void qdict_set_default_str(QDict *dst, const char *key, const char *val);
diff --git a/qobject/qdict.c b/qobject/qdict.c
index e8f15f1132..a032ea629a 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key)
 }
 
 /**
+ * qdict_is_qnull(): Return true if the value for 'key' is QNull
+ */
+bool qdict_is_qnull(const QDict *qdict, const char *key)
+{
+    QObject *value = qdict_get(qdict, key);
+
+    return value && value->type == QTYPE_QNULL;
+}
+
+/**
  * qdict_iter(): Iterate over all the dictionary's stored values.
  *
  * This function allows the user to provide an iterator, which will be
-- 
2.13.6

[Qemu-devel] [PATCH for-2.12 2/3] block: Handle null backing link

[Max Reitz]

Instead of converting all "backing": null instances into "backing": "",
handle a null value directly in bdrv_open_inherit().

This enables explicitly null backing links for json:{} filenames.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                    |  2 +-
 blockdev.c                 | 14 --------------
 tests/qemu-iotests/089     | 20 ++++++++++++++++++++
 tests/qemu-iotests/089.out |  8 ++++++++
 4 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index 85427df577..bc92ddd5a0 100644
--- a/block.c
+++ b/block.c
@@ -2558,7 +2558,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
 
     /* See cautionary note on accessing @options above */
     backing = qdict_get_try_str(options, "backing");
-    if (backing && *backing == '\0') {
+    if (qdict_is_qnull(options, "backing") || (backing && *backing == '\0')) {
         flags |= BDRV_O_NO_BACKING;
         qdict_del(options, "backing");
     }
diff --git a/blockdev.c b/blockdev.c
index 56a6b24a0b..99ef618b39 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3885,7 +3885,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
     QObject *obj;
     Visitor *v = qobject_output_visitor_new(&obj);
     QDict *qdict;
-    const QDictEntry *ent;
     Error *local_err = NULL;
 
     visit_type_BlockdevOptions(v, NULL, &options, &local_err);
@@ -3899,19 +3898,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 
     qdict_flatten(qdict);
 
-    /*
-     * Rewrite "backing": null to "backing": ""
-     * TODO Rewrite "" to null instead, and perhaps not even here
-     */
-    for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
-        char *dot = strrchr(ent->key, '.');
-
-        if (!strcmp(dot ? dot + 1 : ent->key, "backing")
-            && qobject_type(ent->value) == QTYPE_QNULL) {
-            qdict_put(qdict, ent->key, qstring_new());
-        }
-    }
-
     if (!qdict_get_try_str(qdict, "node-name")) {
         error_setg(errp, "'node-name' must be specified for the root node");
         goto fail;
diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
index 9bfe2307b3..832eac1248 100755
--- a/tests/qemu-iotests/089
+++ b/tests/qemu-iotests/089
@@ -82,6 +82,26 @@ $QEMU_IO_PROG --cache $CACHEMODE \
 $QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
 
 
+echo
+echo "=== Testing correct handling of 'backing':null ==="
+echo
+
+_make_test_img -b "$TEST_IMG.base" $IMG_SIZE
+
+# This should read 42
+$QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
+
+# This should read 0
+$QEMU_IO -c 'read -P 0 0 512' "json:{\
+    'driver': '$IMGFMT',
+    'file': {
+        'driver': 'file',
+        'filename': '$TEST_IMG'
+    },
+    'backing': null
+}" | _filter_qemu_io
+
+
 # Taken from test 071
 echo
 echo "=== Testing blkdebug ==="
diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
index 18f5fdda7a..607a9c6d9c 100644
--- a/tests/qemu-iotests/089.out
+++ b/tests/qemu-iotests/089.out
@@ -19,6 +19,14 @@ 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 correct handling of 'backing':null ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
+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 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
-- 
2.13.6

[Qemu-devel] [PATCH for-2.12 3/3] block: Deprecate "backing": ""

[Max Reitz]

We have a clear replacement, so let's deprecate it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 4 ++--
 block.c              | 4 ++++
 qemu-doc.texi        | 7 +++++++
 qemu-options.hx      | 4 ++--
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 76bf50f813..dfe4d3650c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1090,7 +1090,7 @@
 # @overlay: reference to the existing block device that will become
 #           the overlay of @node, as part of creating the snapshot.
 #           It must not have a current backing file (this can be
-#           achieved by passing "backing": "" to blockdev-add).
+#           achieved by passing "backing": null to blockdev-add).
 #
 # Since: 2.5
 ##
@@ -1238,7 +1238,7 @@
 #                     "node-name": "node1534",
 #                     "file": { "driver": "file",
 #                               "filename": "hd1.qcow2" },
-#                     "backing": "" } }
+#                     "backing": null } }
 #
 # <- { "return": {} }
 #
diff --git a/block.c b/block.c
index bc92ddd5a0..463c4de25b 100644
--- a/block.c
+++ b/block.c
@@ -2559,6 +2559,10 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
     /* See cautionary note on accessing @options above */
     backing = qdict_get_try_str(options, "backing");
     if (qdict_is_qnull(options, "backing") || (backing && *backing == '\0')) {
+        if (backing) {
+            warn_report("Use of \"backing\": \"\" is deprecated; "
+                        "use \"backing\": null instead");
+        }
         flags |= BDRV_O_NO_BACKING;
         qdict_del(options, "backing");
     }
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 8c10956a66..8f57d9ad21 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2537,6 +2537,13 @@ or ``ivshmem-doorbell`` device types.
 The ``spapr-pci-vfio-host-bridge'' device type is replaced by
 the ``spapr-pci-host-bridge'' device type.
 
+@section Block device options
+
+@subsection "backing": "" (since 2.12.0)
+
+In order to prevent QEMU from automatically opening an image's backing
+chain, use ``"backing": null'' instead.
+
 @node License
 @appendix License
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 3728e9b4dd..0ee1a04d00 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -731,8 +731,8 @@ Reference to or definition of the data source block driver node
 
 @item backing
 Reference to or definition of the backing file block device (default is taken
-from the image file). It is allowed to pass an empty string here in order to
-disable the default backing file.
+from the image file). It is allowed to pass @code{null} here in order to disable
+the default backing file.
 
 @item lazy-refcounts
 Whether to enable the lazy refcounts feature (on/off; default is taken from the
-- 
2.13.6

Re: [Qemu-devel] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Eric Blake]

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

On 11/10/2017 04:13 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |  1 +
>  qobject/qdict.c          | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.12 3/3] block: Deprecate "backing": ""

[Eric Blake]

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

On 11/10/2017 04:13 PM, Max Reitz wrote:
> We have a clear replacement, so let's deprecate it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/block-core.json | 4 ++--
>  block.c              | 4 ++++
>  qemu-doc.texi        | 7 +++++++
>  qemu-options.hx      | 4 ++--
>  4 files changed, 15 insertions(+), 4 deletions(-)

Dan has more details on the proper documentation to tweak when declaring
something deprecated.  So this patch is incomplete, but what you have
makes sense.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.12 2/3] block: Handle null backing link

[Eric Blake]

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

On 11/10/2017 04:13 PM, Max Reitz wrote:
> Instead of converting all "backing": null instances into "backing": "",
> handle a null value directly in bdrv_open_inherit().
> 
> This enables explicitly null backing links for json:{} filenames.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                    |  2 +-
>  blockdev.c                 | 14 --------------
>  tests/qemu-iotests/089     | 20 ++++++++++++++++++++
>  tests/qemu-iotests/089.out |  8 ++++++++
>  4 files changed, 29 insertions(+), 15 deletions(-)
> 

> @@ -3899,19 +3898,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  
>      qdict_flatten(qdict);
>  
> -    /*
> -     * Rewrite "backing": null to "backing": ""
> -     * TODO Rewrite "" to null instead, and perhaps not even here
> -     */

Nice that the TODO told you what to do :)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH for-2.12 2/3] block: Handle null backing link

[Max Reitz]

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

On 2017-11-10 23:22, Eric Blake wrote:
> On 11/10/2017 04:13 PM, Max Reitz wrote:
>> Instead of converting all "backing": null instances into "backing": "",
>> handle a null value directly in bdrv_open_inherit().
>>
>> This enables explicitly null backing links for json:{} filenames.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c                    |  2 +-
>>  blockdev.c                 | 14 --------------
>>  tests/qemu-iotests/089     | 20 ++++++++++++++++++++
>>  tests/qemu-iotests/089.out |  8 ++++++++
>>  4 files changed, 29 insertions(+), 15 deletions(-)
>>
> 
>> @@ -3899,19 +3898,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>>  
>>      qdict_flatten(qdict);
>>  
>> -    /*
>> -     * Rewrite "backing": null to "backing": ""
>> -     * TODO Rewrite "" to null instead, and perhaps not even here
>> -     */
> 
> Nice that the TODO told you what to do :)

Well, not really, because I disagree that it needs to be rewritten at
all.  I think we just need to deprecate and later disallow "backing":
"", which would absolve us from all of the rewriting trouble.

This code was added here because Markus needed to allow "backing": null
in a hurry (as far as I remember), so he added it centrally here instead
of checking how many places there are that evaluate "backing": "".

I should maybe have said that patch 3 is more of an RFC.  I'm not sure
whether other people agree that "backing": "" should be deprecated --
and if not, we would have to rewrite it still.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.12 3/3] block: Deprecate "backing": ""

[Daniel P. Berrange]

On Fri, Nov 10, 2017 at 04:21:05PM -0600, Eric Blake wrote:
> On 11/10/2017 04:13 PM, Max Reitz wrote:
> > We have a clear replacement, so let's deprecate it.
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  qapi/block-core.json | 4 ++--
> >  block.c              | 4 ++++
> >  qemu-doc.texi        | 7 +++++++
> >  qemu-options.hx      | 4 ++--
> >  4 files changed, 15 insertions(+), 4 deletions(-)
> 
> Dan has more details on the proper documentation to tweak when declaring
> something deprecated.  So this patch is incomplete, but what you have
> makes sense.

Its ok, Max has added to the relevant appendix in qemu-doc.texi

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12 2/3] block: Handle null backing link

[Alberto Garcia]

On Fri 10 Nov 2017 11:13:28 PM CET, Max Reitz wrote:
> Instead of converting all "backing": null instances into "backing": "",
> handle a null value directly in bdrv_open_inherit().
>
> This enables explicitly null backing links for json:{} filenames.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Alberto Garcia]

On Fri 10 Nov 2017 11:13:27 PM CET, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |  1 +
>  qobject/qdict.c          | 10 ++++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index fc218e7be6..c65ebfc748 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
>                            int64_t def_value);
>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
> +bool qdict_is_qnull(const QDict *qdict, const char *key);

I found this name a bit confusing, how about something like
qdict_entry_is_qnull() ?

I'm fine if you want to keep it like this though,

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Max Reitz]

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

On 2017-11-14 15:07, Alberto Garcia wrote:
> On Fri 10 Nov 2017 11:13:27 PM CET, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/qapi/qmp/qdict.h |  1 +
>>  qobject/qdict.c          | 10 ++++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
>> index fc218e7be6..c65ebfc748 100644
>> --- a/include/qapi/qmp/qdict.h
>> +++ b/include/qapi/qmp/qdict.h
>> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
>>                            int64_t def_value);
>>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
>>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
>> +bool qdict_is_qnull(const QDict *qdict, const char *key);
> 
> I found this name a bit confusing, how about something like
> qdict_entry_is_qnull() ?

Yes, that sounds better.  Thanks!

Max

> I'm fine if you want to keep it like this though,
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Berto
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |  1 +
>  qobject/qdict.c          | 10 ++++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index fc218e7be6..c65ebfc748 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
>                            int64_t def_value);
>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
> +bool qdict_is_qnull(const QDict *qdict, const char *key);
>  
>  void qdict_copy_default(QDict *dst, QDict *src, const char *key);
>  void qdict_set_default_str(QDict *dst, const char *key, const char *val);
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index e8f15f1132..a032ea629a 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key)
>  }
>  
>  /**
> + * qdict_is_qnull(): Return true if the value for 'key' is QNull
> + */
> +bool qdict_is_qnull(const QDict *qdict, const char *key)
> +{
> +    QObject *value = qdict_get(qdict, key);
> +
> +    return value && value->type == QTYPE_QNULL;
> +}
> +
> +/**
>   * qdict_iter(): Iterate over all the dictionary's stored values.
>   *
>   * This function allows the user to provide an iterator, which will be

As far as I can tell, the new helper function is going to be used just
once, by bdrv_open_inherit() in PATCH 2:

    qdict_is_qnull(options, "backing")

I dislike abstracting from just one concrete instance.

Perhaps a more general helper could be more generally useful.  Something
like:

    qobject_is(qdict_get(options, "backing", QTYPE_QNULL))

There are numerous instances of

    !obj || qobject_type(obj) == T

in the tree, which could then be replaced by

    qobject_is(obj, T)

An alternative helper: macro qobject_dynamic_cast(obj, T) that returns
(T *)obj if obj is a T, else null.  Leads to something like

    qobject_dynamic_cast(qdict_get(options, "backing", QNull))

Re: [Qemu-devel] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Max Reitz]

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

On 2017-11-14 15:57, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/qapi/qmp/qdict.h |  1 +
>>  qobject/qdict.c          | 10 ++++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
>> index fc218e7be6..c65ebfc748 100644
>> --- a/include/qapi/qmp/qdict.h
>> +++ b/include/qapi/qmp/qdict.h
>> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
>>                            int64_t def_value);
>>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
>>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
>> +bool qdict_is_qnull(const QDict *qdict, const char *key);
>>  
>>  void qdict_copy_default(QDict *dst, QDict *src, const char *key);
>>  void qdict_set_default_str(QDict *dst, const char *key, const char *val);
>> diff --git a/qobject/qdict.c b/qobject/qdict.c
>> index e8f15f1132..a032ea629a 100644
>> --- a/qobject/qdict.c
>> +++ b/qobject/qdict.c
>> @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key)
>>  }
>>  
>>  /**
>> + * qdict_is_qnull(): Return true if the value for 'key' is QNull
>> + */
>> +bool qdict_is_qnull(const QDict *qdict, const char *key)
>> +{
>> +    QObject *value = qdict_get(qdict, key);
>> +
>> +    return value && value->type == QTYPE_QNULL;
>> +}
>> +
>> +/**
>>   * qdict_iter(): Iterate over all the dictionary's stored values.
>>   *
>>   * This function allows the user to provide an iterator, which will be
> 
> As far as I can tell, the new helper function is going to be used just
> once, by bdrv_open_inherit() in PATCH 2:
> 
>     qdict_is_qnull(options, "backing")
> 
> I dislike abstracting from just one concrete instance.
> 
> Perhaps a more general helper could be more generally useful.  Something
> like:
> 
>     qobject_is(qdict_get(options, "backing", QTYPE_QNULL))
> 
> There are numerous instances of
> 
>     !obj || qobject_type(obj) == T
> 
> in the tree, which could then be replaced by
> 
>     qobject_is(obj, T)
> 
> An alternative helper: macro qobject_dynamic_cast(obj, T) that returns
> (T *)obj if obj is a T, else null.  Leads to something like
> 
>     qobject_dynamic_cast(qdict_get(options, "backing", QNull))

If you think that's good, then that's good -- you know the QAPI code
better then me, after all.

To explain myself: I thought it would be the natural extension of the
qdict_get_try_*() functions for the QNull type.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.12 2/3] block: Handle null backing link

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Instead of converting all "backing": null instances into "backing": "",
> handle a null value directly in bdrv_open_inherit().
>
> This enables explicitly null backing links for json:{} filenames.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                    |  2 +-
>  blockdev.c                 | 14 --------------
>  tests/qemu-iotests/089     | 20 ++++++++++++++++++++
>  tests/qemu-iotests/089.out |  8 ++++++++
>  4 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/block.c b/block.c
> index 85427df577..bc92ddd5a0 100644
> --- a/block.c
> +++ b/block.c
> @@ -2558,7 +2558,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>  
>      /* See cautionary note on accessing @options above */
>      backing = qdict_get_try_str(options, "backing");
> -    if (backing && *backing == '\0') {
> +    if (qdict_is_qnull(options, "backing") || (backing && *backing == '\0')) {
>          flags |= BDRV_O_NO_BACKING;
>          qdict_del(options, "backing");
>      }

Consider a comment pointing out "" is deprecated.

See also my reply to PATCH 1/3.

> diff --git a/blockdev.c b/blockdev.c
> index 56a6b24a0b..99ef618b39 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3885,7 +3885,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>      QObject *obj;
>      Visitor *v = qobject_output_visitor_new(&obj);
>      QDict *qdict;
> -    const QDictEntry *ent;
>      Error *local_err = NULL;
>  
>      visit_type_BlockdevOptions(v, NULL, &options, &local_err);
> @@ -3899,19 +3898,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  
>      qdict_flatten(qdict);
>  
> -    /*
> -     * Rewrite "backing": null to "backing": ""
> -     * TODO Rewrite "" to null instead, and perhaps not even here
> -     */
> -    for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
> -        char *dot = strrchr(ent->key, '.');
> -
> -        if (!strcmp(dot ? dot + 1 : ent->key, "backing")
> -            && qobject_type(ent->value) == QTYPE_QNULL) {
> -            qdict_put(qdict, ent->key, qstring_new());
> -        }
> -    }
> -
>      if (!qdict_get_try_str(qdict, "node-name")) {
>          error_setg(errp, "'node-name' must be specified for the root node");
>          goto fail;
> diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
> index 9bfe2307b3..832eac1248 100755
> --- a/tests/qemu-iotests/089
> +++ b/tests/qemu-iotests/089
> @@ -82,6 +82,26 @@ $QEMU_IO_PROG --cache $CACHEMODE \
>  $QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
>  
>  
> +echo
> +echo "=== Testing correct handling of 'backing':null ==="
> +echo
> +
> +_make_test_img -b "$TEST_IMG.base" $IMG_SIZE
> +
> +# This should read 42
> +$QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
> +
> +# This should read 0
> +$QEMU_IO -c 'read -P 0 0 512' "json:{\
> +    'driver': '$IMGFMT',
> +    'file': {
> +        'driver': 'file',
> +        'filename': '$TEST_IMG'
> +    },
> +    'backing': null
> +}" | _filter_qemu_io
> +
> +
>  # Taken from test 071
>  echo
>  echo "=== Testing blkdebug ==="
> diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
> index 18f5fdda7a..607a9c6d9c 100644
> --- a/tests/qemu-iotests/089.out
> +++ b/tests/qemu-iotests/089.out
> @@ -19,6 +19,14 @@ 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 correct handling of 'backing':null ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
> +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 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

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH for-2.12 3/3] block: Deprecate "backing": ""

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> We have a clear replacement, so let's deprecate it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qapi/block-core.json | 4 ++--
>  block.c              | 4 ++++
>  qemu-doc.texi        | 7 +++++++
>  qemu-options.hx      | 4 ++--
>  4 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 76bf50f813..dfe4d3650c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1090,7 +1090,7 @@
>  # @overlay: reference to the existing block device that will become
>  #           the overlay of @node, as part of creating the snapshot.
>  #           It must not have a current backing file (this can be
> -#           achieved by passing "backing": "" to blockdev-add).
> +#           achieved by passing "backing": null to blockdev-add).
>  #
>  # Since: 2.5
>  ##
> @@ -1238,7 +1238,7 @@
>  #                     "node-name": "node1534",
>  #                     "file": { "driver": "file",
>  #                               "filename": "hd1.qcow2" },
> -#                     "backing": "" } }
> +#                     "backing": null } }
>  #
>  # <- { "return": {} }
>  #
> diff --git a/block.c b/block.c
> index bc92ddd5a0..463c4de25b 100644
> --- a/block.c
> +++ b/block.c
> @@ -2559,6 +2559,10 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>      /* See cautionary note on accessing @options above */
>      backing = qdict_get_try_str(options, "backing");
>      if (qdict_is_qnull(options, "backing") || (backing && *backing == '\0')) {
> +        if (backing) {
> +            warn_report("Use of \"backing\": \"\" is deprecated; "
> +                        "use \"backing\": null instead");
> +        }
>          flags |= BDRV_O_NO_BACKING;
>          qdict_del(options, "backing");
>      }
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 8c10956a66..8f57d9ad21 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -2537,6 +2537,13 @@ or ``ivshmem-doorbell`` device types.
>  The ``spapr-pci-vfio-host-bridge'' device type is replaced by
>  the ``spapr-pci-host-bridge'' device type.
>  
> +@section Block device options
> +
> +@subsection "backing": "" (since 2.12.0)
> +
> +In order to prevent QEMU from automatically opening an image's backing
> +chain, use ``"backing": null'' instead.
> +
>  @node License
>  @appendix License
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 3728e9b4dd..0ee1a04d00 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -731,8 +731,8 @@ Reference to or definition of the data source block driver node
>  
>  @item backing
>  Reference to or definition of the backing file block device (default is taken
> -from the image file). It is allowed to pass an empty string here in order to
> -disable the default backing file.
> +from the image file). It is allowed to pass @code{null} here in order to disable
> +the default backing file.
>  
>  @item lazy-refcounts
>  Whether to enable the lazy refcounts feature (on/off; default is taken from the

Missed in commit c42e8742f, I guess.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH for-2.12 2/3] block: Handle null backing link

[Max Reitz]

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

On 2017-11-14 16:17, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> Instead of converting all "backing": null instances into "backing": "",
>> handle a null value directly in bdrv_open_inherit().
>>
>> This enables explicitly null backing links for json:{} filenames.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c                    |  2 +-
>>  blockdev.c                 | 14 --------------
>>  tests/qemu-iotests/089     | 20 ++++++++++++++++++++
>>  tests/qemu-iotests/089.out |  8 ++++++++
>>  4 files changed, 29 insertions(+), 15 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 85427df577..bc92ddd5a0 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2558,7 +2558,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>>  
>>      /* See cautionary note on accessing @options above */
>>      backing = qdict_get_try_str(options, "backing");
>> -    if (backing && *backing == '\0') {
>> +    if (qdict_is_qnull(options, "backing") || (backing && *backing == '\0')) {
>>          flags |= BDRV_O_NO_BACKING;
>>          qdict_del(options, "backing");
>>      }
> 
> Consider a comment pointing out "" is deprecated.

Would not really be necessary after patch 3, but let's see how well
patch 3 is received. :-)

(And in any case, a comment certainly won't hurt.)

Max

> See also my reply to PATCH 1/3.
> 
>> diff --git a/blockdev.c b/blockdev.c
>> index 56a6b24a0b..99ef618b39 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3885,7 +3885,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>>      QObject *obj;
>>      Visitor *v = qobject_output_visitor_new(&obj);
>>      QDict *qdict;
>> -    const QDictEntry *ent;
>>      Error *local_err = NULL;
>>  
>>      visit_type_BlockdevOptions(v, NULL, &options, &local_err);
>> @@ -3899,19 +3898,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>>  
>>      qdict_flatten(qdict);
>>  
>> -    /*
>> -     * Rewrite "backing": null to "backing": ""
>> -     * TODO Rewrite "" to null instead, and perhaps not even here
>> -     */
>> -    for (ent = qdict_first(qdict); ent; ent = qdict_next(qdict, ent)) {
>> -        char *dot = strrchr(ent->key, '.');
>> -
>> -        if (!strcmp(dot ? dot + 1 : ent->key, "backing")
>> -            && qobject_type(ent->value) == QTYPE_QNULL) {
>> -            qdict_put(qdict, ent->key, qstring_new());
>> -        }
>> -    }
>> -
>>      if (!qdict_get_try_str(qdict, "node-name")) {
>>          error_setg(errp, "'node-name' must be specified for the root node");
>>          goto fail;
>> diff --git a/tests/qemu-iotests/089 b/tests/qemu-iotests/089
>> index 9bfe2307b3..832eac1248 100755
>> --- a/tests/qemu-iotests/089
>> +++ b/tests/qemu-iotests/089
>> @@ -82,6 +82,26 @@ $QEMU_IO_PROG --cache $CACHEMODE \
>>  $QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
>>  
>>  
>> +echo
>> +echo "=== Testing correct handling of 'backing':null ==="
>> +echo
>> +
>> +_make_test_img -b "$TEST_IMG.base" $IMG_SIZE
>> +
>> +# This should read 42
>> +$QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
>> +
>> +# This should read 0
>> +$QEMU_IO -c 'read -P 0 0 512' "json:{\
>> +    'driver': '$IMGFMT',
>> +    'file': {
>> +        'driver': 'file',
>> +        'filename': '$TEST_IMG'
>> +    },
>> +    'backing': null
>> +}" | _filter_qemu_io
>> +
>> +
>>  # Taken from test 071
>>  echo
>>  echo "=== Testing blkdebug ==="
>> diff --git a/tests/qemu-iotests/089.out b/tests/qemu-iotests/089.out
>> index 18f5fdda7a..607a9c6d9c 100644
>> --- a/tests/qemu-iotests/089.out
>> +++ b/tests/qemu-iotests/089.out
>> @@ -19,6 +19,14 @@ 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 correct handling of 'backing':null ===
>> +
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
>> +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 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
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.12 1/3] qapi: Add qdict_is_null()

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2017-11-14 15:57, Markus Armbruster wrote:
>> Max Reitz <mreitz@redhat.com> writes:
>> 
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>  include/qapi/qmp/qdict.h |  1 +
>>>  qobject/qdict.c          | 10 ++++++++++
>>>  2 files changed, 11 insertions(+)
>>>
>>> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
>>> index fc218e7be6..c65ebfc748 100644
>>> --- a/include/qapi/qmp/qdict.h
>>> +++ b/include/qapi/qmp/qdict.h
>>> @@ -76,6 +76,7 @@ int64_t qdict_get_try_int(const QDict *qdict, const char *key,
>>>                            int64_t def_value);
>>>  bool qdict_get_try_bool(const QDict *qdict, const char *key, bool def_value);
>>>  const char *qdict_get_try_str(const QDict *qdict, const char *key);
>>> +bool qdict_is_qnull(const QDict *qdict, const char *key);
>>>  
>>>  void qdict_copy_default(QDict *dst, QDict *src, const char *key);
>>>  void qdict_set_default_str(QDict *dst, const char *key, const char *val);
>>> diff --git a/qobject/qdict.c b/qobject/qdict.c
>>> index e8f15f1132..a032ea629a 100644
>>> --- a/qobject/qdict.c
>>> +++ b/qobject/qdict.c
>>> @@ -294,6 +294,16 @@ const char *qdict_get_try_str(const QDict *qdict, const char *key)
>>>  }
>>>  
>>>  /**
>>> + * qdict_is_qnull(): Return true if the value for 'key' is QNull
>>> + */
>>> +bool qdict_is_qnull(const QDict *qdict, const char *key)
>>> +{
>>> +    QObject *value = qdict_get(qdict, key);
>>> +
>>> +    return value && value->type == QTYPE_QNULL;
>>> +}
>>> +
>>> +/**
>>>   * qdict_iter(): Iterate over all the dictionary's stored values.
>>>   *
>>>   * This function allows the user to provide an iterator, which will be
>> 
>> As far as I can tell, the new helper function is going to be used just
>> once, by bdrv_open_inherit() in PATCH 2:
>> 
>>     qdict_is_qnull(options, "backing")
>> 
>> I dislike abstracting from just one concrete instance.
>> 
>> Perhaps a more general helper could be more generally useful.  Something
>> like:
>> 
>>     qobject_is(qdict_get(options, "backing", QTYPE_QNULL))
>> 
>> There are numerous instances of
>> 
>>     !obj || qobject_type(obj) == T
>> 
>> in the tree, which could then be replaced by
>> 
>>     qobject_is(obj, T)
>> 
>> An alternative helper: macro qobject_dynamic_cast(obj, T) that returns
>> (T *)obj if obj is a T, else null.  Leads to something like
>> 
>>     qobject_dynamic_cast(qdict_get(options, "backing", QNull))
>
> If you think that's good, then that's good -- you know the QAPI code
> better then me, after all.

I'll play with it today.

> To explain myself: I thought it would be the natural extension of the
> qdict_get_try_*() functions for the QNull type.

I see now.  The name qdict_get_try_null() would make it obvious, but
what to return on success...