[03/19] block: Respect backing bs in bdrv_refresh_filename

Message ID	1461706338-20219-4-git-send-email-mreitz@redhat.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org> From: Max Reitz <mreitz@redhat.com> To: qemu-block@nongnu.org Date: Tue, 26 Apr 2016 23:32:02 +0200 Message-Id: <1461706338-20219-4-git-send-email-mreitz@redhat.com> In-Reply-To: <1461706338-20219-1-git-send-email-mreitz@redhat.com> References: <1461706338-20219-1-git-send-email-mreitz@redhat.com> Subject: [Qemu-devel] [PATCH 03/19] block: Respect backing bs in bdrv_refresh_filename Precedence: list Cc: Kevin Wolf <kwolf@redhat.com>, Alberto Garcia <berto@igalia.com>, qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com> Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" <qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org>

Message ID

1461706338-20219-4-git-send-email-mreitz@redhat.com (mailing list archive)

State

New, archived

Headers

From: Max Reitz <mreitz@redhat.com>
To: qemu-block@nongnu.org
Date: Tue, 26 Apr 2016 23:32:02 +0200
Message-Id: <1461706338-20219-4-git-send-email-mreitz@redhat.com>
In-Reply-To: <1461706338-20219-1-git-send-email-mreitz@redhat.com>
References: <1461706338-20219-1-git-send-email-mreitz@redhat.com>
Subject: [Qemu-devel] [PATCH 03/19] block: Respect backing bs in
	bdrv_refresh_filename
Precedence: list
Cc: Kevin Wolf <kwolf@redhat.com>, Alberto Garcia <berto@igalia.com>,
	qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>
Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org
Sender: "Qemu-devel"
	<qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org>

Commit Message

Max Reitz April 26, 2016, 9:32 p.m. UTC

Basically, bdrv_refresh_filename() should respect all children of a
BlockDriverState. However, generally those children are driver-specific,
so this function cannot handle the general case. On the other hand,
there are only few drivers which use other children than @file and
@backing (that being vmdk, quorum, and blkverify).

Most block drivers only use @file and/or @backing (if they use any
children at all). Both can be implemented directly in
bdrv_refresh_filename.

The user overriding the file's filename is already handled, however, the
user overriding the backing file is not. If this is done, opening the
BDS with the plain filename of its file will not be correct, so we may
not set bs->exact_filename in that case.

iotest 051 contains test cases for overwriting the backing file, and so
its output changes with this patch applied (which I consider a good
thing).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                       | 14 +++++++++++++-
 tests/qemu-iotests/051.out    |  8 ++++----
 tests/qemu-iotests/051.pc.out |  8 ++++----
 3 files changed, 21 insertions(+), 9 deletions(-)

Comments

Kevin Wolf May 2, 2016, 3:36 p.m. UTC | #1

