[Qemu-devel] [PATCH] block: don't add 'driver' to options when refering to backing via node name

[Qemu-devel] [PATCH] block: don't add 'driver' to options when refering to backing via node name

[Peter Krempa]

When refering to a backing file of an image via node name
bdrv_open_backing_file would add the 'driver' option to the option list
filling it with the backing format driver. This breaks construction of
the backing chain via -blockdev, as bdrv_open_inherit reports an error
if both 'reference' and 'options' are provided.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 block.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 46eb1728da..684cb018da 100644
--- a/block.c
+++ b/block.c
@@ -2245,7 +2245,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
         goto free_exit;
     }

-    if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
+    if (!reference &&
+        bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
         qdict_put_str(options, "driver", bs->backing_format);
     }

-- 
2.14.1

Re: [Qemu-devel] [PATCH] block: don't add 'driver' to options when refering to backing via node name

[Kevin Wolf]

Am 12.10.2017 um 16:14 hat Peter Krempa geschrieben:
> When refering to a backing file of an image via node name
> bdrv_open_backing_file would add the 'driver' option to the option list
> filling it with the backing format driver. This breaks construction of
> the backing chain via -blockdev, as bdrv_open_inherit reports an error
> if both 'reference' and 'options' are provided.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>

If you don't mind, I'd add a specific example to the commit message:

$ qemu-img create -f raw /tmp/backing.raw 64M
$ qemu-img create -f qcow2 -F raw -b /tmp/backing.raw /tmp/test.qcow2
$ qemu-system-x86_64 \
  -blockdev driver=file,filename=/tmp/backing.raw,node-name=backing \
  -blockdev driver=qcow2,file.driver=file,file.filename=/tmp/test.qcow2,node-name=root,backing=backing
qemu-system-x86_64: -blockdev driver=qcow2,file.driver=file,file.filename=/tmp/test.qcow2,node-name=root,backing=backing: Could not open backing file: Cannot reference an existing block device with additional options or a new filename

> diff --git a/block.c b/block.c
> index 46eb1728da..684cb018da 100644
> --- a/block.c
> +++ b/block.c
> @@ -2245,7 +2245,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
>          goto free_exit;
>      }
> 
> -    if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
> +    if (!reference &&
> +        bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
>          qdict_put_str(options, "driver", bs->backing_format);
>      }

Looks good to me.

Kevin

Re: [Qemu-devel] [PATCH] block: don't add 'driver' to options when refering to backing via node name

[Peter Krempa]

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

On Tue, Oct 17, 2017 at 16:41:00 +0200, Kevin Wolf wrote:
> Am 12.10.2017 um 16:14 hat Peter Krempa geschrieben:
> > When refering to a backing file of an image via node name
> > bdrv_open_backing_file would add the 'driver' option to the option list
> > filling it with the backing format driver. This breaks construction of
> > the backing chain via -blockdev, as bdrv_open_inherit reports an error
> > if both 'reference' and 'options' are provided.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> 
> If you don't mind, I'd add a specific example to the commit message:

I certainly don't mind. I was frustrated after some attempts to find
bugs in my code using it before looking into qemu so I forgot to add
the info. (Also it would take me some time to figure out that I have the
backing format specified in the overlay image).

> 
> $ qemu-img create -f raw /tmp/backing.raw 64M
> $ qemu-img create -f qcow2 -F raw -b /tmp/backing.raw /tmp/test.qcow2
> $ qemu-system-x86_64 \
>   -blockdev driver=file,filename=/tmp/backing.raw,node-name=backing \
>   -blockdev driver=qcow2,file.driver=file,file.filename=/tmp/test.qcow2,node-name=root,backing=backing
> qemu-system-x86_64: -blockdev driver=qcow2,file.driver=file,file.filename=/tmp/test.qcow2,node-name=root,backing=backing: Could not open backing file: Cannot reference an existing block device with additional options or a new filename
> 
> > diff --git a/block.c b/block.c
> > index 46eb1728da..684cb018da 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2245,7 +2245,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
> >          goto free_exit;
> >      }
> > 
> > -    if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
> > +    if (!reference &&
> > +        bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
> >          qdict_put_str(options, "driver", bs->backing_format);
> >      }
> 
> Looks good to me.

Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 2/1] qemu-iotests: Test backing_fmt with backing node reference

[Kevin Wolf]

This changes test case 191 to include a backing image that has
backing_fmt set in the image file, but is referenced by node name in the
qemu command line.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/191     | 5 +++--
 tests/qemu-iotests/191.out | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
index ac2b88fd78..00202efd47 100755
--- a/tests/qemu-iotests/191
+++ b/tests/qemu-iotests/191
@@ -56,9 +56,10 @@ echo === Preparing and starting VM ===
 echo
 
 TEST_IMG="${TEST_IMG}.base" _make_test_img $size
