[Qemu-devel] [PATCH v3 0/2] block: Fix unset "filename" for certain drivers

[Qemu-devel] [PATCH v3 0/2] block: Fix unset "filename" for certain drivers

[Max Reitz]

For some protocol block drivers, the "filename" attribute in their BDSs
is unset due to bdrv_file_open() removing it from the options QDict
before bdrv_open_common() is able to copy it into the BDS. Fix this by
not removing it until until bdrv_open_common() has indeed copied it.


v3:
 - Patch 1: Rebased onto Kevin's block branch, especially his series
   "bdrv_open() cleanups, part 1" [Eric]

v2:
 - Patch 1: Only remove "filename" from the QDict if it has been parsed
   before; this prevents removal of the entry if it is given for some
   driver which supports neither parsing not filenames at all (such as
   all non-protocol drivers) which would then break test 051 [Stefan]
 - Patch 2: Fixed the headline of the test output


git-backport-diff against v2:

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/2:[0041] [FC] 'block: Do not prematurely remove "filename"'
002/2:[----] [--] 'iotests: Add test for set "filename" for NBD'


Max Reitz (2):
  block: Do not prematurely remove "filename"
  iotests: Add test for set "filename" for NBD

 block.c                    | 35 ++++++++++++++++++----
 tests/qemu-iotests/097     | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/097.out | 13 +++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 115 insertions(+), 6 deletions(-)
 create mode 100755 tests/qemu-iotests/097
 create mode 100644 tests/qemu-iotests/097.out

-- 
2.0.0

[Qemu-devel] [PATCH v3 1/2] block: Do not prematurely remove "filename"

[Max Reitz]

If "filename" is removed from the options QDict before entering
bdrv_open_common(), it cannot be stored in the BDS.  Therefore, wait
until it has been copied there and remove it from the options only
afterwards.

This fixes "filename" in the BDS being empty for block drivers which do
not need the filename because they have parsed it already (e.g. NBD).

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

diff --git a/block.c b/block.c
index 7d69f31..beab752 100644
--- a/block.c
+++ b/block.c
@@ -881,7 +881,8 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
  * Removes all processed options from *options.
  */
 static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
-    QDict *options, int flags, BlockDriver *drv, Error **errp)
+    QDict *options, int flags, BlockDriver *drv, bool remove_filename,
+    Error **errp)
 {
     int ret, open_flags;
     const char *filename;
@@ -955,6 +956,20 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         bs->filename[0] = '\0';
     }
 
+    if (remove_filename) {
+        qdict_del(options, "filename");
+
+        /* remove_filename is set by bdrv_fill_options() only if
+         * bdrv_needs_filename is false */
+        assert(!drv->bdrv_needs_filename);
+        /* The pointer "filename" may reference the just deleted QDict entry; in
+         * any case, it is only read once from here on and that is for
+         * determining whether it is set in case bdrv_needs_filename is true.
+         * Because bdrv_needs_filename is false here, that means it is not used
+         * at all anymore, so indicate that by setting it to NULL. */
+        filename = NULL;
+    }
+
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
 
@@ -1036,9 +1051,14 @@ static QDict *parse_json_filename(const char *filename, Error **errp)
 /*
  * Fills in default options for opening images and converts the legacy
  * filename/flags pair to option QDict entries.
+ *
+ * *remove_filename is set to true if the "filename" entry should be removed
+ * from the option QDict before drv->bdrv_open() or drv->bdrv_file_open() is
+ * called.
  */
-static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
-                             BlockDriver *drv, Error **errp)
+static int bdrv_fill_options(QDict **options, const char **pfilename,
+                             bool *remove_filename, int flags, BlockDriver *drv,
+                             Error **errp)
 {
     const char *filename = *pfilename;
     const char *drvname;
@@ -1119,7 +1139,7 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
         }
 
         if (!drv->bdrv_needs_filename) {
-            qdict_del(*options, "filename");
+            *remove_filename = true;
         }
     }
 
@@ -1368,6 +1388,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     const char *drvname;
     Error *local_err = NULL;
     int snapshot_flags = 0;
+    bool remove_filename = false;
 
     assert(pbs);
 
@@ -1407,7 +1428,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
-    ret = bdrv_fill_options(&options, &filename, flags, drv, &local_err);
+    ret = bdrv_fill_options(&options, &filename, &remove_filename, flags, drv,
+                            &local_err);
     if (local_err) {
         goto fail;
     }
@@ -1467,7 +1489,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     }
 
     /* Open the image */
-    ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
+    ret = bdrv_open_common(bs, file, options, flags, drv, remove_filename,
+                           &local_err);
     if (ret < 0) {
         goto fail;
     }
-- 
2.0.0

[Qemu-devel] [PATCH v3 2/2] iotests: Add test for set "filename" for NBD

[Max Reitz]

Add a new test for qemu-iotests which checks whether the "filename" (and
consequently the "file") attribute is set for images which are opened
over NBD.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/097     | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/097.out | 13 +++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 86 insertions(+)
 create mode 100755 tests/qemu-iotests/097
 create mode 100644 tests/qemu-iotests/097.out