Am 26.04.2016 um 23:32 hat Max Reitz geschrieben:
> Basically, bdrv_refresh_filename() should respect all children of a
> BlockDriverState. However, generally those children are driver-specific,
> so this function cannot handle the general case. On the other hand,
> there are only few drivers which use other children than @file and
> @backing (that being vmdk, quorum, and blkverify).
> 
> Most block drivers only use @file and/or @backing (if they use any
> children at all). Both can be implemented directly in
> bdrv_refresh_filename.
> 
> The user overriding the file's filename is already handled, however, the
> user overriding the backing file is not. If this is done, opening the
> BDS with the plain filename of its file will not be correct, so we may
> not set bs->exact_filename in that case.
> 
> iotest 051 contains test cases for overwriting the backing file, and so
> its output changes with this patch applied (which I consider a good
> thing).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                       | 14 +++++++++++++-
>  tests/qemu-iotests/051.out    |  8 ++++----
>  tests/qemu-iotests/051.pc.out |  8 ++++----
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e178488..aff9ea3 100644
> --- a/block.c
> +++ b/block.c
> @@ -3925,6 +3925,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>  
>          opts = qdict_new();
>          has_open_options = append_open_options(opts, bs);
> +        has_open_options |= bs->backing_overridden;
>  
>          /* If no specific options have been given for this BDS, the filename of
>           * the underlying file should suffice for this one as well */
> @@ -3936,13 +3937,24 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>           * file BDS. The full options QDict of that file BDS should somehow
>           * contain a representation of the filename, therefore the following
>           * suffices without querying the (exact_)filename of this BDS. */
> -        if (bs->file->bs->full_open_options) {
> +        if (bs->file->bs->full_open_options &&
> +            (!bs->backing || bs->backing->bs->full_open_options))
> +        {
>              qdict_put_obj(opts, "driver",
>                            QOBJECT(qstring_from_str(drv->format_name)));
> +
>              QINCREF(bs->file->bs->full_open_options);
>              qdict_put_obj(opts, "file",
>                            QOBJECT(bs->file->bs->full_open_options));
>  
> +            if (bs->backing) {
> +                QINCREF(bs->backing->bs->full_open_options);
> +                qdict_put_obj(opts, "backing",
> +                              QOBJECT(bs->backing->bs->full_open_options));
> +            } else if (bs->backing_overridden && !bs->backing) {
> +                qdict_put_obj(opts, "backing", QOBJECT(qstring_new()));
> +            }

I see that the surrounding code does the same, but what's wrong with
qdict_put()?

Kevin

Max Reitz May 3, 2016, 10:50 a.m. UTC | #2

On 02.05.2016 17:36, Kevin Wolf wrote:
> Am 26.04.2016 um 23:32 hat Max Reitz geschrieben:
>> Basically, bdrv_refresh_filename() should respect all children of a
>> BlockDriverState. However, generally those children are driver-specific,
>> so this function cannot handle the general case. On the other hand,
>> there are only few drivers which use other children than @file and
>> @backing (that being vmdk, quorum, and blkverify).
>>
>> Most block drivers only use @file and/or @backing (if they use any
>> children at all). Both can be implemented directly in
>> bdrv_refresh_filename.
>>
>> The user overriding the file's filename is already handled, however, the
>> user overriding the backing file is not. If this is done, opening the
>> BDS with the plain filename of its file will not be correct, so we may
>> not set bs->exact_filename in that case.
>>
>> iotest 051 contains test cases for overwriting the backing file, and so
>> its output changes with this patch applied (which I consider a good
>> thing).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c                       | 14 +++++++++++++-
>>  tests/qemu-iotests/051.out    |  8 ++++----
>>  tests/qemu-iotests/051.pc.out |  8 ++++----
>>  3 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e178488..aff9ea3 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3925,6 +3925,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>>  
>>          opts = qdict_new();
>>          has_open_options = append_open_options(opts, bs);
>> +        has_open_options |= bs->backing_overridden;
>>  
>>          /* If no specific options have been given for this BDS, the filename of
>>           * the underlying file should suffice for this one as well */
>> @@ -3936,13 +3937,24 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>>           * file BDS. The full options QDict of that file BDS should somehow
>>           * contain a representation of the filename, therefore the following
>>           * suffices without querying the (exact_)filename of this BDS. */
>> -        if (bs->file->bs->full_open_options) {
>> +        if (bs->file->bs->full_open_options &&
>> +            (!bs->backing || bs->backing->bs->full_open_options))
>> +        {
>>              qdict_put_obj(opts, "driver",
>>                            QOBJECT(qstring_from_str(drv->format_name)));
>> +
>>              QINCREF(bs->file->bs->full_open_options);
>>              qdict_put_obj(opts, "file",
>>                            QOBJECT(bs->file->bs->full_open_options));
>>  
>> +            if (bs->backing) {
>> +                QINCREF(bs->backing->bs->full_open_options);
>> +                qdict_put_obj(opts, "backing",
>> +                              QOBJECT(bs->backing->bs->full_open_options));
>> +            } else if (bs->backing_overridden && !bs->backing) {
>> +                qdict_put_obj(opts, "backing", QOBJECT(qstring_new()));
>> +            }
> 
> I see that the surrounding code does the same, but what's wrong with
> qdict_put()?

That it's a macro. clang_complete does not tell me about macros.

Will fix and try to keep in mind in the future, thanks.

Max

diff --git a/block.c b/block.c
index e178488..aff9ea3 100644
--- a/block.c
+++ b/block.c
@@ -3925,6 +3925,7 @@  void bdrv_refresh_filename(BlockDriverState *bs)
 
         opts = qdict_new();
         has_open_options = append_open_options(opts, bs);
+        has_open_options |= bs->backing_overridden;
 
         /* If no specific options have been given for this BDS, the filename of
          * the underlying file should suffice for this one as well */
@@ -3936,13 +3937,24 @@  void bdrv_refresh_filename(BlockDriverState *bs)
          * file BDS. The full options QDict of that file BDS should somehow
          * contain a representation of the filename, therefore the following
          * suffices without querying the (exact_)filename of this BDS. */
-        if (bs->file->bs->full_open_options) {
+        if (bs->file->bs->full_open_options &&
+            (!bs->backing || bs->backing->bs->full_open_options))
+        {
             qdict_put_obj(opts, "driver",
                           QOBJECT(qstring_from_str(drv->format_name)));
+
             QINCREF(bs->file->bs->full_open_options);
             qdict_put_obj(opts, "file",
                           QOBJECT(bs->file->bs->full_open_options));
 
+            if (bs->backing) {
+                QINCREF(bs->backing->bs->full_open_options);
+                qdict_put_obj(opts, "backing",
+                              QOBJECT(bs->backing->bs->full_open_options));
+            } else if (bs->backing_overridden && !bs->backing) {
+                qdict_put_obj(opts, "backing", QOBJECT(qstring_new()));
+            }
+
             bs->full_open_options = opts;
         } else {
             QDECREF(opts);
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 408d613..d936798 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -59,7 +59,7 @@  QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive file=TEST_DIR/t.qcow2,driver=qcow2,backing.file.filename=TEST_DIR/t.qcow2.orig,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i[K[Din[K[D[Dinf[K[D[D[Dinfo[K[D[D[D[Dinfo [K[D[D[D[D[Dinfo b[K[D[D[D[D[D[Dinfo bl[K[D[D[D[D[D[D[Dinfo blo[K[D[D[D[D[D[D[D[Dinfo bloc[K[D[D[D[D[D[D[D[D[Dinfo block[K
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.orig"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
     Removable device: not locked, tray closed
     Cache mode:       writeback
     Backing file:     TEST_DIR/t.qcow2.orig (chain depth: 1)
@@ -148,7 +148,7 @@  QEMU_PROG: -drive driver=null-co,cache=invalid_value: invalid cache option
 Testing: -drive file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i[K[Din[K[D[Dinf[K[D[D[Dinfo[K[D[D[D[Dinfo [K[D[D[D[D[Dinfo b[K[D[D[D[D[D[Dinfo bl[K[D[D[D[D[D[D[Dinfo blo[K[D[D[D[D[D[D[D[Dinfo bloc[K[D[D[D[D[D[D[D[D[Dinfo block[K
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.base"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
     Removable device: not locked, tray closed
     Cache mode:       writeback
     Backing file:     TEST_DIR/t.qcow2.base (chain depth: 1)
@@ -168,7 +168,7 @@  backing-file: TEST_DIR/t.qcow2.base (file, read-only)
 Testing: -drive file=TEST_DIR/t.qcow2,cache=writethrough,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i[K[Din[K[D[Dinf[K[D[D[Dinfo[K[D[D[D[Dinfo [K[D[D[D[D[Dinfo b[K[D[D[D[D[D[Dinfo bl[K[D[D[D[D[D[D[Dinfo blo[K[D[D[D[D[D[D[D[Dinfo bloc[K[D[D[D[D[D[D[D[D[Dinfo block[K
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.base"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
     Removable device: not locked, tray closed
     Cache mode:       writethrough
     Backing file:     TEST_DIR/t.qcow2.base (chain depth: 1)
@@ -188,7 +188,7 @@  backing-file: TEST_DIR/t.qcow2.base (file, read-only)
 Testing: -drive file=TEST_DIR/t.qcow2,cache=unsafe,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i[K[Din[K[D[Dinf[K[D[D[Dinfo[K[D[D[D[Dinfo [K[D[D[D[D[Dinfo b[K[D[D[D[D[D[Dinfo bl[K[D[D[D[D[D[D[Dinfo blo[K[D[D[D[D[D[D[D[Dinfo bloc[K[D[D[D[D[D[D[D[D[Dinfo block[K
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.base"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
     Removable device: not locked, tray closed
     Cache mode:       writeback, ignore flushes
     Backing file:     TEST_DIR/t.qcow2.base (chain depth: 1)
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index ec6d222..cb899ce 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -59,7 +59,7 @@  QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive file=TEST_DIR/t.qcow2,driver=qcow2,backing.file.filename=TEST_DIR/t.qcow2.orig,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i[K[Din[K[D[Dinf[K[D[D[Dinfo[K[D[D[D[Dinfo [K[D[D[D[D[Dinfo b[K[D[D[D[D[D[Dinfo bl[K[D[D[D[D[D[D[Dinfo blo[K[D[D[D[D[D[D[D[Dinfo bloc[K[D[D[D[D[D[D[D[D[Dinfo block[K
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.orig"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
     Removable device: not locked, tray closed
     Cache mode:       writeback
     Backing file:     TEST_DIR/t.qcow2.orig (chain depth: 1)
@@ -242,7 +242,7 @@  QEMU_PROG: -drive driver=null-co,cache=invalid_value: invalid cache option
 Testing: -drive file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i[K[Din[K[D[Dinf[K[D[D[Dinfo[K[D[D[D[Dinfo [K[D[D[D[D[Dinfo b[K[D[D[D[D[D[Dinfo bl[K[D[D[D[D[D[D[Dinfo blo[K[D[D[D[D[D[D[D[Dinfo bloc[K[D[D[D[D[D[D[D[D[Dinfo block[K
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.base"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
     Removable device: not locked, tray closed
     Cache mode:       writeback
     Backing file:     TEST_DIR/t.qcow2.base (chain depth: 1)
@@ -262,7 +262,7 @@  backing-file: TEST_DIR/t.qcow2.base (file, read-only)
 Testing: -drive file=TEST_DIR/t.qcow2,cache=writethrough,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i[K[Din[K[D[Dinf[K[D[D[Dinfo[K[D[D[D[Dinfo [K[D[D[D[D[Dinfo b[K[D[D[D[D[D[Dinfo bl[K[D[D[D[D[D[D[Dinfo blo[K[D[D[D[D[D[D[D[Dinfo bloc[K[D[D[D[D[D[D[D[D[Dinfo block[K
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.base"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
     Removable device: not locked, tray closed
     Cache mode:       writethrough
     Backing file:     TEST_DIR/t.qcow2.base (chain depth: 1)
@@ -282,7 +282,7 @@  backing-file: TEST_DIR/t.qcow2.base (file, read-only)
 Testing: -drive file=TEST_DIR/t.qcow2,cache=unsafe,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i[K[Din[K[D[Dinf[K[D[D[Dinfo[K[D[D[D[Dinfo [K[D[D[D[D[Dinfo b[K[D[D[D[D[D[Dinfo bl[K[D[D[D[D[D[D[Dinfo blo[K[D[D[D[D[D[D[D[Dinfo bloc[K[D[D[D[D[D[D[D[D[Dinfo block[K
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.base"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
     Removable device: not locked, tray closed
     Cache mode:       writeback, ignore flushes
     Backing file:     TEST_DIR/t.qcow2.base (chain depth: 1)

[03/19] block: Respect backing bs in bdrv_refresh_filename

Commit Message

Comments

Patch