Re: [Qemu-block] [PATCH v7 for-2.12 05/25] block: Respect backing bs in bdrv_refresh_filename
On Mon 20 Nov 2017 09:09:44 PM CET, Max Reitz wrote:
> 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.
>
> iotests 051 and 191 contain test cases for overwriting the backing file,
> and so their output changes with this patch applied (which I consider a
> good thing). Note that in the case of 191, the implicitly opened
> (non-overridden) base file is included in the json:{} filename as well
> -- this will be remedied by a later patch.
>
> Signed-off-by: Max Reitz
Reviewed-by: Alberto Garcia
Berto
Re: [Qemu-block] [PATCH v7 for-2.12 05/25] block: Respect backing bs in bdrv_refresh_filename
On 2017-12-05 14:27, Alberto Garcia wrote:
> On Mon 20 Nov 2017 09:09:44 PM CET, Max Reitz wrote:
>> @@ -5016,6 +5016,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 */
>> @@ -5027,11 +5028,20 @@ 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))
>> +{
>
> Does this mean that both file. and backing. have to be overriden?
The bs->file->bs->full_open_options is not a check whether anything has
been overridden but just whether we have a QDict of runtime options for
file (and we should always have one). After this series, that check
therefore disappears.
> Shouldn't that be a || instead of a && ??
The block under this if condition automatically constructs
bs->full_open_options -- but it can only do so if all of the relevant
children's options are known (because their options need to be put into
bs->full_open_options).
Previously, that was only the file child. This patch adds support for
the backing child: Whenever there is a backing child node, we need to
include its options under the "backing" key.
So that's where the condition comes from: For all children that this
node has, we need their full_open_options or we cannot construct this
node's full_open_options.
We do have bs->file (because that is the condition for the block this
condition is in), so we need bs->file->bs->full_open_options. And if we
have bs->backing, we also need bs->backing->bs->full_open_options.
>> qdict_put_str(opts, "driver", drv->format_name);
>> QINCREF(bs->file->bs->full_open_options);
>> qdict_put(opts, "file", bs->file->bs->full_open_options);
>>
>> +if (bs->backing) {
>> +QINCREF(bs->backing->bs->full_open_options);
>> +qdict_put(opts, "backing",
>> bs->backing->bs->full_open_options);
>> +} else if (bs->backing_overridden && !bs->backing) {
>> +qdict_put(opts, "backing", qstring_new());
>> +}
>
> You don't need the !bs->backing in the second if, it's implied from the
> previous one.
Maybe that was intentional (to be more explicit, and because after this
series only this branch remains), maybe it wasn't. I don't know
anymore, so I'll just drop it (even though I'll have to re-add it later,
because as I said, only this else if branch will remain).
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v7 for-2.12 05/25] block: Respect backing bs in bdrv_refresh_filename
On Mon 20 Nov 2017 09:09:44 PM CET, Max Reitz wrote:
> @@ -5016,6 +5016,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 */
> @@ -5027,11 +5028,20 @@ 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))
> +{
Does this mean that both file. and backing. have to be overriden?
Shouldn't that be a || instead of a && ??
> qdict_put_str(opts, "driver", drv->format_name);
> QINCREF(bs->file->bs->full_open_options);
> qdict_put(opts, "file", bs->file->bs->full_open_options);
>
> +if (bs->backing) {
> +QINCREF(bs->backing->bs->full_open_options);
> +qdict_put(opts, "backing",
> bs->backing->bs->full_open_options);
> +} else if (bs->backing_overridden && !bs->backing) {
> +qdict_put(opts, "backing", qstring_new());
> +}
You don't need the !bs->backing in the second if, it's implied from the
previous one.
Berto
[Qemu-block] [PATCH v7 for-2.12 05/25] block: Respect backing bs in bdrv_refresh_filename
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.
iotests 051 and 191 contain test cases for overwriting the backing file,
and so their output changes with this patch applied (which I consider a
good thing). Note that in the case of 191, the implicitly opened
(non-overridden) base file is included in the json:{} filename as well
-- this will be remedied by a later patch.
Signed-off-by: Max Reitz
---
block.c | 12 +++-
tests/qemu-iotests/051.out| 8
tests/qemu-iotests/051.pc.out | 8
tests/qemu-iotests/191.out| 24
4 files changed, 31 insertions(+), 21 deletions(-)
diff --git a/block.c b/block.c
index eb67dfbcc0..2dc06cb9cb 100644
--- a/block.c
+++ b/block.c
@@ -5016,6 +5016,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 */
@@ -5027,11 +5028,20 @@ 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_str(opts, "driver", drv->format_name);
QINCREF(bs->file->bs->full_open_options);
qdict_put(opts, "file", bs->file->bs->full_open_options);
+if (bs->backing) {
+QINCREF(bs->backing->bs->full_open_options);
+qdict_put(opts, "backing", bs->backing->bs->full_open_options);
+} else if (bs->backing_overridden && !bs->backing) {
+qdict_put(opts, "backing", 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 e3c6eaba57..50d5cd07c8 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) info block
-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) info block
-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) info block
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backi