-TEST_IMG="${TEST_IMG}.mid" _make_test_img -b "${TEST_IMG}.base"
+IMGOPTS=$(_optstr_add "$IMGOPTS" "backing_fmt=$IMGFMT") \
+    TEST_IMG="${TEST_IMG}.mid" _make_test_img -b "${TEST_IMG}.base" $size
 _make_test_img -b "${TEST_IMG}.mid"
-TEST_IMG="${TEST_IMG}.ovl2" _make_test_img -b "${TEST_IMG}.mid"
+TEST_IMG="${TEST_IMG}.ovl2" _make_test_img -b "${TEST_IMG}.mid" $size
 
 $QEMU_IO -c 'write -P 0x55 1M 64k' "${TEST_IMG}.mid" | _filter_qemu_io
 
diff --git a/tests/qemu-iotests/191.out b/tests/qemu-iotests/191.out
index 7bfcd2d5d8..6157fa520f 100644
--- a/tests/qemu-iotests/191.out
+++ b/tests/qemu-iotests/191.out
@@ -3,7 +3,7 @@ QA output created by 191
 === Preparing and starting VM ===
 
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
-Formatting 'TEST_DIR/t.IMGFMT.mid', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT.mid', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.mid
 Formatting 'TEST_DIR/t.IMGFMT.ovl2', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.mid
 wrote 65536/65536 bytes at offset 1048576
-- 
2.13.6

Re: [Qemu-devel] [PATCH 2/1] qemu-iotests: Test backing_fmt with backing node reference

[Kevin Wolf]

Am 17.10.2017 um 17:11 hat Kevin Wolf geschrieben:
> This changes test case 191 to include a backing image that has
> backing_fmt set in the image file, but is referenced by node name in the
> qemu command line.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/191     | 5 +++--
>  tests/qemu-iotests/191.out | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
> index ac2b88fd78..00202efd47 100755
> --- a/tests/qemu-iotests/191
> +++ b/tests/qemu-iotests/191
> @@ -56,9 +56,10 @@ echo === Preparing and starting VM ===
>  echo
>  
>  TEST_IMG="${TEST_IMG}.base" _make_test_img $size
> -TEST_IMG="${TEST_IMG}.mid" _make_test_img -b "${TEST_IMG}.base"
> +IMGOPTS=$(_optstr_add "$IMGOPTS" "backing_fmt=$IMGFMT") \
> +    TEST_IMG="${TEST_IMG}.mid" _make_test_img -b "${TEST_IMG}.base" $size
>  _make_test_img -b "${TEST_IMG}.mid"
> -TEST_IMG="${TEST_IMG}.ovl2" _make_test_img -b "${TEST_IMG}.mid"
> +TEST_IMG="${TEST_IMG}.ovl2" _make_test_img -b "${TEST_IMG}.mid" $size

Please pretend you don't see the addition of " $size" here, it's
unnecessary and I'll remove it again.

Kevin

Re: [Qemu-devel] [PATCH 2/1] qemu-iotests: Test backing_fmt with backing node reference

[Eric Blake]

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

On 10/17/2017 10:16 AM, Kevin Wolf wrote:
> Am 17.10.2017 um 17:11 hat Kevin Wolf geschrieben:
>> This changes test case 191 to include a backing image that has
>> backing_fmt set in the image file, but is referenced by node name in the
>> qemu command line.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  tests/qemu-iotests/191     | 5 +++--
>>  tests/qemu-iotests/191.out | 2 +-
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
>> index ac2b88fd78..00202efd47 100755
>> --- a/tests/qemu-iotests/191
>> +++ b/tests/qemu-iotests/191
>> @@ -56,9 +56,10 @@ echo === Preparing and starting VM ===
>>  echo
>>  
>>  TEST_IMG="${TEST_IMG}.base" _make_test_img $size
>> -TEST_IMG="${TEST_IMG}.mid" _make_test_img -b "${TEST_IMG}.base"
>> +IMGOPTS=$(_optstr_add "$IMGOPTS" "backing_fmt=$IMGFMT") \
>> +    TEST_IMG="${TEST_IMG}.mid" _make_test_img -b "${TEST_IMG}.base" $size
>>  _make_test_img -b "${TEST_IMG}.mid"
>> -TEST_IMG="${TEST_IMG}.ovl2" _make_test_img -b "${TEST_IMG}.mid"
>> +TEST_IMG="${TEST_IMG}.ovl2" _make_test_img -b "${TEST_IMG}.mid" $size
> 
> Please pretend you don't see the addition of " $size" here, it's
> unnecessary and I'll remove it again.

As tweaked,
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] block: don't add 'driver' to options when refering to backing via node name

[Eric Blake]

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

On 10/12/2017 09:14 AM, Peter Krempa wrote:
> When refering to a backing file of an image via node name

s/refering/referring/ (here and in the subject)

> bdrv_open_backing_file would add the 'driver' option to the option list
> filling it with the backing format driver. This breaks construction of
> the backing chain via -blockdev, as bdrv_open_inherit reports an error
> if both 'reference' and 'options' are provided.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  block.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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