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

[Qemu-devel] [PATCH v2 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.


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 v1:

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:[0014] [FC] 'block: Do not prematurely remove "filename"'
002/2:[0002] [FC] '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                    | 24 +++++++++++-----
 tests/qemu-iotests/097     | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/097.out | 13 +++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 103 insertions(+), 7 deletions(-)
 create mode 100755 tests/qemu-iotests/097
 create mode 100644 tests/qemu-iotests/097.out

-- 
2.0.0

[Qemu-devel] [PATCH v2 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 | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index ff44e76..850c5d7 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 parsed_filename,
+    Error **errp)
 {
     int ret, open_flags;
     const char *filename;
@@ -954,6 +955,14 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
         bs->filename[0] = '\0';
     }
 
+    if (parsed_filename && !drv->bdrv_needs_filename) {
+        qdict_del(options, "filename");
+        /* The pointer "filename" may reference the just deleted QDict entry; in
+         * any case, it is no longer needed, so indicate that by setting it to
+         * NULL. */
+        filename = NULL;
+    }
+
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
 
@@ -1020,7 +1029,7 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
 {
     BlockDriver *drv;
     const char *drvname;
-    bool parse_filename = false;
+    bool parse_filename = false, parsed_filename = false;
     Error *local_err = NULL;
     int ret;
 
@@ -1070,18 +1079,19 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename,
             goto fail;
         }
 
-        if (!drv->bdrv_needs_filename) {
-            qdict_del(*options, "filename");
-        } else {
+        if (drv->bdrv_needs_filename) {
             filename = qdict_get_str(*options, "filename");
         }
+
+        parsed_filename = true;
     }
 
     if (!drv->bdrv_file_open) {
         ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err);
         *options = NULL;
     } else {
-        ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err);
+        ret = bdrv_open_common(bs, NULL, *options, flags, drv, parsed_filename,
+                               &local_err);
     }
     if (ret < 0) {
         error_propagate(errp, local_err);
@@ -1471,7 +1481,7 @@ 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, false, &local_err);
     if (ret < 0) {
         goto fail;
     }
-- 
2.0.0

[Qemu-devel] [PATCH v2 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 v2 1/2] block: Do not prematurely remove "filename"

[Eric Blake]

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

On 06/26/2014 01:09 PM, Max Reitz wrote:
> 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 | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 

How does this play with Kevin's bdrv_open cleanups?
https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg06173.html

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

[Max Reitz]

On 26.06.2014 21:46, Eric Blake wrote:
> On 06/26/2014 01:09 PM, Max Reitz wrote:
>> 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 | 24 +++++++++++++++++-------
>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>
> How does this play with Kevin's bdrv_open cleanups?
> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg06173.html

To be honest, I hoped to get this series merged before Kevin's, because 
I guessed adapting his series to this would be easier than the other way 
round. ;-)

I guess there will be some conflicts but nothing unfixable (and the 
concept will stay the same), as he still leaves bdrv_open_common() intact.

Max

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

[Max Reitz]

On 26.06.2014 22:01, Max Reitz wrote:
> On 26.06.2014 21:46, Eric Blake wrote:
>> On 06/26/2014 01:09 PM, Max Reitz wrote:
>>> 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 | 24 +++++++++++++++++-------
>>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>>
>> How does this play with Kevin's bdrv_open cleanups?
>> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg06173.html
>
> To be honest, I hoped to get this series merged before Kevin's, 
> because I guessed adapting his series to this would be easier than the 
> other way round. ;-)
>
> I guess there will be some conflicts but nothing unfixable (and the 
> concept will stay the same), as he still leaves bdrv_open_common() intact.

Hm, now that I'm seeing Kevin merged his series already, I'll see to fix 
eventual conflicts myself.

Max