diff --git a/tests/qemu-iotests/097 b/tests/qemu-iotests/097
new file mode 100755
index 0000000..c471ef2
--- /dev/null
+++ b/tests/qemu-iotests/097
@@ -0,0 +1,72 @@
+#!/bin/bash
+#
+# Test case for correct filename attribute for NBD
+#
+# 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 generic
+_supported_proto nbd
+_supported_os Linux
+
+function do_run_qemu()
+{
+    $QEMU -nographic -qmp stdio -serial none "$@"
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | sed -e "s#$TEST_IMG#TEST_IMG#g"
+}
+
+IMG_SIZE=128K
+
+echo
+echo '=== Testing NBD filename ("filename" and "file" should be set to TEST_IMG) ==='
+echo
+
+_make_test_img $IMG_SIZE
+
+run_qemu -drive file=$TEST_IMG,format=raw,if=none <<QMP
+{ 'execute': 'qmp_capabilities' }
+{ 'execute': 'query-block' }
+{ 'execute': 'quit' }
+QMP
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/097.out b/tests/qemu-iotests/097.out
new file mode 100644
index 0000000..6607683
--- /dev/null
+++ b/tests/qemu-iotests/097.out
@@ -0,0 +1,13 @@
+QA output created by 097
+
+=== Testing NBD filename ("filename" and "file" should be set to TEST_IMG) ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 
+QMP_VERSION
+{"return": {}}
+{"return": [{"device": "none0", "locked": false, "removable": true, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"virtual-size": 131072, "filename": "TEST_IMG", "format": "raw"}, "iops_wr": 0, "ro": false, "backing_file_depth": 0, "drv": "raw", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_IMG", "encryption_key_missing": false}, "tray_open": false, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0f07440..5447660 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -99,3 +99,4 @@
 090 rw auto quick
 091 rw auto
 092 rw auto quick
+097 rw auto
-- 
2.0.0

Re: [Qemu-devel] [PATCH for 2.1 v3 0/2] block: Fix unset "filename" for certain drivers

[Eric Blake]

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

On 06/26/2014 03:38 PM, Max Reitz wrote:
> For some protocol block drivers, the "filename" attribute in their BDSs
> is unset due to bdrv_file_open() removing it from the options QDict
> before bdrv_open_common() is able to copy it into the BDS. Fix this by
> not removing it until until bdrv_open_common() has indeed copied it.
> 
> 
> v3:
>  - Patch 1: Rebased onto Kevin's block branch, especially his series
>    "bdrv_open() cleanups, part 1" [Eric]

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


> Max Reitz (2):
>   block: Do not prematurely remove "filename"
>   iotests: Add test for set "filename" for NBD
> 
>  block.c                    | 35 ++++++++++++++++++----
>  tests/qemu-iotests/097     | 72 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/097.out | 13 +++++++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 115 insertions(+), 6 deletions(-)
>  create mode 100755 tests/qemu-iotests/097
>  create mode 100644 tests/qemu-iotests/097.out
> 

-- 
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 v3 1/2] block: Do not prematurely remove "filename"

[Kevin Wolf]

Am 26.06.2014 um 23:38 hat Max Reitz geschrieben:
> If "filename" is removed from the options QDict before entering
> bdrv_open_common(), it cannot be stored in the BDS.  Therefore, wait
> until it has been copied there and remove it from the options only
> afterwards.
> 
> This fixes "filename" in the BDS being empty for block drivers which do
> not need the filename because they have parsed it already (e.g. NBD).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

I can't say I like this approach. It looks a bit odd to pass a boolean
variable to bdrv_open(), and in some other function called from there
the cleanup is done that logically really belong to bdrv_fill_options().

More importantly, the goal was to get rid of the filename and handle
everything through the options so that we get a uniform state again.
This would involve replacing bs->filename by a new callback function in
BlockDriver that constructs a filename that describes the BDS. This way
we would get useful output not only for "nbd:localhost:10809", but also
for "driver=nbd,host=localhost".

In hard cases, the callback might just use "json:{...}" syntax. This
suggests that maybe in the end we'll want to have two different
callbacks, one giving a short human-readable description
('localhost:10809') and another one giving something that can be used on
the command line ('json:{"driver": "nbd", "host": "localhost", "ipv6":
true}').

Kevin

Re: [Qemu-devel] [PATCH v3 1/2] block: Do not prematurely remove "filename"

[Max Reitz]

On 30.06.2014 11:42, Kevin Wolf wrote:
> Am 26.06.2014 um 23:38 hat Max Reitz geschrieben:
>> If "filename" is removed from the options QDict before entering
>> bdrv_open_common(), it cannot be stored in the BDS.  Therefore, wait
>> until it has been copied there and remove it from the options only
>> afterwards.
>>
>> This fixes "filename" in the BDS being empty for block drivers which do
>> not need the filename because they have parsed it already (e.g. NBD).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> I can't say I like this approach. It looks a bit odd to pass a boolean
> variable to bdrv_open(), and in some other function called from there
> the cleanup is done that logically really belong to bdrv_fill_options().
>
> More importantly, the goal was to get rid of the filename and handle
> everything through the options so that we get a uniform state again.
> This would involve replacing bs->filename by a new callback function in
> BlockDriver that constructs a filename that describes the BDS. This way
> we would get useful output not only for "nbd:localhost:10809", but also
> for "driver=nbd,host=localhost".

Right. This "bug" isn't that severe either, so I guess it's fine to take 
more time to fix the general problem.

Max

> In hard cases, the callback might just use "json:{...}" syntax. This
> suggests that maybe in the end we'll want to have two different
> callbacks, one giving a short human-readable description
> ('localhost:10809') and another one giving something that can be used on
> the command line ('json:{"driver": "nbd", "host": "localhost", "ipv6":
> true}').
>
> Kevin