* [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues
@ 2018-02-05 15:18 Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 01/26] block/mirror: Small absolute-paths simplification Max Reitz
` (25 more replies)
0 siblings, 26 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
I'm sparing myself writing this cover letter again, and I'll just give
you a link to one of the previous versions:
http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html
The main difference is that I dropped patch 16 which added a QAPI
@base-directory option for any node that could be used to override the
base directory to be used for resolving relative filenames. (Because
Berto and Kevin convinced me it's not that useful now -- and the patch
is pretty stand-alone, so we can always add it later if needed.)
In turn, I had to add patch 4 because test 191 was broken by this
series.
v8: Rebased, and:
- Patch 3: Actually just contextual difference
- Patch 4: Rebase conflict: 191 now filters its output a bit more, so
there are differences in the reference output ("format-specific" is
missing and %s/qcow2/IMGFMT/g)
- Patch 5:
- Drop superfluous "&& !bs->backing" in one else branch following a
"bs->backing" if [Berto]
- Same 191.out conflicts as in patch 4
- Patch 18: Indentation conflicts in block/sheepdog.c
- Patch 20:
- Dropped now-missing "base-directory" option from the global_options
array [Berto]
- Same old 191.out conflicts
- Patch 21:
- Trivial rebase conflict because patch 5 has changed
- For blkdebug, both "image" and "x-image" may contain child-related
information, so we need to consider both in its
.bdrv_refresh_filename implementation [Berto]
- Patch 24: Added [Berto]
- Patch 25: In curl, don't "silently" encode the default values in
its .bdrv_refresh_filename(), but use explicit macros
(except for "readahead" and "timeout", which we can ignore,
and for everything that is a pointer, because the default
value is rather obviously NULL; so that actually just
leaves us with "sslverify") [Berto]
git-backport-diff against v7:
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/26:[----] [--] 'block/mirror: Small absolute-paths simplification'
002/26:[----] [--] 'block: Use children list in bdrv_refresh_filename'
003/26:[0002] [FC] 'block: Add BDS.backing_overridden'
004/26:[0074] [FC] 'iotests: Drop explicit base blockdev in 191'
005/26:[0050] [FC] 'block: Respect backing bs in bdrv_refresh_filename'
006/26:[----] [--] 'block: Make path_combine() return the path'
007/26:[----] [--] 'block: bdrv_get_full_backing_filename_from_...'s ret. val.'
008/26:[----] [--] 'block: bdrv_get_full_backing_filename's ret. val.'
009/26:[----] [--] 'block: Add bdrv_make_absolute_filename()'
010/26:[----] [--] 'block: Fix bdrv_find_backing_image()'
011/26:[----] [--] 'block: Add bdrv_dirname()'
012/26:[----] [--] 'blkverify: Make bdrv_dirname() return NULL'
013/26:[----] [--] 'quorum: Make bdrv_dirname() return NULL'
014/26:[----] [-C] 'block/nbd: Make bdrv_dirname() return NULL'
015/26:[----] [--] 'block/nfs: Implement bdrv_dirname()'
016/26:[----] [--] 'block: Use bdrv_dirname() for relative filenames'
017/26:[----] [--] 'iotests: Add quorum case to test 110'
018/26:[0006] [FC] 'block: Add sgfnt_runtime_opts to BlockDriver'
019/26:[----] [--] 'block: Add BlockDriver.bdrv_gather_child_options'
020/26:[0050] [FC] 'block: Generically refresh runtime options'
021/26:[0005] [FC] 'block: Purify .bdrv_refresh_filename()'
022/26:[----] [--] 'block: Do not copy exact_filename from format file'
023/26:[----] [--] 'block: Fix FIXME from "Add BDS.backing_overridden"'
024/26:[down] 'block/curl: Harmonize option defaults'
025/26:[0007] [FC] 'block/curl: Implement bdrv_refresh_filename()'
026/26:[----] [--] 'block/null: Generate filename even with latency-ns'
Max Reitz (26):
block/mirror: Small absolute-paths simplification
block: Use children list in bdrv_refresh_filename
block: Add BDS.backing_overridden
iotests: Drop explicit base blockdev in 191
block: Respect backing bs in bdrv_refresh_filename
block: Make path_combine() return the path
block: bdrv_get_full_backing_filename_from_...'s ret. val.
block: bdrv_get_full_backing_filename's ret. val.
block: Add bdrv_make_absolute_filename()
block: Fix bdrv_find_backing_image()
block: Add bdrv_dirname()
blkverify: Make bdrv_dirname() return NULL
quorum: Make bdrv_dirname() return NULL
block/nbd: Make bdrv_dirname() return NULL
block/nfs: Implement bdrv_dirname()
block: Use bdrv_dirname() for relative filenames
iotests: Add quorum case to test 110
block: Add sgfnt_runtime_opts to BlockDriver
block: Add BlockDriver.bdrv_gather_child_options
block: Generically refresh runtime options
block: Purify .bdrv_refresh_filename()
block: Do not copy exact_filename from format file
block: Fix FIXME from "Add BDS.backing_overridden"
block/curl: Harmonize option defaults
block/curl: Implement bdrv_refresh_filename()
block/null: Generate filename even with latency-ns
include/block/block.h | 15 +-
include/block/block_int.h | 38 +++-
block.c | 507 ++++++++++++++++++++++++++++--------------
block/blkdebug.c | 72 +++---
block/blkverify.c | 29 +--
block/commit.c | 3 +-
block/crypto.c | 8 +
block/curl.c | 55 ++++-
block/gluster.c | 19 ++
block/iscsi.c | 18 ++
block/mirror.c | 19 +-
block/nbd.c | 50 +++--
block/nfs.c | 53 ++---
block/null.c | 33 ++-
block/qapi.c | 12 +-
block/qcow.c | 7 +
block/qcow2.c | 7 +
block/quorum.c | 69 ++++--
block/raw-format.c | 10 +-
block/rbd.c | 14 ++
block/replication.c | 8 +
block/sheepdog.c | 12 +
block/ssh.c | 12 +
block/throttle.c | 7 +
block/vmdk.c | 25 ++-
block/vpc.c | 7 +
block/vvfat.c | 12 +
block/vxhs.c | 11 +
blockdev.c | 16 ++
tests/qemu-iotests/051.out | 8 +-
tests/qemu-iotests/051.pc.out | 8 +-
tests/qemu-iotests/110 | 29 ++-
tests/qemu-iotests/110.out | 9 +-
tests/qemu-iotests/191 | 3 +-
tests/qemu-iotests/191.out | 76 +++----
35 files changed, 884 insertions(+), 397 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 01/26] block/mirror: Small absolute-paths simplification
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-22 12:27 ` Kevin Wolf
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 02/26] block: Use children list in bdrv_refresh_filename Max Reitz
` (24 subsequent siblings)
25 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
When invoking drive-mirror in absolute-paths mode, the target's backing
BDS is assigned to it in mirror_exit(). The current logic only does so
if the target does not have that backing BDS already; but it actually
cannot have a backing BDS at all (the BDS is opened with O_NO_BACKING in
qmp_drive_mirror()), so just assert that and assign the new backing BDS
unconditionally.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/mirror.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index c9badc1203..45e35c909f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -523,12 +523,12 @@ static void mirror_exit(BlockJob *job, void *opaque)
&error_abort);
if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
BlockDriverState *backing = s->is_none_mode ? src : s->base;
- if (backing_bs(target_bs) != backing) {
- bdrv_set_backing_hd(target_bs, backing, &local_err);
- if (local_err) {
- error_report_err(local_err);
- data->ret = -EPERM;
- }
+
+ assert(!target_bs->backing);
+ bdrv_set_backing_hd(target_bs, backing, &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ data->ret = -EPERM;
}
}
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 02/26] block: Use children list in bdrv_refresh_filename
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 01/26] block/mirror: Small absolute-paths simplification Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-22 12:30 ` Kevin Wolf
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden Max Reitz
` (23 subsequent siblings)
25 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
bdrv_refresh_filename() should invoke itself recursively on all
children, not just on file.
With that change, we can remove the manual invocations in blkverify,
quorum, commit, and mirror.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 9 +++++----
block/blkverify.c | 3 ---
block/commit.c | 1 -
block/mirror.c | 1 -
block/quorum.c | 1 -
5 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/block.c b/block.c
index a8da4f2b25..b84aba2d95 100644
--- a/block.c
+++ b/block.c
@@ -5020,16 +5020,17 @@ static bool append_open_options(QDict *d, BlockDriverState *bs)
void bdrv_refresh_filename(BlockDriverState *bs)
{
BlockDriver *drv = bs->drv;
+ BdrvChild *child;
QDict *opts;
if (!drv) {
return;
}
- /* This BDS's file name will most probably depend on its file's name, so
- * refresh that first */
- if (bs->file) {
- bdrv_refresh_filename(bs->file->bs);
+ /* This BDS's file name may depend on any of its children's file names, so
+ * refresh those first */
+ QLIST_FOREACH(child, &bs->children, next) {
+ bdrv_refresh_filename(child->bs);
}
if (drv->bdrv_refresh_filename) {
diff --git a/block/blkverify.c b/block/blkverify.c
index 06369f9eac..b2ed8cd70d 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -281,9 +281,6 @@ static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options)
{
BDRVBlkverifyState *s = bs->opaque;
- /* bs->file->bs has already been refreshed */
- bdrv_refresh_filename(s->test_file->bs);
-
if (bs->file->bs->full_open_options
&& s->test_file->bs->full_open_options)
{
diff --git a/block/commit.c b/block/commit.c
index bb6c904704..1fa6c9c0e0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -241,7 +241,6 @@ static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs,
static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts)
{
- bdrv_refresh_filename(bs->backing->bs);
pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
bs->backing->bs->filename);
}
diff --git a/block/mirror.c b/block/mirror.c
index 45e35c909f..5a627ba589 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1061,7 +1061,6 @@ static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
* bdrv_set_backing_hd */
return;
}
- bdrv_refresh_filename(bs->backing->bs);
pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
bs->backing->bs->filename);
}
diff --git a/block/quorum.c b/block/quorum.c
index 272f9a5b77..2f1a628449 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1074,7 +1074,6 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
int i;
for (i = 0; i < s->num_children; i++) {
- bdrv_refresh_filename(s->children[i]->bs);
if (!s->children[i]->bs->full_open_options) {
return;
}
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 01/26] block/mirror: Small absolute-paths simplification Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 02/26] block: Use children list in bdrv_refresh_filename Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-22 13:39 ` Kevin Wolf
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 04/26] iotests: Drop explicit base blockdev in 191 Max Reitz
` (22 subsequent siblings)
25 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
If the backing file is overridden, this most probably does change the
guest-visible data of a BDS. Therefore, we will need to consider this in
bdrv_refresh_filename().
Adding a new field to the BDS is not nice, but it is very simple and
exactly keeps track of whether the backing file has been overridden.
This commit adds a FIXME which will be remedied by a follow-up commit.
Until then, the respective piece of code will not result in any behavior
that is worse than what we currently have.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block_int.h | 1 +
block.c | 13 +++++++++++++
block/mirror.c | 4 ++++
blockdev.c | 16 ++++++++++++++++
4 files changed, 34 insertions(+)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 29cafa4236..b5c124fcbd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -619,6 +619,7 @@ struct BlockDriverState {
char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
this file image */
char backing_format[16]; /* if non-zero and backing_file exists */
+ bool backing_overridden; /* backing file has been specified by the user */
QDict *full_open_options;
char exact_filename[PATH_MAX];
diff --git a/block.c b/block.c
index b84aba2d95..66e37219b9 100644
--- a/block.c
+++ b/block.c
@@ -2268,6 +2268,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
reference = qdict_get_try_str(parent_options, bdref_key);
if (reference || qdict_haskey(options, "file.filename")) {
backing_filename[0] = '\0';
+
+ /* FIXME: Should also be set to true if @options contains other runtime
+ * options which control the data that is read from the backing
+ * BDS */
+ bs->backing_overridden = true;
} else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
QDECREF(options);
goto free_exit;
@@ -2469,6 +2474,9 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
goto out;
}
+ bs_snapshot->backing_overridden = true;
+ bdrv_refresh_filename(bs_snapshot);
+
out:
QDECREF(snapshot_options);
g_free(tmp_filename);
@@ -2599,6 +2607,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
backing = qdict_get_try_str(options, "backing");
if (backing && *backing == '\0') {
flags |= BDRV_O_NO_BACKING;
+ bs->backing_overridden = true;
qdict_del(options, "backing");
}
@@ -5031,6 +5040,10 @@ void bdrv_refresh_filename(BlockDriverState *bs)
* refresh those first */
QLIST_FOREACH(child, &bs->children, next) {
bdrv_refresh_filename(child->bs);
+
+ if (child->role == &child_backing && child->bs->backing_overridden) {
+ bs->backing_overridden = true;
+ }
}
if (drv->bdrv_refresh_filename) {
diff --git a/block/mirror.c b/block/mirror.c
index 5a627ba589..aa68531352 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -530,6 +530,10 @@ static void mirror_exit(BlockJob *job, void *opaque)
error_report_err(local_err);
data->ret = -EPERM;
}
+
+ /* The target image's file already has been created with the backing
+ * file we just set, so there is no need to set backing_overridden or
+ * call bdrv_refresh_filename(). */
}
if (s->to_replace) {
diff --git a/blockdev.c b/blockdev.c
index 8e977eef11..bbe264c3c0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1795,6 +1795,8 @@ static void external_snapshot_commit(BlkActionState *common)
{
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
+ TransactionAction *action = common->action;
+ bool image_was_existing = false;
AioContext *aio_context;
aio_context = bdrv_get_aio_context(state->old_bs);
@@ -1808,6 +1810,20 @@ static void external_snapshot_commit(BlkActionState *common)
NULL);
}
+ if (action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) {
+ BlockdevSnapshotSync *s = action->u.blockdev_snapshot_sync.data;
+ if (s->has_mode && s->mode == NEW_IMAGE_MODE_EXISTING) {
+ image_was_existing = true;
+ }
+ } else {
+ image_was_existing = true;
+ }
+
+ if (image_was_existing) {
+ state->new_bs->backing_overridden = true;
+ bdrv_refresh_filename(state->new_bs);
+ }
+
aio_context_release(aio_context);
}
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 04/26] iotests: Drop explicit base blockdev in 191
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (2 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-06 13:56 ` Alberto Garcia
2018-02-22 14:34 ` Kevin Wolf
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 05/26] block: Respect backing bs in bdrv_refresh_filename Max Reitz
` (21 subsequent siblings)
25 siblings, 2 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Overriding the backing image should result in a json:{} pseudo-filename.
Then, you can no longer use the commit block job with filename
parameters. Therefore, do not explicitly add the base and override the
middle image in iotest 191, since we do not need to anyway. This will
allow us to continue to use the middle image's filename to identify it.
In the long run, we want block-commit to accept node names for base and
top (just like block-stream does).
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/191 | 3 +--
tests/qemu-iotests/191.out | 52 +++++++++++++++++++++++-----------------------
2 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
index dfad6555e4..af93e932bf 100755
--- a/tests/qemu-iotests/191
+++ b/tests/qemu-iotests/191
@@ -66,8 +66,7 @@ qemu_comm_method="qmp"
qmp_pretty="y"
_launch_qemu \
- -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.base,node-name=base" \
- -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.mid,node-name=mid,backing=base" \
+ -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.mid,node-name=mid" \
-blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG},node-name=top,backing=mid" \
-blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.ovl2,node-name=top2,backing=mid"
h=$QEMU_HANDLE
diff --git a/tests/qemu-iotests/191.out b/tests/qemu-iotests/191.out
index 190c5f049a..575cd3ea2d 100644
--- a/tests/qemu-iotests/191.out
+++ b/tests/qemu-iotests/191.out
@@ -184,33 +184,21 @@ wrote 65536/65536 bytes at offset 1048576
"iops_rd": 0,
"detect_zeroes": "off",
"image": {
- "backing-image": {
- "virtual-size": 67108864,
- "filename": "TEST_DIR/t.IMGFMT.base",
- "cluster-size": 65536,
- "format": "IMGFMT",
- "actual-size": SIZE,
- "dirty-flag": false
- },
- "backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "TEST_DIR/t.IMGFMT.mid",
+ "filename": "TEST_DIR/t.IMGFMT.base",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
- "full-backing-filename": "TEST_DIR/t.IMGFMT.base",
- "backing-filename": "TEST_DIR/t.IMGFMT.base",
"dirty-flag": false
},
"iops_wr": 0,
- "ro": false,
- "node-name": "mid",
- "backing_file_depth": 1,
+ "ro": true,
+ "node-name": "NODE_NAME",
+ "backing_file_depth": 0,
"drv": "IMGFMT",
"iops": 0,
"bps_wr": 0,
"write_threshold": 0,
- "backing_file": "TEST_DIR/t.IMGFMT.base",
"encrypted": false,
"bps": 0,
"bps_rd": 0,
@@ -219,7 +207,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "TEST_DIR/t.IMGFMT.mid",
+ "file": "TEST_DIR/t.IMGFMT.base",
"encryption_key_missing": false
},
{
@@ -227,13 +215,13 @@ wrote 65536/65536 bytes at offset 1048576
"detect_zeroes": "off",
"image": {
"virtual-size": 393216,
- "filename": "TEST_DIR/t.IMGFMT.mid",
+ "filename": "TEST_DIR/t.IMGFMT.base",
"format": "file",
"actual-size": SIZE,
"dirty-flag": false
},
"iops_wr": 0,
- "ro": false,
+ "ro": true,
"node-name": "NODE_NAME",
"backing_file_depth": 0,
"drv": "file",
@@ -248,28 +236,40 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "TEST_DIR/t.IMGFMT.mid",
+ "file": "TEST_DIR/t.IMGFMT.base",
"encryption_key_missing": false
},
{
"iops_rd": 0,
"detect_zeroes": "off",
"image": {
+ "backing-image": {
+ "virtual-size": 67108864,
+ "filename": "TEST_DIR/t.IMGFMT.base",
+ "cluster-size": 65536,
+ "format": "IMGFMT",
+ "actual-size": SIZE,
+ "dirty-flag": false
+ },
+ "backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "TEST_DIR/t.IMGFMT.base",
+ "filename": "TEST_DIR/t.IMGFMT.mid",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
+ "full-backing-filename": "TEST_DIR/t.IMGFMT.base",
+ "backing-filename": "TEST_DIR/t.IMGFMT.base",
"dirty-flag": false
},
"iops_wr": 0,
"ro": false,
- "node-name": "base",
- "backing_file_depth": 0,
+ "node-name": "mid",
+ "backing_file_depth": 1,
"drv": "IMGFMT",
"iops": 0,
"bps_wr": 0,
"write_threshold": 0,
+ "backing_file": "TEST_DIR/t.IMGFMT.base",
"encrypted": false,
"bps": 0,
"bps_rd": 0,
@@ -278,7 +278,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "TEST_DIR/t.IMGFMT.base",
+ "file": "TEST_DIR/t.IMGFMT.mid",
"encryption_key_missing": false
},
{
@@ -286,7 +286,7 @@ wrote 65536/65536 bytes at offset 1048576
"detect_zeroes": "off",
"image": {
"virtual-size": 393216,
- "filename": "TEST_DIR/t.IMGFMT.base",
+ "filename": "TEST_DIR/t.IMGFMT.mid",
"format": "file",
"actual-size": SIZE,
"dirty-flag": false
@@ -307,7 +307,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "TEST_DIR/t.IMGFMT.base",
+ "file": "TEST_DIR/t.IMGFMT.mid",
"encryption_key_missing": false
}
]
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 05/26] block: Respect backing bs in bdrv_refresh_filename
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (3 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 04/26] iotests: Drop explicit base blockdev in 191 Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-06 14:00 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 06/26] block: Make path_combine() return the path Max Reitz
` (20 subsequent siblings)
25 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
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 <mreitz@redhat.com>
---
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 66e37219b9..249616af68 100644
--- a/block.c
+++ b/block.c
@@ -5071,6 +5071,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 */
@@ -5082,11 +5083,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) {
+ 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 dd9846d1ce..6bf76ed0fb 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)
@@ -149,7 +149,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)
@@ -169,7 +169,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:{"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)
@@ -189,7 +189,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) 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, 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 830c11880a..777a6b3adc 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) 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)
@@ -241,7 +241,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)
@@ -261,7 +261,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:{"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)
@@ -281,7 +281,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) 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, ignore flushes
Backing file: TEST_DIR/t.qcow2.base (chain depth: 1)
diff --git a/tests/qemu-iotests/191.out b/tests/qemu-iotests/191.out
index 575cd3ea2d..b22471b40a 100644
--- a/tests/qemu-iotests/191.out
+++ b/tests/qemu-iotests/191.out
@@ -52,7 +52,7 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "TEST_DIR/t.IMGFMT.ovl2",
+ "filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
@@ -77,7 +77,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "TEST_DIR/t.IMGFMT.ovl2",
+ "file": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"encryption_key_missing": false
},
{
@@ -123,7 +123,7 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "TEST_DIR/t.IMGFMT",
+ "filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
@@ -148,7 +148,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "TEST_DIR/t.IMGFMT",
+ "file": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}",
"encryption_key_missing": false
},
{
@@ -392,7 +392,7 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "TEST_DIR/t.IMGFMT.ovl2",
+ "filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
@@ -417,7 +417,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "TEST_DIR/t.IMGFMT.ovl2",
+ "file": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"encryption_key_missing": false
},
{
@@ -464,7 +464,7 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "TEST_DIR/t.IMGFMT.ovl2",
+ "filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
@@ -474,12 +474,12 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "TEST_DIR/t.IMGFMT.ovl3",
+ "filename": "json:{"backing": {"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl3"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
- "full-backing-filename": "TEST_DIR/t.IMGFMT.ovl2",
- "backing-filename": "TEST_DIR/t.IMGFMT.ovl2",
+ "full-backing-filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
+ "backing-filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"dirty-flag": false
},
"iops_wr": 0,
@@ -490,7 +490,7 @@ wrote 65536/65536 bytes at offset 1048576
"iops": 0,
"bps_wr": 0,
"write_threshold": 0,
- "backing_file": "TEST_DIR/t.IMGFMT.ovl2",
+ "backing_file": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"encrypted": false,
"bps": 0,
"bps_rd": 0,
@@ -499,7 +499,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "TEST_DIR/t.IMGFMT.ovl3",
+ "file": "json:{"backing": {"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl3"}}",
"encryption_key_missing": false
},
{
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 06/26] block: Make path_combine() return the path
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (4 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 05/26] block: Respect backing bs in bdrv_refresh_filename Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-22 14:57 ` Kevin Wolf
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 07/26] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
` (19 subsequent siblings)
25 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Besides being safe for arbitrary path lengths, after some follow-up
patches all callers will want a freshly allocated buffer anyway.
In the meantime, path_combine_deprecated() is added which has the same
interface as path_combine() had before this patch. All callers to that
function will be converted in follow-up patches.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block.h | 4 +--
block.c | 85 ++++++++++++++++++++++++++++-----------------------
block/vmdk.c | 3 +-
3 files changed, 49 insertions(+), 43 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 9b12774ddf..993ae74b7e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -490,9 +490,7 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed,
int path_has_protocol(const char *path);
int path_is_absolute(const char *path);
-void path_combine(char *dest, int dest_size,
- const char *base_path,
- const char *filename);
+char *path_combine(const char *base_path, const char *filename);
int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
diff --git a/block.c b/block.c
index 249616af68..84ab8d4675 100644
--- a/block.c
+++ b/block.c
@@ -147,53 +147,62 @@ int path_is_absolute(const char *path)
#endif
}
-/* if filename is absolute, just copy it to dest. Otherwise, build a
+/* if filename is absolute, just return its duplicate. Otherwise, build a
path to it by considering it is relative to base_path. URL are
supported. */
-void path_combine(char *dest, int dest_size,
- const char *base_path,
- const char *filename)
+char *path_combine(const char *base_path, const char *filename)
{
+ const char *protocol_stripped = NULL;
const char *p, *p1;
+ char *result;
int len;
- if (dest_size <= 0)
- return;
if (path_is_absolute(filename)) {
- pstrcpy(dest, dest_size, filename);
- } else {
- const char *protocol_stripped = NULL;
+ return g_strdup(filename);
+ }
- if (path_has_protocol(base_path)) {
- protocol_stripped = strchr(base_path, ':');
- if (protocol_stripped) {
- protocol_stripped++;
- }
+ if (path_has_protocol(base_path)) {
+ protocol_stripped = strchr(base_path, ':');
+ if (protocol_stripped) {
+ protocol_stripped++;
}
- p = protocol_stripped ?: base_path;
+ }
+ p = protocol_stripped ?: base_path;
- p1 = strrchr(base_path, '/');
+ p1 = strrchr(base_path, '/');
#ifdef _WIN32
- {
- const char *p2;
- p2 = strrchr(base_path, '\\');
- if (!p1 || p2 > p1)
- p1 = p2;
+ {
+ const char *p2;
+ p2 = strrchr(base_path, '\\');
+ if (!p1 || p2 > p1) {
+ p1 = p2;
}
+ }
#endif
- if (p1)
- p1++;
- else
- p1 = base_path;
- if (p1 > p)
- p = p1;
- len = p - base_path;
- if (len > dest_size - 1)
- len = dest_size - 1;
- memcpy(dest, base_path, len);
- dest[len] = '\0';
- pstrcat(dest, dest_size, filename);
+ if (p1) {
+ p1++;
+ } else {
+ p1 = base_path;
+ }
+ if (p1 > p) {
+ p = p1;
}
+ len = p - base_path;
+
+ result = g_malloc(len + strlen(filename) + 1);
+ memcpy(result, base_path, len);
+ strcpy(result + len, filename);
+
+ return result;
+}
+
+static void path_combine_deprecated(char *dest, int dest_size,
+ const char *base_path,
+ const char *filename)
+{
+ char *combined = path_combine(base_path, filename);
+ pstrcpy(dest, dest_size, combined);
+ g_free(combined);
}
/*
@@ -292,7 +301,7 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed,
error_setg(errp, "Cannot use relative backing file names for '%s'",
backed);
} else {
- path_combine(dest, sz, backed, backing);
+ path_combine_deprecated(dest, sz, backed, backing);
}
}
@@ -4182,8 +4191,8 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
} else {
/* If not an absolute filename path, make it relative to the current
* image's filename path */
- path_combine(filename_tmp, PATH_MAX, curr_bs->filename,
- backing_file);
+ path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
+ backing_file);
/* We are going to compare absolute pathnames */
if (!realpath(filename_tmp, filename_full)) {
@@ -4192,8 +4201,8 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
/* We need to make sure the backing filename we are comparing against
* is relative to the current image filename (or absolute) */
- path_combine(filename_tmp, PATH_MAX, curr_bs->filename,
- curr_bs->backing_file);
+ path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
+ curr_bs->backing_file);
if (!realpath(filename_tmp, backing_file_full)) {
continue;
diff --git a/block/vmdk.c b/block/vmdk.c
index d71cec4f31..0a0f16d0f0 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -857,8 +857,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
return -EINVAL;
}
- extent_path = g_malloc0(PATH_MAX);
- path_combine(extent_path, PATH_MAX, desc_file_path, fname);
+ extent_path = path_combine(desc_file_path, fname);
ret = snprintf(extent_opt_prefix, 32, "extents.%d", s->num_extents);
assert(ret < 32);
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 07/26] block: bdrv_get_full_backing_filename_from_...'s ret. val.
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (5 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 06/26] block: Make path_combine() return the path Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 08/26] block: bdrv_get_full_backing_filename's " Max Reitz
` (18 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Make bdrv_get_full_backing_filename_from_filename() return an allocated
string instead of placing the result in a caller-provided buffer.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block.h | 7 +++----
block.c | 51 +++++++++++++++++++++++++++++++++++----------------
block/vmdk.c | 9 ++++-----
3 files changed, 42 insertions(+), 25 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 993ae74b7e..46ceb1c8d3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -483,10 +483,9 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);
void bdrv_get_full_backing_filename(BlockDriverState *bs,
char *dest, size_t sz, Error **errp);
-void bdrv_get_full_backing_filename_from_filename(const char *backed,
- const char *backing,
- char *dest, size_t sz,
- Error **errp);
+char *bdrv_get_full_backing_filename_from_filename(const char *backed,
+ const char *backing,
+ Error **errp);
int path_has_protocol(const char *path);
int path_is_absolute(const char *path);
diff --git a/block.c b/block.c
index 84ab8d4675..6269d25093 100644
--- a/block.c
+++ b/block.c
@@ -288,20 +288,28 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
return 0;
}
-void bdrv_get_full_backing_filename_from_filename(const char *backed,
- const char *backing,
- char *dest, size_t sz,
- Error **errp)
+/* If @backing is empty, this function returns NULL without setting
+ * @errp. In all other cases, NULL will only be returned with @errp
+ * set.
+ *
+ * Therefore, a return value of NULL without @errp set means that
+ * there is no backing file; if @errp is set, there is one but its
+ * absolute filename cannot be generated.
+ */
+char *bdrv_get_full_backing_filename_from_filename(const char *backed,
+ const char *backing,
+ Error **errp)
{
- if (backing[0] == '\0' || path_has_protocol(backing) ||
- path_is_absolute(backing))
- {
- pstrcpy(dest, sz, backing);
+ if (backing[0] == '\0') {
+ return NULL;
+ } else if (path_has_protocol(backing) || path_is_absolute(backing)) {
+ return g_strdup(backing);
} else if (backed[0] == '\0' || strstart(backed, "json:", NULL)) {
error_setg(errp, "Cannot use relative backing file names for '%s'",
backed);
+ return NULL;
} else {
- path_combine_deprecated(dest, sz, backed, backing);
+ return path_combine(backed, backing);
}
}
@@ -309,9 +317,20 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz,
Error **errp)
{
char *backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
+ char *full_name;
+ Error *local_error = NULL;
- bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file,
- dest, sz, errp);
+ full_name = bdrv_get_full_backing_filename_from_filename(backed,
+ bs->backing_file,
+ &local_error);
+ if (full_name) {
+ pstrcpy(dest, sz, full_name);
+ g_free(full_name);
+ } else if (local_error) {
+ error_propagate(errp, local_error);
+ } else if (sz > 0) {
+ *dest = '\0';
+ }
}
void bdrv_register(BlockDriver *bdrv)
@@ -4647,17 +4666,17 @@ void bdrv_img_create(const char *filename, const char *fmt,
size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, img_size);
if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
BlockDriverState *bs;
- char *full_backing = g_new0(char, PATH_MAX);
+ char *full_backing;
int back_flags;
QDict *backing_options = NULL;
- bdrv_get_full_backing_filename_from_filename(filename, backing_file,
- full_backing, PATH_MAX,
- &local_err);
+ full_backing =
+ bdrv_get_full_backing_filename_from_filename(filename, backing_file,
+ &local_err);
if (local_err) {
- g_free(full_backing);
goto out;
}
+ assert(full_backing);
/* backing files always opened read-only */
back_flags = flags;
diff --git a/block/vmdk.c b/block/vmdk.c
index 0a0f16d0f0..e61d15bc92 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2017,16 +2017,15 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
}
if (backing_file) {
BlockBackend *blk;
- char *full_backing = g_new0(char, PATH_MAX);
- bdrv_get_full_backing_filename_from_filename(filename, backing_file,
- full_backing, PATH_MAX,
- &local_err);
+ char *full_backing =
+ bdrv_get_full_backing_filename_from_filename(filename, backing_file,
+ &local_err);
if (local_err) {
- g_free(full_backing);
error_propagate(errp, local_err);
ret = -ENOENT;
goto exit;
}
+ assert(full_backing);
blk = blk_new_open(full_backing, NULL, NULL,
BDRV_O_NO_BACKING, errp);
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 08/26] block: bdrv_get_full_backing_filename's ret. val.
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (6 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 07/26] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 09/26] block: Add bdrv_make_absolute_filename() Max Reitz
` (17 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Make bdrv_get_full_backing_filename() return an allocated string instead
of placing the result in a caller-provided buffer.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block.h | 3 +--
block.c | 47 +++++++++++++++++------------------------------
block/qapi.c | 12 ++----------
3 files changed, 20 insertions(+), 42 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 46ceb1c8d3..fafb8a6a7a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -481,8 +481,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs,
const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);
-void bdrv_get_full_backing_filename(BlockDriverState *bs,
- char *dest, size_t sz, Error **errp);
+char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp);
char *bdrv_get_full_backing_filename_from_filename(const char *backed,
const char *backing,
Error **errp);
diff --git a/block.c b/block.c
index 6269d25093..483dcf863e 100644
--- a/block.c
+++ b/block.c
@@ -313,24 +313,13 @@ char *bdrv_get_full_backing_filename_from_filename(const char *backed,
}
}
-void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz,
- Error **errp)
+char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
{
char *backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
- char *full_name;
- Error *local_error = NULL;
- full_name = bdrv_get_full_backing_filename_from_filename(backed,
- bs->backing_file,
- &local_error);
- if (full_name) {
- pstrcpy(dest, sz, full_name);
- g_free(full_name);
- } else if (local_error) {
- error_propagate(errp, local_error);
- } else if (sz > 0) {
- *dest = '\0';
- }
+ return bdrv_get_full_backing_filename_from_filename(backed,
+ bs->backing_file,
+ errp);
}
void bdrv_register(BlockDriver *bdrv)
@@ -2261,7 +2250,7 @@ out:
int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp)
{
- char *backing_filename = g_malloc0(PATH_MAX);
+ char *backing_filename = NULL;
char *bdref_key_dot;
const char *reference = NULL;
int ret = 0;
@@ -2295,7 +2284,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
*/
reference = qdict_get_try_str(parent_options, bdref_key);
if (reference || qdict_haskey(options, "file.filename")) {
- backing_filename[0] = '\0';
+ /* keep backing_filename NULL */
/* FIXME: Should also be set to true if @options contains other runtime
* options which control the data that is read from the backing
@@ -2305,8 +2294,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
QDECREF(options);
goto free_exit;
} else {
- bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX,
- &local_err);
+ backing_filename = bdrv_get_full_backing_filename(bs, &local_err);
if (local_err) {
ret = -EINVAL;
error_propagate(errp, local_err);
@@ -2327,9 +2315,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
qdict_put_str(options, "driver", bs->backing_format);
}
- backing_hd = bdrv_open_inherit(*backing_filename ? backing_filename : NULL,
- reference, options, 0, bs, &child_backing,
- errp);
+ backing_hd = bdrv_open_inherit(backing_filename, reference, options, 0, bs,
+ &child_backing, errp);
if (!backing_hd) {
bs->open_flags |= BDRV_O_NO_BACKING;
error_prepend(errp, "Could not open backing file: ");
@@ -4174,7 +4161,6 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
int is_protocol = 0;
BlockDriverState *curr_bs = NULL;
BlockDriverState *retval = NULL;
- Error *local_error = NULL;
if (!bs || !bs->drv || !backing_file) {
return NULL;
@@ -4191,21 +4177,22 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
/* If either of the filename paths is actually a protocol, then
* compare unmodified paths; otherwise make paths relative */
if (is_protocol || path_has_protocol(curr_bs->backing_file)) {
+ char *backing_file_full_ret;
+
if (strcmp(backing_file, curr_bs->backing_file) == 0) {
retval = curr_bs->backing->bs;
break;
}
/* Also check against the full backing filename for the image */
- bdrv_get_full_backing_filename(curr_bs, backing_file_full, PATH_MAX,
- &local_error);
- if (local_error == NULL) {
- if (strcmp(backing_file, backing_file_full) == 0) {
+ backing_file_full_ret = bdrv_get_full_backing_filename(curr_bs,
+ NULL);
+ if (backing_file_full_ret) {
+ bool equal = strcmp(backing_file, backing_file_full_ret) == 0;
+ g_free(backing_file_full_ret);
+ if (equal) {
retval = curr_bs->backing->bs;
break;
}
- } else {
- error_free(local_error);
- local_error = NULL;
}
} else {
/* If not an absolute filename path, make it relative to the current
diff --git a/block/qapi.c b/block/qapi.c
index fc10f0a565..c7da9bd312 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -282,18 +282,10 @@ void bdrv_query_image_info(BlockDriverState *bs,
backing_filename = bs->backing_file;
if (backing_filename[0] != '\0') {
- char *backing_filename2 = g_malloc0(PATH_MAX);
+ char *backing_filename2;
info->backing_filename = g_strdup(backing_filename);
info->has_backing_filename = true;
- bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err);
- if (err) {
- /* Can't reconstruct the full backing filename, so we must omit
- * this field and apply a Best Effort to this query. */
- g_free(backing_filename2);
- backing_filename2 = NULL;
- error_free(err);
- err = NULL;
- }
+ backing_filename2 = bdrv_get_full_backing_filename(bs, NULL);
/* Always report the full_backing_filename if present, even if it's the
* same as backing_filename. That they are same is useful info. */
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 09/26] block: Add bdrv_make_absolute_filename()
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (7 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 08/26] block: bdrv_get_full_backing_filename's " Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 10/26] block: Fix bdrv_find_backing_image() Max Reitz
` (16 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
This is a general function for making a filename that is relative to a
certain BDS absolute.
It calls bdrv_get_full_backing_filename_from_filename() for now, but
that will be changed in a follow-up patch.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 483dcf863e..f404728732 100644
--- a/block.c
+++ b/block.c
@@ -313,13 +313,24 @@ char *bdrv_get_full_backing_filename_from_filename(const char *backed,
}
}
-char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
+/* If @filename is empty or NULL, this function returns NULL without
+ * setting @errp. In all other cases, NULL will only be returned with
+ * @errp set.
+ */
+static char *bdrv_make_absolute_filename(BlockDriverState *relative_to,
+ const char *filename, Error **errp)
{
- char *backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
+ char *bs_filename = relative_to->exact_filename[0]
+ ? relative_to->exact_filename
+ : relative_to->filename;
+
+ return bdrv_get_full_backing_filename_from_filename(bs_filename,
+ filename ?: "", errp);
+}
- return bdrv_get_full_backing_filename_from_filename(backed,
- bs->backing_file,
- errp);
+char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
+{
+ return bdrv_make_absolute_filename(bs, bs->backing_file, errp);
}
void bdrv_register(BlockDriver *bdrv)
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 10/26] block: Fix bdrv_find_backing_image()
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (8 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 09/26] block: Add bdrv_make_absolute_filename() Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 11/26] block: Add bdrv_dirname() Max Reitz
` (15 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
bdrv_find_backing_image() should use bdrv_get_full_backing_filename() or
bdrv_make_absolute_filename() instead of trying to do what those
functions do by itself.
path_combine_deprecated() can now be dropped, so let's do that.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 30 ++++++++++--------------------
1 file changed, 10 insertions(+), 20 deletions(-)
diff --git a/block.c b/block.c
index f404728732..06ee694ac5 100644
--- a/block.c
+++ b/block.c
@@ -196,15 +196,6 @@ char *path_combine(const char *base_path, const char *filename)
return result;
}
-static void path_combine_deprecated(char *dest, int dest_size,
- const char *base_path,
- const char *filename)
-{
- char *combined = path_combine(base_path, filename);
- pstrcpy(dest, dest_size, combined);
- g_free(combined);
-}
-
/*
* Helper function for bdrv_parse_filename() implementations to remove optional
* protocol prefixes (especially "file:") from a filename and for putting the
@@ -4179,7 +4170,6 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
filename_full = g_malloc(PATH_MAX);
backing_file_full = g_malloc(PATH_MAX);
- filename_tmp = g_malloc(PATH_MAX);
is_protocol = path_has_protocol(backing_file);
@@ -4208,22 +4198,23 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
} else {
/* If not an absolute filename path, make it relative to the current
* image's filename path */
- path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
- backing_file);
-
- /* We are going to compare absolute pathnames */
- if (!realpath(filename_tmp, filename_full)) {
+ filename_tmp = bdrv_make_absolute_filename(curr_bs, backing_file,
+ NULL);
+ /* We are going to compare canonicalized absolute pathnames */
+ if (!filename_tmp || !realpath(filename_tmp, filename_full)) {
+ g_free(filename_tmp);
continue;
}
+ g_free(filename_tmp);
/* We need to make sure the backing filename we are comparing against
* is relative to the current image filename (or absolute) */
- path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
- curr_bs->backing_file);
-
- if (!realpath(filename_tmp, backing_file_full)) {
+ filename_tmp = bdrv_get_full_backing_filename(curr_bs, NULL);
+ if (!filename_tmp || !realpath(filename_tmp, backing_file_full)) {
+ g_free(filename_tmp);
continue;
}
+ g_free(filename_tmp);
if (strcmp(backing_file_full, filename_full) == 0) {
retval = curr_bs->backing->bs;
@@ -4234,7 +4225,6 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
g_free(filename_full);
g_free(backing_file_full);
- g_free(filename_tmp);
return retval;
}
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 11/26] block: Add bdrv_dirname()
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (9 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 10/26] block: Fix bdrv_find_backing_image() Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 12/26] blkverify: Make bdrv_dirname() return NULL Max Reitz
` (14 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
This function may be implemented by block drivers to derive a directory
name from a BDS. Concatenating this g_free()-able string with a relative
filename must result in a valid (not necessarily existing) filename, so
this is a function that should generally be not implemented by format
drivers, because this is protocol-specific.
If a BDS's driver does not implement this function, bdrv_dirname() will
fall through to the BDS's file if it exists. If it does not, the
exact_filename field will be used to generate a directory name.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block.h | 1 +
include/block/block_int.h | 7 +++++++
block.c | 26 ++++++++++++++++++++++++++
3 files changed, 34 insertions(+)
diff --git a/include/block/block.h b/include/block/block.h
index fafb8a6a7a..1983055916 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -485,6 +485,7 @@ char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp);
char *bdrv_get_full_backing_filename_from_filename(const char *backed,
const char *backing,
Error **errp);
+char *bdrv_dirname(BlockDriverState *bs, Error **errp);
int path_has_protocol(const char *path);
int path_is_absolute(const char *path);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b5c124fcbd..8b0413e3df 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -135,6 +135,13 @@ struct BlockDriver {
void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
+ /*
+ * Returns an allocated string which is the directory name of this BDS: It
+ * will be used to make relative filenames absolute by prepending this
+ * function's return value to them.
+ */
+ char *(*bdrv_dirname)(BlockDriverState *bs, Error **errp);
+
/* aio */
BlockAIOCB *(*bdrv_aio_readv)(BlockDriverState *bs,
int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
diff --git a/block.c b/block.c
index 06ee694ac5..8957a3d4dc 100644
--- a/block.c
+++ b/block.c
@@ -5153,6 +5153,32 @@ void bdrv_refresh_filename(BlockDriverState *bs)
}
}
+char *bdrv_dirname(BlockDriverState *bs, Error **errp)
+{
+ BlockDriver *drv = bs->drv;
+
+ if (!drv) {
+ error_setg(errp, "Node '%s' is ejected", bs->node_name);
+ return NULL;
+ }
+
+ if (drv->bdrv_dirname) {
+ return drv->bdrv_dirname(bs, errp);
+ }
+
+ if (bs->file) {
+ return bdrv_dirname(bs->file->bs, errp);
+ }
+
+ if (bs->exact_filename[0] != '\0') {
+ return path_combine(bs->exact_filename, "");
+ }
+
+ error_setg(errp, "Cannot generate a base directory for %s nodes",
+ drv->format_name);
+ return NULL;
+}
+
/*
* Hot add/remove a BDS's child. So the user can take a child offline when
* it is broken and take a new child online
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 12/26] blkverify: Make bdrv_dirname() return NULL
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (10 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 11/26] block: Add bdrv_dirname() Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 13/26] quorum: " Max Reitz
` (13 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
blkverify's BDSs have a file BDS, but we do not want this to be
preferred over the raw node. There is no way to decide between the two
(and not really a reason to, either), so just return NULL in blkverify's
implementation of bdrv_dirname().
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/blkverify.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/block/blkverify.c b/block/blkverify.c
index b2ed8cd70d..d5233eeaf9 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -309,6 +309,15 @@ static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options)
}
}
+static char *blkverify_dirname(BlockDriverState *bs, Error **errp)
+{
+ /* In general, there are two BDSs with different dirnames below this one;
+ * so there is no unique dirname we could return (unless both are equal by
+ * chance). Therefore, to be consistent, just always return NULL. */
+ error_setg(errp, "Cannot generate a base directory for blkverify nodes");
+ return NULL;
+}
+
static BlockDriver bdrv_blkverify = {
.format_name = "blkverify",
.protocol_name = "blkverify",
@@ -320,6 +329,7 @@ static BlockDriver bdrv_blkverify = {
.bdrv_child_perm = bdrv_filter_default_perms,
.bdrv_getlength = blkverify_getlength,
.bdrv_refresh_filename = blkverify_refresh_filename,
+ .bdrv_dirname = blkverify_dirname,
.bdrv_co_preadv = blkverify_co_preadv,
.bdrv_co_pwritev = blkverify_co_pwritev,
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 13/26] quorum: Make bdrv_dirname() return NULL
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (11 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 12/26] blkverify: Make bdrv_dirname() return NULL Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 14/26] block/nbd: " Max Reitz
` (12 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
While the common implementation for bdrv_dirname() should return NULL
for quorum BDSs already (because they do not have a file node and their
exact_filename field should be empty), there is no reason not to make
that explicit.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/quorum.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/block/quorum.c b/block/quorum.c
index 2f1a628449..e5a844335e 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1095,6 +1095,16 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
bs->full_open_options = opts;
}
+static char *quorum_dirname(BlockDriverState *bs, Error **errp)
+{
+ /* In general, there are multiple BDSs with different dirnames below this
+ * one; so there is no unique dirname we could return (unless all are equal
+ * by chance, or there is only one). Therefore, to be consistent, just
+ * always return NULL. */
+ error_setg(errp, "Cannot generate a base directory for quorum nodes");
+ return NULL;
+}
+
static BlockDriver bdrv_quorum = {
.format_name = "quorum",
.protocol_name = "quorum",
@@ -1104,6 +1114,7 @@ static BlockDriver bdrv_quorum = {
.bdrv_file_open = quorum_open,
.bdrv_close = quorum_close,
.bdrv_refresh_filename = quorum_refresh_filename,
+ .bdrv_dirname = quorum_dirname,
.bdrv_co_flush_to_disk = quorum_co_flush,
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 14/26] block/nbd: Make bdrv_dirname() return NULL
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (12 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 13/26] quorum: " Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 15/26] block/nfs: Implement bdrv_dirname() Max Reitz
` (11 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
The generic bdrv_dirname() implementation would be able to generate some
form of directory name for many NBD nodes, but it would be always wrong.
Therefore, we have to explicitly make it an error (until NBD has some
form of specification for export paths, if it ever will).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/nbd.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/block/nbd.c b/block/nbd.c
index 94220f6d14..dcea7ffaef 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -574,6 +574,16 @@ static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return 0;
}
+static char *nbd_dirname(BlockDriverState *bs, Error **errp)
+{
+ /* The generic bdrv_dirname() implementation is able to work out some
+ * directory name for NBD nodes, but that would be wrong. So far there is no
+ * specification for how "export paths" would work, so NBD does not have
+ * directory names. */
+ error_setg(errp, "Cannot generate a base directory for NBD nodes");
+ return NULL;
+}
+
static BlockDriver bdrv_nbd = {
.format_name = "nbd",
.protocol_name = "nbd",
@@ -592,6 +602,7 @@ static BlockDriver bdrv_nbd = {
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_get_info = nbd_get_info,
+ .bdrv_dirname = nbd_dirname,
};
static BlockDriver bdrv_nbd_tcp = {
@@ -612,6 +623,7 @@ static BlockDriver bdrv_nbd_tcp = {
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_get_info = nbd_get_info,
+ .bdrv_dirname = nbd_dirname,
};
static BlockDriver bdrv_nbd_unix = {
@@ -632,6 +644,7 @@ static BlockDriver bdrv_nbd_unix = {
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_get_info = nbd_get_info,
+ .bdrv_dirname = nbd_dirname,
};
static void bdrv_nbd_init(void)
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 15/26] block/nfs: Implement bdrv_dirname()
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (13 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 14/26] block/nbd: " Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 16/26] block: Use bdrv_dirname() for relative filenames Max Reitz
` (10 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
While the basic idea is obvious and could be handled by the default
bdrv_dirname() implementation, we cannot generate a directory name if
the gid or uid are set, so we have to explicitly return NULL in those
cases.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/nfs.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/block/nfs.c b/block/nfs.c
index effc8719b5..ee1160980a 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -874,6 +874,19 @@ static void nfs_refresh_filename(BlockDriverState *bs, QDict *options)
bs->full_open_options = opts;
}
+static char *nfs_dirname(BlockDriverState *bs, Error **errp)
+{
+ NFSClient *client = bs->opaque;
+
+ if (client->uid || client->gid) {
+ error_setg(errp, "Cannot generate a base directory for NBD node '%s'",
+ bs->filename);
+ return NULL;
+ }
+
+ return g_strdup_printf("nfs://%s%s/", client->server->host, client->path);
+}
+
#ifdef LIBNFS_FEATURE_PAGECACHE
static void nfs_invalidate_cache(BlockDriverState *bs,
Error **errp)
@@ -907,6 +920,7 @@ static BlockDriver bdrv_nfs = {
.bdrv_detach_aio_context = nfs_detach_aio_context,
.bdrv_attach_aio_context = nfs_attach_aio_context,
.bdrv_refresh_filename = nfs_refresh_filename,
+ .bdrv_dirname = nfs_dirname,
#ifdef LIBNFS_FEATURE_PAGECACHE
.bdrv_invalidate_cache = nfs_invalidate_cache,
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 16/26] block: Use bdrv_dirname() for relative filenames
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (14 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 15/26] block/nfs: Implement bdrv_dirname() Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 17/26] iotests: Add quorum case to test 110 Max Reitz
` (9 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
bdrv_get_full_backing_filename_from_filename() breaks down when it comes
to JSON filenames. Using bdrv_dirname() as the basis is better because
since we have BDS, we can descend through the BDS tree to the protocol
layer, which gives us a greater probability of finding a non-JSON name;
also, bdrv_dirname() is more correct as it allows block drivers to
override the generation of that directory name in a protocol-specific
way.
We still need to keep bdrv_get_full_backing_filename_from_filename(),
though, because it has valid callers which need it during image creation
when no BDS is available yet.
This makes a test case in qemu-iotest 110, which was supposed to fail,
work. That is actually good, but we need to change the reference output
(and the comment in 110) accordingly.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 20 +++++++++++++++-----
tests/qemu-iotests/110 | 3 ++-
tests/qemu-iotests/110.out | 2 +-
3 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 8957a3d4dc..f23b35405d 100644
--- a/block.c
+++ b/block.c
@@ -311,12 +311,22 @@ char *bdrv_get_full_backing_filename_from_filename(const char *backed,
static char *bdrv_make_absolute_filename(BlockDriverState *relative_to,
const char *filename, Error **errp)
{
- char *bs_filename = relative_to->exact_filename[0]
- ? relative_to->exact_filename
- : relative_to->filename;
+ char *dir, *full_name;
- return bdrv_get_full_backing_filename_from_filename(bs_filename,
- filename ?: "", errp);
+ if (!filename || filename[0] == '\0') {
+ return NULL;
+ } else if (path_has_protocol(filename) || path_is_absolute(filename)) {
+ return g_strdup(filename);
+ }
+
+ dir = bdrv_dirname(relative_to, errp);
+ if (!dir) {
+ return NULL;
+ }
+
+ full_name = g_strconcat(dir, filename, NULL);
+ g_free(dir);
+ return full_name;
}
char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp)
diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
index 9de7369f3a..ba1b3c6c7d 100755
--- a/tests/qemu-iotests/110
+++ b/tests/qemu-iotests/110
@@ -61,7 +61,8 @@ echo '=== Non-reconstructable filename ==='
echo
# Across blkdebug without a config file, you cannot reconstruct filenames, so
-# qemu is incapable of knowing the directory of the top image
+# qemu is incapable of knowing the directory of the top image from the filename
+# alone. However, using bdrv_dirname(), it should still work.
TEST_IMG="json:{
'driver': '$IMGFMT',
'file': {
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index b3584ff87f..5370bc1d26 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -14,7 +14,7 @@ backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
image: json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "set-state.0.new_state": 42}}
file format: IMGFMT
virtual size: 64M (67108864 bytes)
-backing file: t.IMGFMT.base (cannot determine actual path)
+backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
=== Backing name is always relative to the backed image ===
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 17/26] iotests: Add quorum case to test 110
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (15 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 16/26] block: Use bdrv_dirname() for relative filenames Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver Max Reitz
` (8 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Test 110 tests relative backing filenames for complex BDS trees. Now
that the originally supposedly failing test passes, let us add a new
failing test: Quorum can never work automatically (without detecting
whether all child nodes have the same base directory, but that would be
rather inconsistent behavior).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
tests/qemu-iotests/110 | 26 ++++++++++++++++++++++++++
tests/qemu-iotests/110.out | 7 +++++++
2 files changed, 33 insertions(+)
diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
index ba1b3c6c7d..08976c8a61 100755
--- a/tests/qemu-iotests/110
+++ b/tests/qemu-iotests/110
@@ -30,6 +30,7 @@ status=1 # failure is the default!
_cleanup()
{
_cleanup_test_img
+ rm -f "$TEST_IMG.copy"
}
trap "_cleanup; exit \$status" 0 1 2 3 15
@@ -87,6 +88,31 @@ echo
# omit the image size; it should work anyway
_make_test_img -b "$TEST_IMG_REL.base"
+echo
+echo '=== Nodes without a common directory ==='
+echo
+
+cp "$TEST_IMG" "$TEST_IMG.copy"
+
+# Should inform us that the actual path of the backing file cannot be determined
+TEST_IMG="json:{
+ 'driver': '$IMGFMT',
+ 'file': {
+ 'driver': 'quorum',
+ 'vote-threshold': 1,
+ 'children': [
+ {
+ 'driver': 'file',
+ 'filename': '$TEST_IMG'
+ },
+ {
+ 'driver': 'file',
+ 'filename': '$TEST_IMG.copy'
+ }
+ ]
+ }
+}" _img_info | _filter_img_info
+
# success, all done
echo '*** done'
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index 5370bc1d26..1d0b2475cc 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -19,4 +19,11 @@ backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
=== Backing name is always relative to the backed image ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=t.IMGFMT.base
+
+=== Nodes without a common directory ===
+
+image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "blkverify": false, "rewrite-corrupted": false, "vote-threshold": 1}}
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+backing file: t.IMGFMT.base (cannot determine actual path)
*** done
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (16 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 17/26] iotests: Add quorum case to test 110 Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-06 15:23 ` Alberto Garcia
2018-02-06 19:43 ` Eric Blake
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 19/26] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
` (7 subsequent siblings)
25 siblings, 2 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
This new field can be set by block drivers to list the runtime options
they accept that may influence the contents of the respective BDS. As of
a follow-up patch, this list will be used by the common
bdrv_refresh_filename() implementation to decide which options to put
into BDS.full_open_options (and consequently whether a JSON filename has
to be created), thus freeing the drivers of having to implement that
logic themselves.
Additionally, this patch adds the field to all of the block drivers that
need it and sets it accordingly.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/block/block_int.h | 7 +++++++
block/blkdebug.c | 17 +++++++++++++++++
block/crypto.c | 8 ++++++++
block/curl.c | 21 +++++++++++++++++++++
block/gluster.c | 19 +++++++++++++++++++
block/iscsi.c | 18 ++++++++++++++++++
block/nbd.c | 14 ++++++++++++++
block/nfs.c | 11 +++++++++++
block/null.c | 9 +++++++++
block/qcow.c | 7 +++++++
block/qcow2.c | 7 +++++++
block/quorum.c | 11 +++++++++++
block/raw-format.c | 10 +++++++++-
block/rbd.c | 14 ++++++++++++++
block/replication.c | 8 ++++++++
block/sheepdog.c | 12 ++++++++++++
block/ssh.c | 12 ++++++++++++
block/throttle.c | 7 +++++++
block/vpc.c | 7 +++++++
block/vvfat.c | 12 ++++++++++++
block/vxhs.c | 11 +++++++++++
21 files changed, 241 insertions(+), 1 deletion(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8b0413e3df..9e6800d42e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -454,6 +454,13 @@ struct BlockDriver {
Error **errp);
QLIST_ENTRY(BlockDriver) list;
+
+ /* Pointer to a NULL-terminated array of names of significant options that
+ * can be specified for bdrv_open(). A significant option is one that
+ * changes the data of a BDS.
+ * If this pointer is NULL, the array is considered empty.
+ * "filename" and "driver" are always considered significant. */
+ const char *const *sgfnt_runtime_opts;
};
typedef struct BlockLimits {
diff --git a/block/blkdebug.c b/block/blkdebug.c
index e21669979d..f93139da58 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -886,6 +886,21 @@ static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
return 0;
}
+static const char *const blkdebug_sgfnt_runtime_opts[] = {
+ "config",
+ "inject-error.",
+ "set-state.",
+ "suspend.",
+ "align",
+ "max-transfer",
+ "opt-write-zero",
+ "max-write-zero",
+ "opt-discard",
+ "max-discard",
+
+ NULL
+};
+
static BlockDriver bdrv_blkdebug = {
.format_name = "blkdebug",
.protocol_name = "blkdebug",
@@ -915,6 +930,8 @@ static BlockDriver bdrv_blkdebug = {
= blkdebug_debug_remove_breakpoint,
.bdrv_debug_resume = blkdebug_debug_resume,
.bdrv_debug_is_suspended = blkdebug_debug_is_suspended,
+
+ .sgfnt_runtime_opts = blkdebug_sgfnt_runtime_opts,
};
static void bdrv_blkdebug_init(void)
diff --git a/block/crypto.c b/block/crypto.c
index 60ddf8623e..492e64e922 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -609,6 +609,12 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs)
return spec_info;
}
+static const char *const block_crypto_sgfnt_runtime_opts[] = {
+ BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,
+
+ NULL
+};
+
BlockDriver bdrv_crypto_luks = {
.format_name = "luks",
.instance_size = sizeof(BlockCrypto),
@@ -626,6 +632,8 @@ BlockDriver bdrv_crypto_luks = {
.bdrv_getlength = block_crypto_getlength,
.bdrv_get_info = block_crypto_get_info_luks,
.bdrv_get_specific_info = block_crypto_get_specific_info_luks,
+
+ .sgfnt_runtime_opts = block_crypto_sgfnt_runtime_opts,
};
static void block_crypto_init(void)
diff --git a/block/curl.c b/block/curl.c
index 35cf417f59..7c5418324f 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -969,6 +969,19 @@ static int64_t curl_getlength(BlockDriverState *bs)
return s->len;
}
+static const char *const curl_sgfnt_runtime_opts[] = {
+ CURL_BLOCK_OPT_URL,
+ CURL_BLOCK_OPT_SSLVERIFY,
+ CURL_BLOCK_OPT_COOKIE,
+ CURL_BLOCK_OPT_COOKIE_SECRET,
+ CURL_BLOCK_OPT_USERNAME,
+ CURL_BLOCK_OPT_PASSWORD_SECRET,
+ CURL_BLOCK_OPT_PROXY_USERNAME,
+ CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET,
+
+ NULL
+};
+
static BlockDriver bdrv_http = {
.format_name = "http",
.protocol_name = "http",
@@ -983,6 +996,8 @@ static BlockDriver bdrv_http = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+
+ .sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
};
static BlockDriver bdrv_https = {
@@ -999,6 +1014,8 @@ static BlockDriver bdrv_https = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+
+ .sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
};
static BlockDriver bdrv_ftp = {
@@ -1015,6 +1032,8 @@ static BlockDriver bdrv_ftp = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+
+ .sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
};
static BlockDriver bdrv_ftps = {
@@ -1031,6 +1050,8 @@ static BlockDriver bdrv_ftps = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+
+ .sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
};
static void curl_block_init(void)
diff --git a/block/gluster.c b/block/gluster.c
index 0f4265a3a4..e47bda97df 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1414,6 +1414,21 @@ static int64_t coroutine_fn qemu_gluster_co_get_block_status(
}
+static const char *const gluster_sgfnt_open_opts[] = {
+ GLUSTER_OPT_VOLUME,
+ GLUSTER_OPT_PATH,
+ GLUSTER_OPT_TYPE,
+ GLUSTER_OPT_SERVER_PATTERN,
+ GLUSTER_OPT_HOST,
+ GLUSTER_OPT_PORT,
+ GLUSTER_OPT_TO,
+ GLUSTER_OPT_IPV4,
+ GLUSTER_OPT_IPV6,
+ GLUSTER_OPT_SOCKET,
+
+ NULL
+};
+
static BlockDriver bdrv_gluster = {
.format_name = "gluster",
.protocol_name = "gluster",
@@ -1440,6 +1455,7 @@ static BlockDriver bdrv_gluster = {
#endif
.bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
.create_opts = &qemu_gluster_create_opts,
+ .sgfnt_runtime_opts = gluster_sgfnt_open_opts,
};
static BlockDriver bdrv_gluster_tcp = {
@@ -1468,6 +1484,7 @@ static BlockDriver bdrv_gluster_tcp = {
#endif
.bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
.create_opts = &qemu_gluster_create_opts,
+ .sgfnt_runtime_opts = gluster_sgfnt_open_opts,
};
static BlockDriver bdrv_gluster_unix = {
@@ -1496,6 +1513,7 @@ static BlockDriver bdrv_gluster_unix = {
#endif
.bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
.create_opts = &qemu_gluster_create_opts,
+ .sgfnt_runtime_opts = gluster_sgfnt_open_opts,
};
/* rdma is deprecated (actually never supported for volfile fetch).
@@ -1530,6 +1548,7 @@ static BlockDriver bdrv_gluster_rdma = {
#endif
.bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
.create_opts = &qemu_gluster_create_opts,
+ .sgfnt_runtime_opts = gluster_sgfnt_open_opts,
};
static void bdrv_gluster_init(void)
diff --git a/block/iscsi.c b/block/iscsi.c
index 6a1c53711a..f77acf68af 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2183,6 +2183,20 @@ static QemuOptsList iscsi_create_opts = {
}
};
+static const char *const iscsi_sgfnt_runtime_opts[] = {
+ "transport",
+ "portal",
+ "target",
+ "user",
+ "password",
+ "password-secret",
+ "lun",
+ "initiator-name",
+ "header-digest",
+
+ NULL
+};
+
static BlockDriver bdrv_iscsi = {
.format_name = "iscsi",
.protocol_name = "iscsi",
@@ -2215,6 +2229,8 @@ static BlockDriver bdrv_iscsi = {
.bdrv_detach_aio_context = iscsi_detach_aio_context,
.bdrv_attach_aio_context = iscsi_attach_aio_context,
+
+ .sgfnt_runtime_opts = iscsi_sgfnt_runtime_opts,
};
#if LIBISCSI_API_VERSION >= (20160603)
@@ -2250,6 +2266,8 @@ static BlockDriver bdrv_iser = {
.bdrv_detach_aio_context = iscsi_detach_aio_context,
.bdrv_attach_aio_context = iscsi_attach_aio_context,
+
+ .sgfnt_runtime_opts = iscsi_sgfnt_runtime_opts,
};
#endif
diff --git a/block/nbd.c b/block/nbd.c
index dcea7ffaef..44a60f59bd 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -584,6 +584,17 @@ static char *nbd_dirname(BlockDriverState *bs, Error **errp)
return NULL;
}
+static const char *const nbd_sgfnt_runtime_opts[] = {
+ "path",
+ "host",
+ "port",
+ "export",
+ "tls-creds",
+ "server.",
+
+ NULL
+};
+
static BlockDriver bdrv_nbd = {
.format_name = "nbd",
.protocol_name = "nbd",
@@ -603,6 +614,7 @@ static BlockDriver bdrv_nbd = {
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_get_info = nbd_get_info,
.bdrv_dirname = nbd_dirname,
+ .sgfnt_runtime_opts = nbd_sgfnt_runtime_opts,
};
static BlockDriver bdrv_nbd_tcp = {
@@ -624,6 +636,7 @@ static BlockDriver bdrv_nbd_tcp = {
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_get_info = nbd_get_info,
.bdrv_dirname = nbd_dirname,
+ .sgfnt_runtime_opts = nbd_sgfnt_runtime_opts,
};
static BlockDriver bdrv_nbd_unix = {
@@ -645,6 +658,7 @@ static BlockDriver bdrv_nbd_unix = {
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_get_info = nbd_get_info,
.bdrv_dirname = nbd_dirname,
+ .sgfnt_runtime_opts = nbd_sgfnt_runtime_opts,
};
static void bdrv_nbd_init(void)
diff --git a/block/nfs.c b/block/nfs.c
index ee1160980a..298ba7e320 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -896,6 +896,15 @@ static void nfs_invalidate_cache(BlockDriverState *bs,
}
#endif
+static const char *nfs_sgfnt_runtime_opts[] = {
+ "path",
+ "user",
+ "group",
+ "server.",
+
+ NULL
+};
+
static BlockDriver bdrv_nfs = {
.format_name = "nfs",
.protocol_name = "nfs",
@@ -922,6 +931,8 @@ static BlockDriver bdrv_nfs = {
.bdrv_refresh_filename = nfs_refresh_filename,
.bdrv_dirname = nfs_dirname,
+ .sgfnt_runtime_opts = nfs_sgfnt_runtime_opts,
+
#ifdef LIBNFS_FEATURE_PAGECACHE
.bdrv_invalidate_cache = nfs_invalidate_cache,
#endif
diff --git a/block/null.c b/block/null.c
index 0cdabaa440..0fcd2671c8 100644
--- a/block/null.c
+++ b/block/null.c
@@ -254,6 +254,13 @@ static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
bs->full_open_options = opts;
}
+static const char *const null_sgfnt_runtime_opts[] = {
+ BLOCK_OPT_SIZE,
+ NULL_OPT_ZEROES,
+
+ NULL
+};
+
static BlockDriver bdrv_null_co = {
.format_name = "null-co",
.protocol_name = "null-co",
@@ -272,6 +279,7 @@ static BlockDriver bdrv_null_co = {
.bdrv_co_get_block_status = null_co_get_block_status,
.bdrv_refresh_filename = null_refresh_filename,
+ .sgfnt_runtime_opts = null_sgfnt_runtime_opts,
};
static BlockDriver bdrv_null_aio = {
@@ -292,6 +300,7 @@ static BlockDriver bdrv_null_aio = {
.bdrv_co_get_block_status = null_co_get_block_status,
.bdrv_refresh_filename = null_refresh_filename,
+ .sgfnt_runtime_opts = null_sgfnt_runtime_opts,
};
static void bdrv_null_init(void)
diff --git a/block/qcow.c b/block/qcow.c
index d552a6eba8..1b8c7378db 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -1113,6 +1113,12 @@ static QemuOptsList qcow_create_opts = {
}
};
+static const char *const qcow_sgfnt_runtime_opts[] = {
+ "encrypt." BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET,
+
+ NULL
+};
+
static BlockDriver bdrv_qcow = {
.format_name = "qcow",
.instance_size = sizeof(BDRVQcowState),
@@ -1134,6 +1140,7 @@ static BlockDriver bdrv_qcow = {
.bdrv_get_info = qcow_get_info,
.create_opts = &qcow_create_opts,
+ .sgfnt_runtime_opts = qcow_sgfnt_runtime_opts,
};
static void bdrv_qcow_init(void)
diff --git a/block/qcow2.c b/block/qcow2.c
index 1f80961e1b..c61933aa2e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4315,6 +4315,12 @@ static QemuOptsList qcow2_create_opts = {
}
};
+static const char *const qcow2_sgfnt_runtime_opts[] = {
+ "encrypt." BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET,
+
+ NULL
+};
+
BlockDriver bdrv_qcow2 = {
.format_name = "qcow2",
.instance_size = sizeof(BDRVQcow2State),
@@ -4360,6 +4366,7 @@ BlockDriver bdrv_qcow2 = {
.bdrv_inactivate = qcow2_inactivate,
.create_opts = &qcow2_create_opts,
+ .sgfnt_runtime_opts = qcow2_sgfnt_runtime_opts,
.bdrv_check = qcow2_check,
.bdrv_amend_options = qcow2_amend_options,
diff --git a/block/quorum.c b/block/quorum.c
index e5a844335e..4b38201aa2 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1105,6 +1105,15 @@ static char *quorum_dirname(BlockDriverState *bs, Error **errp)
return NULL;
}
+static const char *const quorum_sgfnt_runtime_opts[] = {
+ QUORUM_OPT_VOTE_THRESHOLD,
+ QUORUM_OPT_BLKVERIFY,
+ QUORUM_OPT_REWRITE,
+ QUORUM_OPT_READ_PATTERN,
+
+ NULL
+};
+
static BlockDriver bdrv_quorum = {
.format_name = "quorum",
.protocol_name = "quorum",
@@ -1130,6 +1139,8 @@ static BlockDriver bdrv_quorum = {
.is_filter = true,
.bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter,
+
+ .sgfnt_runtime_opts = quorum_sgfnt_runtime_opts,
};
static void bdrv_quorum_init(void)
diff --git a/block/raw-format.c b/block/raw-format.c
index ab552c0954..f09b707b0d 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -481,6 +481,13 @@ static int raw_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
return bdrv_probe_geometry(bs->file->bs, geo);
}
+static const char *const raw_sgfnt_runtime_opts[] = {
+ "offset",
+ "size",
+
+ NULL
+};
+
BlockDriver bdrv_raw = {
.format_name = "raw",
.instance_size = sizeof(BDRVRawState),
@@ -509,7 +516,8 @@ BlockDriver bdrv_raw = {
.bdrv_lock_medium = &raw_lock_medium,
.bdrv_co_ioctl = &raw_co_ioctl,
.create_opts = &raw_create_opts,
- .bdrv_has_zero_init = &raw_has_zero_init
+ .bdrv_has_zero_init = &raw_has_zero_init,
+ .sgfnt_runtime_opts = raw_sgfnt_runtime_opts,
};
static void bdrv_raw_init(void)
diff --git a/block/rbd.c b/block/rbd.c
index a76a5e8755..389f006bea 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1122,6 +1122,18 @@ static QemuOptsList qemu_rbd_create_opts = {
}
};
+static const char *const qemu_rbd_sgfnt_runtime_opts[] = {
+ "pool",
+ "image",
+ "conf",
+ "snapshot",
+ "user",
+ "server.",
+ "password-secret",
+
+ NULL
+};
+
static BlockDriver bdrv_rbd = {
.format_name = "rbd",
.instance_size = sizeof(BDRVRBDState),
@@ -1157,6 +1169,8 @@ static BlockDriver bdrv_rbd = {
#ifdef LIBRBD_SUPPORTS_INVALIDATE
.bdrv_invalidate_cache = qemu_rbd_invalidate_cache,
#endif
+
+ .sgfnt_runtime_opts = qemu_rbd_sgfnt_runtime_opts,
};
static void bdrv_rbd_init(void)
diff --git a/block/replication.c b/block/replication.c
index b1ea3caa4b..12f57c9676 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -701,6 +701,13 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
aio_context_release(aio_context);
}
+static const char *const replication_sgfnt_runtime_opts[] = {
+ REPLICATION_MODE,
+ REPLICATION_TOP_ID,
+
+ NULL
+};
+
BlockDriver bdrv_replication = {
.format_name = "replication",
.protocol_name = "replication",
@@ -718,6 +725,7 @@ BlockDriver bdrv_replication = {
.bdrv_recurse_is_first_non_filter = replication_recurse_is_first_non_filter,
.has_variable_length = true,
+ .sgfnt_runtime_opts = replication_sgfnt_runtime_opts,
};
static void bdrv_replication_init(void)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index f684477328..2425a0775b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -3076,6 +3076,15 @@ static QemuOptsList sd_create_opts = {
}
};
+static const char *const sd_sgfnt_runtime_opts[] = {
+ "vdi",
+ "snap-id",
+ "tag",
+ "server.",
+
+ NULL
+};
+
static BlockDriver bdrv_sheepdog = {
.format_name = "sheepdog",
.protocol_name = "sheepdog",
@@ -3110,6 +3119,7 @@ static BlockDriver bdrv_sheepdog = {
.bdrv_attach_aio_context = sd_attach_aio_context,
.create_opts = &sd_create_opts,
+ .sgfnt_runtime_opts = sd_sgfnt_runtime_opts,
};
static BlockDriver bdrv_sheepdog_tcp = {
@@ -3146,6 +3156,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
.bdrv_attach_aio_context = sd_attach_aio_context,
.create_opts = &sd_create_opts,
+ .sgfnt_runtime_opts = sd_sgfnt_runtime_opts,
};
static BlockDriver bdrv_sheepdog_unix = {
@@ -3182,6 +3193,7 @@ static BlockDriver bdrv_sheepdog_unix = {
.bdrv_attach_aio_context = sd_attach_aio_context,
.create_opts = &sd_create_opts,
+ .sgfnt_runtime_opts = sd_sgfnt_runtime_opts,
};
static void bdrv_sheepdog_init(void)
diff --git a/block/ssh.c b/block/ssh.c
index 8890a0c4ba..905fc44142 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -1196,6 +1196,17 @@ static int64_t ssh_getlength(BlockDriverState *bs)
return length;
}
+static const char *const ssh_sgfnt_runtime_opts[] = {
+ "host",
+ "port",
+ "path",
+ "user",
+ "host_key_check",
+ "server.",
+
+ NULL
+};
+
static BlockDriver bdrv_ssh = {
.format_name = "ssh",
.protocol_name = "ssh",
@@ -1210,6 +1221,7 @@ static BlockDriver bdrv_ssh = {
.bdrv_getlength = ssh_getlength,
.bdrv_co_flush_to_disk = ssh_co_flush,
.create_opts = &ssh_create_opts,
+ .sgfnt_runtime_opts = ssh_sgfnt_runtime_opts,
};
static void bdrv_ssh_init(void)
diff --git a/block/throttle.c b/block/throttle.c
index 833175ac77..efdc57616b 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -212,6 +212,12 @@ static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs)
atomic_dec(&tgm->io_limits_disabled);
}
+static const char *const throttle_sgfnt_runtime_opts[] = {
+ QEMU_OPT_THROTTLE_GROUP_NAME,
+
+ NULL
+};
+
static BlockDriver bdrv_throttle = {
.format_name = "throttle",
.protocol_name = "throttle",
@@ -245,6 +251,7 @@ static BlockDriver bdrv_throttle = {
.bdrv_co_drain_end = throttle_co_drain_end,
.is_filter = true,
+ .sgfnt_runtime_opts = throttle_sgfnt_runtime_opts,
};
static void bdrv_throttle_init(void)
diff --git a/block/vpc.c b/block/vpc.c
index 1576d7b595..a0a30b6e33 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1084,6 +1084,12 @@ static QemuOptsList vpc_create_opts = {
}
};
+static const char *const vpc_sgfnt_runtime_opts[] = {
+ VPC_OPT_SIZE_CALC,
+
+ NULL
+};
+
static BlockDriver bdrv_vpc = {
.format_name = "vpc",
.instance_size = sizeof(BDRVVPCState),
@@ -1103,6 +1109,7 @@ static BlockDriver bdrv_vpc = {
.create_opts = &vpc_create_opts,
.bdrv_has_zero_init = vpc_has_zero_init,
+ .sgfnt_runtime_opts = vpc_sgfnt_runtime_opts,
};
static void bdrv_vpc_init(void)
diff --git a/block/vvfat.c b/block/vvfat.c
index a690595f2c..76a91f97cb 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3242,6 +3242,16 @@ static void vvfat_close(BlockDriverState *bs)
}
}
+static const char *const vvfat_sgfnt_runtime_opts[] = {
+ "dir",
+ "fat-type",
+ "floppy",
+ "label",
+ "rw",
+
+ NULL
+};
+
static BlockDriver bdrv_vvfat = {
.format_name = "vvfat",
.protocol_name = "fat",
@@ -3256,6 +3266,8 @@ static BlockDriver bdrv_vvfat = {
.bdrv_co_preadv = vvfat_co_preadv,
.bdrv_co_pwritev = vvfat_co_pwritev,
.bdrv_co_get_block_status = vvfat_co_get_block_status,
+
+ .sgfnt_runtime_opts = vvfat_sgfnt_runtime_opts,
};
static void bdrv_vvfat_init(void)
diff --git a/block/vxhs.c b/block/vxhs.c
index 75cc6c8672..4d6dc5fe4c 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -555,6 +555,16 @@ static int64_t vxhs_getlength(BlockDriverState *bs)
return vdisk_size;
}
+static const char *const vxhs_sgfnt_runtime_opts[] = {
+ VXHS_OPT_VDISK_ID,
+ "tls-creds",
+ VXHS_OPT_HOST,
+ VXHS_OPT_PORT,
+ VXHS_OPT_SERVER".",
+
+ NULL
+};
+
static BlockDriver bdrv_vxhs = {
.format_name = "vxhs",
.protocol_name = "vxhs",
@@ -565,6 +575,7 @@ static BlockDriver bdrv_vxhs = {
.bdrv_getlength = vxhs_getlength,
.bdrv_aio_readv = vxhs_aio_readv,
.bdrv_aio_writev = vxhs_aio_writev,
+ .sgfnt_runtime_opts = vxhs_sgfnt_runtime_opts,
};
static void bdrv_vxhs_init(void)
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 19/26] block: Add BlockDriver.bdrv_gather_child_options
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (17 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 20/26] block: Generically refresh runtime options Max Reitz
` (6 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Some follow-up patches will rework the way bs->full_open_options is
refreshed in bdrv_refresh_filename(). The new implementation will remove
the need for the block drivers' bdrv_refresh_filename() implementations
to set bs->full_open_options; instead, it will be generic and use static
information from each block driver.
However, by implementing bdrv_gather_child_options(), block drivers will
still be able to override the way the full_open_options of their
children are incorporated into their own.
We need to implement this function for VMDK because we have to prevent
the generic implementation from gathering the options of all children:
It is not possible to specify options for the extents through the
runtime options.
For quorum, the child names that would be used by the generic
implementation and the ones that we actually (currently) want to use
differ. See quorum_gather_child_options() for more information.
Note that both of these are cases which are not ideal: In case of VMDK
it would probably be nice to be able to specify options for all extents.
In case of quorum, the current runtime option structure is simply broken
and needs to be fixed (but that is left for another patch).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block_int.h | 17 +++++++++++++++++
block/quorum.c | 38 ++++++++++++++++++++++++++++++++++++++
block/vmdk.c | 13 +++++++++++++
3 files changed, 68 insertions(+)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9e6800d42e..5e0bfe70d4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -135,6 +135,23 @@ struct BlockDriver {
void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
+ /*
+ * Gathers the open options for all children into @target.
+ * A simple format driver (without backing file support) might
+ * implement this function like this:
+ *
+ * QINCREF(bs->file->bs->full_open_options);
+ * qdict_put(target, "file", bs->file->bs->full_open_options);
+ *
+ * If not specified, the generic implementation will simply put
+ * all children's options under their respective name.
+ *
+ * Note that ideally this function would not be needed. Every
+ * block driver which implements it is probably doing something
+ * shady regarding its runtime option structure.
+ */
+ void (*bdrv_gather_child_options)(BlockDriverState *bs, QDict *target);
+
/*
* Returns an allocated string which is the directory name of this BDS: It
* will be used to make relative filenames absolute by prepending this
diff --git a/block/quorum.c b/block/quorum.c
index 4b38201aa2..9d680addc6 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1095,6 +1095,43 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
bs->full_open_options = opts;
}
+static void quorum_gather_child_options(BlockDriverState *bs, QDict *target)
+{
+ BDRVQuorumState *s = bs->opaque;
+ QList *children_list;
+ int i;
+
+ /* The generic implementation for gathering child options in
+ * bdrv_refresh_filename() would use the names of the children
+ * as specified for bdrv_open_child() or bdrv_attach_child(),
+ * which is "children.%u" with %u being a value
+ * (s->next_child_index) that is incremented each time a new child
+ * is added (and never decremented). Since children can be
+ * deleted at runtime, there may be gaps in that enumeration.
+ * When creating a new quorum BDS and specifying the children for
+ * it through runtime options, the enumeration used there may not
+ * have any gaps, though.
+ *
+ * Therefore, we have to create a new gap-less enumeration here
+ * (which we can achieve by simply putting all of the children's
+ * full_open_options into a QList).
+ *
+ * XXX: Note that there are issues with the current child option
+ * structure quorum uses (such as the fact that children do
+ * not really have unique permanent names). Therefore, this
+ * is going to have to change in the future and ideally we
+ * want quorum to be covered by the generic implementation.
+ */
+
+ children_list = qlist_new();
+ qdict_put(target, "children", children_list);
+
+ for (i = 0; i < s->num_children; i++) {
+ QINCREF(s->children[i]->bs->full_open_options);
+ qlist_append(children_list, s->children[i]->bs->full_open_options);
+ }
+}
+
static char *quorum_dirname(BlockDriverState *bs, Error **errp)
{
/* In general, there are multiple BDSs with different dirnames below this
@@ -1123,6 +1160,7 @@ static BlockDriver bdrv_quorum = {
.bdrv_file_open = quorum_open,
.bdrv_close = quorum_close,
.bdrv_refresh_filename = quorum_refresh_filename,
+ .bdrv_gather_child_options = quorum_gather_child_options,
.bdrv_dirname = quorum_dirname,
.bdrv_co_flush_to_disk = quorum_co_flush,
diff --git a/block/vmdk.c b/block/vmdk.c
index e61d15bc92..caa6131712 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2343,6 +2343,18 @@ static int vmdk_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return 0;
}
+static void vmdk_gather_child_options(BlockDriverState *bs, QDict *target)
+{
+ /* No children but file and backing can be explicitly specified (TODO) */
+ QINCREF(bs->file->bs->full_open_options);
+ qdict_put(target, "file", bs->file->bs->full_open_options);
+
+ if (bs->backing && bs->backing_overridden) {
+ QINCREF(bs->backing->bs->full_open_options);
+ qdict_put(target, "backing", bs->backing->bs->full_open_options);
+ }
+}
+
static QemuOptsList vmdk_create_opts = {
.name = "vmdk-create-opts",
.head = QTAILQ_HEAD_INITIALIZER(vmdk_create_opts.head),
@@ -2413,6 +2425,7 @@ static BlockDriver bdrv_vmdk = {
.bdrv_get_specific_info = vmdk_get_specific_info,
.bdrv_refresh_limits = vmdk_refresh_limits,
.bdrv_get_info = vmdk_get_info,
+ .bdrv_gather_child_options = vmdk_gather_child_options,
.supports_backing = true,
.create_opts = &vmdk_create_opts,
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 20/26] block: Generically refresh runtime options
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (18 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 19/26] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-06 14:03 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 21/26] block: Purify .bdrv_refresh_filename() Max Reitz
` (5 subsequent siblings)
25 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Instead of having every block driver which implements
bdrv_refresh_filename() copy all of the significant runtime options over
to bs->full_open_options, implement this process generically in
bdrv_refresh_filename().
This patch only adds this new generic implementation, it does not remove
the old functionality. This is done in a follow-up patch.
With this patch, some superfluous information (that should never have
been there) may be removed from some JSON filenames, as can be seen in
the change to iotest 110's reference output. In case of 191, backing
nodes that have not been overridden are now removed from the filename.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 116 ++++++++++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/110.out | 2 +-
tests/qemu-iotests/191.out | 24 +++++-----
3 files changed, 128 insertions(+), 14 deletions(-)
diff --git a/block.c b/block.c
index f23b35405d..7980090d25 100644
--- a/block.c
+++ b/block.c
@@ -4999,6 +4999,92 @@ out:
return to_replace_bs;
}
+/**
+ * Iterates through the list of runtime option keys that are said to be
+ * "significant" for a BDS. An option is called "significant" if it changes a
+ * BDS's data. For example, the null block driver's "size" and "read-zeroes"
+ * options are significant, but its "latency-ns" option is not.
+ *
+ * If a key returned by this function ends with a dot, all options starting with
+ * that prefix are significant.
+ */
+static const char *const *significant_options(BlockDriverState *bs,
+ const char *const *curopt)
+{
+ static const char *const global_options[] = {
+ "driver", "filename", NULL
+ };
+
+ if (!curopt) {
+ return &global_options[0];
+ }
+
+ curopt++;
+ if (curopt == &global_options[ARRAY_SIZE(global_options) - 1] && bs->drv) {
+ curopt = bs->drv->sgfnt_runtime_opts;
+ }
+
+ return (curopt && *curopt) ? curopt : NULL;
+}
+
+/**
+ * Copies all significant runtime options from bs->options to the given QDict.
+ * The set of significant option keys is determined by invoking
+ * significant_options().
+ *
+ * Returns true iff any significant option was present in bs->options (and thus
+ * copied to the target QDict) with the exception of "filename" and "driver".
+ * The caller is expected to use this value to decide whether the existence of
+ * significant options prevents the generation of a plain filename.
+ */
+static bool append_significant_runtime_options(QDict *d, BlockDriverState *bs)
+{
+ bool found_any = false;
+ const char *const *option_name = NULL;
+
+ if (!bs->drv) {
+ return false;
+ }
+
+ while ((option_name = significant_options(bs, option_name))) {
+ bool option_given = false;
+
+ assert(strlen(*option_name) > 0);
+ if ((*option_name)[strlen(*option_name) - 1] != '.') {
+ QObject *entry = qdict_get(bs->options, *option_name);
+ if (!entry) {
+ continue;
+ }
+
+ qobject_incref(entry);
+ qdict_put_obj(d, *option_name, entry);
+ option_given = true;
+ } else {
+ const QDictEntry *entry;
+ for (entry = qdict_first(bs->options); entry;
+ entry = qdict_next(bs->options, entry))
+ {
+ if (strstart(qdict_entry_key(entry), *option_name, NULL)) {
+ qobject_incref(qdict_entry_value(entry));
+ qdict_put_obj(d, qdict_entry_key(entry),
+ qdict_entry_value(entry));
+ option_given = true;
+ }
+ }
+ }
+
+ /* While "driver" and "filename" need to be included in a JSON filename,
+ * their existence does not prohibit generation of a plain filename. */
+ if (!found_any && option_given &&
+ strcmp(*option_name, "driver") && strcmp(*option_name, "filename"))
+ {
+ found_any = true;
+ }
+ }
+
+ return found_any;
+}
+
static bool append_open_options(QDict *d, BlockDriverState *bs)
{
const QDictEntry *entry;
@@ -5153,9 +5239,37 @@ void bdrv_refresh_filename(BlockDriverState *bs)
bs->full_open_options = opts;
}
+ /* Gather the options QDict */
+ opts = qdict_new();
+ append_significant_runtime_options(opts, bs);
+
+ if (drv->bdrv_gather_child_options) {
+ /* Some block drivers may not want to present all of their children's
+ * options, or name them differently from BdrvChild.name */
+ drv->bdrv_gather_child_options(bs, opts);
+ } else {
+ QLIST_FOREACH(child, &bs->children, next) {
+ if (child->role == &child_backing && !bs->backing_overridden) {
+ /* We can skip the backing BDS if it has not been overridden */
+ continue;
+ }
+
+ QINCREF(child->bs->full_open_options);
+ qdict_put(opts, child->name, child->bs->full_open_options);
+ }
+
+ if (bs->backing_overridden && !bs->backing) {
+ /* Force no backing file */
+ qdict_put(opts, "backing", qstring_new());
+ }
+ }
+
+ QDECREF(bs->full_open_options);
+ bs->full_open_options = opts;
+
if (bs->exact_filename[0]) {
pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
- } else if (bs->full_open_options) {
+ } else {
QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
snprintf(bs->filename, sizeof(bs->filename), "json:%s",
qstring_get_str(json));
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index 1d0b2475cc..46e6a60510 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -22,7 +22,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=t.IMGFMT.b
=== Nodes without a common directory ===
-image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "blkverify": false, "rewrite-corrupted": false, "vote-threshold": 1}}
+image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "vote-threshold": 1}}
file format: IMGFMT
virtual size: 64M (67108864 bytes)
backing file: t.IMGFMT.base (cannot determine actual path)
diff --git a/tests/qemu-iotests/191.out b/tests/qemu-iotests/191.out
index b22471b40a..627ae51106 100644
--- a/tests/qemu-iotests/191.out
+++ b/tests/qemu-iotests/191.out
@@ -52,7 +52,7 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
+ "filename": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
@@ -77,7 +77,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
+ "file": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"encryption_key_missing": false
},
{
@@ -123,7 +123,7 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}",
+ "filename": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
@@ -148,7 +148,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}",
+ "file": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}",
"encryption_key_missing": false
},
{
@@ -392,7 +392,7 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
+ "filename": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
@@ -417,7 +417,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
+ "file": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"encryption_key_missing": false
},
{
@@ -464,7 +464,7 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
+ "filename": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
@@ -474,12 +474,12 @@ wrote 65536/65536 bytes at offset 1048576
},
"backing-filename-format": "IMGFMT",
"virtual-size": 67108864,
- "filename": "json:{"backing": {"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl3"}}",
+ "filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl3"}}",
"cluster-size": 65536,
"format": "IMGFMT",
"actual-size": SIZE,
- "full-backing-filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
- "backing-filename": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
+ "full-backing-filename": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
+ "backing-filename": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"dirty-flag": false
},
"iops_wr": 0,
@@ -490,7 +490,7 @@ wrote 65536/65536 bytes at offset 1048576
"iops": 0,
"bps_wr": 0,
"write_threshold": 0,
- "backing_file": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
+ "backing_file": "json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}",
"encrypted": false,
"bps": 0,
"bps_rd": 0,
@@ -499,7 +499,7 @@ wrote 65536/65536 bytes at offset 1048576
"direct": false,
"writeback": true
},
- "file": "json:{"backing": {"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl3"}}",
+ "file": "json:{"backing": {"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.mid"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl2"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.ovl3"}}",
"encryption_key_missing": false
},
{
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 21/26] block: Purify .bdrv_refresh_filename()
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (19 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 20/26] block: Generically refresh runtime options Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-06 14:50 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 22/26] block: Do not copy exact_filename from format file Max Reitz
` (4 subsequent siblings)
25 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Currently, BlockDriver.bdrv_refresh_filename() is supposed to both
refresh the filename (BDS.exact_filename) and set BDS.full_open_options.
Now that we have generic code in the central bdrv_refresh_filename() for
creating BDS.full_open_options, we can drop the latter part from all
BlockDriver.bdrv_refresh_filename() implementations.
This also means that we can drop all of the existing default code for
this from the global bdrv_refresh_filename() itself.
Furthermore, we now have to call BlockDriver.bdrv_refresh_filename()
after having set BDS.full_open_options, because the block driver's
implementation should now be allowed to depend on BDS.full_open_options
being set correctly.
Finally, with this patch we can drop the @options parameter from
BlockDriver.bdrv_refresh_filename(); also, add a comment on this
function's purpose in block/block_int.h while touching its interface.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/block/block_int.h | 6 +-
block.c | 145 +++++++---------------------------------------
block/blkdebug.c | 55 +++++++-----------
block/blkverify.c | 16 +----
block/commit.c | 2 +-
block/mirror.c | 2 +-
block/nbd.c | 23 +-------
block/nfs.c | 36 +-----------
block/null.c | 23 +++++---
block/quorum.c | 30 ----------
10 files changed, 66 insertions(+), 272 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 5e0bfe70d4..178adde654 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -133,7 +133,11 @@ struct BlockDriver {
int (*bdrv_create)(const char *filename, QemuOpts *opts, Error **errp);
int (*bdrv_make_empty)(BlockDriverState *bs);
- void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
+ /*
+ * Refreshes the bs->exact_filename field. If that is impossible,
+ * bs->exact_filename has to be left empty.
+ */
+ void (*bdrv_refresh_filename)(BlockDriverState *bs);
/*
* Gathers the open options for all children into @target.
diff --git a/block.c b/block.c
index 7980090d25..2963cbc921 100644
--- a/block.c
+++ b/block.c
@@ -5085,47 +5085,6 @@ static bool append_significant_runtime_options(QDict *d, BlockDriverState *bs)
return found_any;
}
-static bool append_open_options(QDict *d, BlockDriverState *bs)
-{
- const QDictEntry *entry;
- QemuOptDesc *desc;
- BdrvChild *child;
- bool found_any = false;
- const char *p;
-
- for (entry = qdict_first(bs->options); entry;
- entry = qdict_next(bs->options, entry))
- {
- /* Exclude options for children */
- QLIST_FOREACH(child, &bs->children, next) {
- if (strstart(qdict_entry_key(entry), child->name, &p)
- && (!*p || *p == '.'))
- {
- break;
- }
- }
- if (child) {
- continue;
- }
-
- /* And exclude all non-driver-specific options */
- for (desc = bdrv_runtime_opts.desc; desc->name; desc++) {
- if (!strcmp(qdict_entry_key(entry), desc->name)) {
- break;
- }
- }
- if (desc->name) {
- continue;
- }
-
- qobject_incref(qdict_entry_value(entry));
- qdict_put_obj(d, qdict_entry_key(entry), qdict_entry_value(entry));
- found_any = true;
- }
-
- return found_any;
-}
-
/* Updates the following BDS fields:
* - exact_filename: A filename which may be used for opening a block device
* which (mostly) equals the given BDS (even without any
@@ -5143,6 +5102,8 @@ void bdrv_refresh_filename(BlockDriverState *bs)
BlockDriver *drv = bs->drv;
BdrvChild *child;
QDict *opts;
+ bool generate_json_filename; /* Whether our default implementation should
+ fill exact_filename (false) or not (true) */
if (!drv) {
return;
@@ -5158,90 +5119,10 @@ void bdrv_refresh_filename(BlockDriverState *bs)
}
}
- if (drv->bdrv_refresh_filename) {
- /* Obsolete information is of no use here, so drop the old file name
- * information before refreshing it */
- bs->exact_filename[0] = '\0';
- if (bs->full_open_options) {
- QDECREF(bs->full_open_options);
- bs->full_open_options = NULL;
- }
-
- opts = qdict_new();
- append_open_options(opts, bs);
- drv->bdrv_refresh_filename(bs, opts);
- QDECREF(opts);
- } else if (bs->file) {
- /* Try to reconstruct valid information from the underlying file */
- bool has_open_options;
-
- bs->exact_filename[0] = '\0';
- if (bs->full_open_options) {
- QDECREF(bs->full_open_options);
- bs->full_open_options = NULL;
- }
-
- 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 */
- if (bs->file->bs->exact_filename[0] && !has_open_options) {
- strcpy(bs->exact_filename, bs->file->bs->exact_filename);
- }
- /* Reconstructing the full options QDict is simple for most format block
- * drivers, as long as the full options are known for the underlying
- * 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 &&
- (!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) {
- qdict_put(opts, "backing", qstring_new());
- }
-
- bs->full_open_options = opts;
- } else {
- QDECREF(opts);
- }
- } else if (!bs->full_open_options && qdict_size(bs->options)) {
- /* There is no underlying file BDS (at least referenced by BDS.file),
- * so the full options QDict should be equal to the options given
- * specifically for this block device when it was opened (plus the
- * driver specification).
- * Because those options don't change, there is no need to update
- * full_open_options when it's already set. */
-
- opts = qdict_new();
- append_open_options(opts, bs);
- qdict_put_str(opts, "driver", drv->format_name);
-
- if (bs->exact_filename[0]) {
- /* This may not work for all block protocol drivers (some may
- * require this filename to be parsed), but we have to find some
- * default solution here, so just include it. If some block driver
- * does not support pure options without any filename at all or
- * needs some special format of the options QDict, it needs to
- * implement the driver-specific bdrv_refresh_filename() function.
- */
- qdict_put_str(opts, "filename", bs->exact_filename);
- }
-
- bs->full_open_options = opts;
- }
-
/* Gather the options QDict */
opts = qdict_new();
- append_significant_runtime_options(opts, bs);
+ generate_json_filename = append_significant_runtime_options(opts, bs);
+ generate_json_filename |= bs->backing_overridden;
if (drv->bdrv_gather_child_options) {
/* Some block drivers may not want to present all of their children's
@@ -5267,6 +5148,24 @@ void bdrv_refresh_filename(BlockDriverState *bs)
QDECREF(bs->full_open_options);
bs->full_open_options = opts;
+ if (drv->bdrv_refresh_filename) {
+ /* Obsolete information is of no use here, so drop the old file name
+ * information before refreshing it */
+ bs->exact_filename[0] = '\0';
+
+ drv->bdrv_refresh_filename(bs);
+ } else if (bs->file) {
+ /* Try to reconstruct valid information from the underlying file */
+
+ bs->exact_filename[0] = '\0';
+
+ /* If no specific options have been given for this BDS, the filename of
+ * the underlying file should suffice for this one as well */
+ if (bs->file->bs->exact_filename[0] && !generate_json_filename) {
+ strcpy(bs->exact_filename, bs->file->bs->exact_filename);
+ }
+ }
+
if (bs->exact_filename[0]) {
pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
} else {
diff --git a/block/blkdebug.c b/block/blkdebug.c
index f93139da58..7bd0e8d968 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -808,52 +808,37 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
return bdrv_getlength(bs->file->bs);
}
-static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
+static void blkdebug_refresh_filename(BlockDriverState *bs)
{
BDRVBlkdebugState *s = bs->opaque;
- QDict *opts;
const QDictEntry *e;
- bool force_json = false;
-
- for (e = qdict_first(options); e; e = qdict_next(options, e)) {
- if (strcmp(qdict_entry_key(e), "config") &&
- strcmp(qdict_entry_key(e), "x-image"))
- {
- force_json = true;
- break;
- }
- }
+ int ret;
- if (force_json && !bs->file->bs->full_open_options) {
- /* The config file cannot be recreated, so creating a plain filename
- * is impossible */
+ if (!bs->file->bs->exact_filename[0]) {
return;
}
- if (!force_json && bs->file->bs->exact_filename[0]) {
- int ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
- "blkdebug:%s:%s", s->config_file ?: "",
- bs->file->bs->exact_filename);
- if (ret >= sizeof(bs->exact_filename)) {
- /* An overflow makes the filename unusable, so do not report any */
- bs->exact_filename[0] = 0;
+ for (e = qdict_first(bs->full_open_options); e;
+ e = qdict_next(bs->full_open_options, e))
+ {
+ /* Real child options are under "image", but "x-image" may
+ * contain a filename */
+ if (strcmp(qdict_entry_key(e), "config") &&
+ strcmp(qdict_entry_key(e), "image") &&
+ strcmp(qdict_entry_key(e), "x-image") &&
+ strcmp(qdict_entry_key(e), "driver"))
+ {
+ return;
}
}
- opts = qdict_new();
- qdict_put_str(opts, "driver", "blkdebug");
-
- QINCREF(bs->file->bs->full_open_options);
- qdict_put(opts, "image", bs->file->bs->full_open_options);
-
- for (e = qdict_first(options); e; e = qdict_next(options, e)) {
- if (strcmp(qdict_entry_key(e), "x-image")) {
- qobject_incref(qdict_entry_value(e));
- qdict_put_obj(opts, qdict_entry_key(e), qdict_entry_value(e));
- }
+ ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+ "blkdebug:%s:%s",
+ s->config_file ?: "", bs->file->bs->exact_filename);
+ if (ret >= sizeof(bs->exact_filename)) {
+ /* An overflow makes the filename unusable, so do not report any */
+ bs->exact_filename[0] = 0;
}
-
- bs->full_open_options = opts;
}
static void blkdebug_refresh_limits(BlockDriverState *bs, Error **errp)
diff --git a/block/blkverify.c b/block/blkverify.c
index d5233eeaf9..7da2e08a06 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -277,24 +277,10 @@ static bool blkverify_recurse_is_first_non_filter(BlockDriverState *bs,
return bdrv_recurse_is_first_non_filter(s->test_file->bs, candidate);
}
-static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options)
+static void blkverify_refresh_filename(BlockDriverState *bs)
{
BDRVBlkverifyState *s = bs->opaque;
- if (bs->file->bs->full_open_options
- && s->test_file->bs->full_open_options)
- {
- QDict *opts = qdict_new();
- qdict_put_str(opts, "driver", "blkverify");
-
- QINCREF(bs->file->bs->full_open_options);
- qdict_put(opts, "raw", bs->file->bs->full_open_options);
- QINCREF(s->test_file->bs->full_open_options);
- qdict_put(opts, "test", s->test_file->bs->full_open_options);
-
- bs->full_open_options = opts;
- }
-
if (bs->file->bs->exact_filename[0]
&& s->test_file->bs->exact_filename[0])
{
diff --git a/block/commit.c b/block/commit.c
index 1fa6c9c0e0..1270b2290b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -239,7 +239,7 @@ static int coroutine_fn bdrv_commit_top_preadv(BlockDriverState *bs,
return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
}
-static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts)
+static void bdrv_commit_top_refresh_filename(BlockDriverState *bs)
{
pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
bs->backing->bs->filename);
diff --git a/block/mirror.c b/block/mirror.c
index aa68531352..1e3b40c2b8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1058,7 +1058,7 @@ static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
return bdrv_co_pdiscard(bs->backing->bs, offset, bytes);
}
-static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
+static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
{
if (bs->backing == NULL) {
/* we can be here after failed bdrv_attach_child in
diff --git a/block/nbd.c b/block/nbd.c
index 44a60f59bd..9b74894efb 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -515,12 +515,9 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
nbd_client_attach_aio_context(bs, new_context);
}
-static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
+static void nbd_refresh_filename(BlockDriverState *bs)
{
BDRVNBDState *s = bs->opaque;
- QDict *opts = qdict_new();
- QObject *saddr_qdict;
- Visitor *ov;
const char *host = NULL, *port = NULL, *path = NULL;
if (s->saddr->type == SOCKET_ADDRESS_TYPE_INET) {
@@ -533,8 +530,6 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
path = s->saddr->u.q_unix.path;
} /* else can't represent as pseudo-filename */
- qdict_put_str(opts, "driver", "nbd");
-
if (path && s->export) {
snprintf(bs->exact_filename, sizeof(bs->exact_filename),
"nbd+unix:///%s?socket=%s", s->export, path);
@@ -548,22 +543,6 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
snprintf(bs->exact_filename, sizeof(bs->exact_filename),
"nbd://%s:%s", host, port);
}
-
- ov = qobject_output_visitor_new(&saddr_qdict);
- visit_type_SocketAddress(ov, NULL, &s->saddr, &error_abort);
- visit_complete(ov, &saddr_qdict);
- visit_free(ov);
- qdict_put_obj(opts, "server", saddr_qdict);
-
- if (s->export) {
- qdict_put_str(opts, "export", s->export);
- }
- if (s->tlscredsid) {
- qdict_put_str(opts, "tls-creds", s->tlscredsid);
- }
-
- qdict_flatten(opts);
- bs->full_open_options = opts;
}
static int nbd_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
diff --git a/block/nfs.c b/block/nfs.c
index 298ba7e320..6a9783be39 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -818,14 +818,9 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
return 0;
}
-static void nfs_refresh_filename(BlockDriverState *bs, QDict *options)
+static void nfs_refresh_filename(BlockDriverState *bs)
{
NFSClient *client = bs->opaque;
- QDict *opts = qdict_new();
- QObject *server_qdict;
- Visitor *ov;
-
- qdict_put_str(opts, "driver", "nfs");
if (client->uid && !client->gid) {
snprintf(bs->exact_filename, sizeof(bs->exact_filename),
@@ -843,35 +838,6 @@ static void nfs_refresh_filename(BlockDriverState *bs, QDict *options)
snprintf(bs->exact_filename, sizeof(bs->exact_filename),
"nfs://%s%s", client->server->host, client->path);
}
-
- ov = qobject_output_visitor_new(&server_qdict);
- visit_type_NFSServer(ov, NULL, &client->server, &error_abort);
- visit_complete(ov, &server_qdict);
- qdict_put_obj(opts, "server", server_qdict);
- qdict_put_str(opts, "path", client->path);
-
- if (client->uid) {
- qdict_put_int(opts, "user", client->uid);
- }
- if (client->gid) {
- qdict_put_int(opts, "group", client->gid);
- }
- if (client->tcp_syncnt) {
- qdict_put_int(opts, "tcp-syn-cnt", client->tcp_syncnt);
- }
- if (client->readahead) {
- qdict_put_int(opts, "readahead-size", client->readahead);
- }
- if (client->pagecache) {
- qdict_put_int(opts, "page-cache-size", client->pagecache);
- }
- if (client->debug) {
- qdict_put_int(opts, "debug", client->debug);
- }
-
- visit_free(ov);
- qdict_flatten(opts);
- bs->full_open_options = opts;
}
static char *nfs_dirname(BlockDriverState *bs, Error **errp)
diff --git a/block/null.c b/block/null.c
index 0fcd2671c8..cca149ce9c 100644
--- a/block/null.c
+++ b/block/null.c
@@ -240,18 +240,23 @@ static int64_t coroutine_fn null_co_get_block_status(BlockDriverState *bs,
}
}
-static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
+static void null_refresh_filename(BlockDriverState *bs)
{
- QINCREF(opts);
- qdict_del(opts, "filename");
-
- if (!qdict_size(opts)) {
- snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
- bs->drv->format_name);
+ const QDictEntry *e;
+
+ for (e = qdict_first(bs->full_open_options); e;
+ e = qdict_next(bs->full_open_options, e))
+ {
+ /* These options can be ignored */
+ if (strcmp(qdict_entry_key(e), "filename") &&
+ strcmp(qdict_entry_key(e), "driver"))
+ {
+ return;
+ }
}
- qdict_put_str(opts, "driver", bs->drv->format_name);
- bs->full_open_options = opts;
+ snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
+ bs->drv->format_name);
}
static const char *const null_sgfnt_runtime_opts[] = {
diff --git a/block/quorum.c b/block/quorum.c
index 9d680addc6..7e8cccd60b 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1066,35 +1066,6 @@ static void quorum_del_child(BlockDriverState *bs, BdrvChild *child,
bdrv_drained_end(bs);
}
-static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
-{
- BDRVQuorumState *s = bs->opaque;
- QDict *opts;
- QList *children;
- int i;
-
- for (i = 0; i < s->num_children; i++) {
- if (!s->children[i]->bs->full_open_options) {
- return;
- }
- }
-
- children = qlist_new();
- for (i = 0; i < s->num_children; i++) {
- QINCREF(s->children[i]->bs->full_open_options);
- qlist_append(children, s->children[i]->bs->full_open_options);
- }
-
- opts = qdict_new();
- qdict_put_str(opts, "driver", "quorum");
- qdict_put_int(opts, QUORUM_OPT_VOTE_THRESHOLD, s->threshold);
- qdict_put_bool(opts, QUORUM_OPT_BLKVERIFY, s->is_blkverify);
- qdict_put_bool(opts, QUORUM_OPT_REWRITE, s->rewrite_corrupted);
- qdict_put(opts, "children", children);
-
- bs->full_open_options = opts;
-}
-
static void quorum_gather_child_options(BlockDriverState *bs, QDict *target)
{
BDRVQuorumState *s = bs->opaque;
@@ -1159,7 +1130,6 @@ static BlockDriver bdrv_quorum = {
.bdrv_file_open = quorum_open,
.bdrv_close = quorum_close,
- .bdrv_refresh_filename = quorum_refresh_filename,
.bdrv_gather_child_options = quorum_gather_child_options,
.bdrv_dirname = quorum_dirname,
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 22/26] block: Do not copy exact_filename from format file
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (20 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 21/26] block: Purify .bdrv_refresh_filename() Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 23/26] block: Fix FIXME from "Add BDS.backing_overridden" Max Reitz
` (3 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
If a format BDS's file BDS is in turn a format BDS, we cannot simply use
the same filename, because when opening a BDS tree based on a filename
alone, qemu will create only one format node on top of one protocol node
(disregarding a potential backing file).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 2963cbc921..664f87bad4 100644
--- a/block.c
+++ b/block.c
@@ -5159,9 +5159,20 @@ void bdrv_refresh_filename(BlockDriverState *bs)
bs->exact_filename[0] = '\0';
- /* If no specific options have been given for this BDS, the filename of
- * the underlying file should suffice for this one as well */
- if (bs->file->bs->exact_filename[0] && !generate_json_filename) {
+ /* We can use the underlying file's filename if:
+ * - it has a filename,
+ * - the file is a protocol BDS, and
+ * - opening that file (as this BDS's format) will automatically create
+ * the BDS tree we have right now, that is:
+ * - the user did not significantly change this BDS's behavior with
+ * some explicit options
+ * - no non-file child of this BDS has been overridden by the user
+ * Both of these conditions are represented by generate_json_filename.
+ */
+ if (bs->file->bs->exact_filename[0] &&
+ bs->file->bs->drv->bdrv_file_open &&
+ !generate_json_filename)
+ {
strcpy(bs->exact_filename, bs->file->bs->exact_filename);
}
}
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 23/26] block: Fix FIXME from "Add BDS.backing_overridden"
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (21 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 22/26] block: Do not copy exact_filename from format file Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 24/26] block/curl: Harmonize option defaults Max Reitz
` (2 subsequent siblings)
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Said commit introduced a FIXME stating that bdrv_open_backing_file()
should set bs->backing_overridden to true not only if the file.filename
option was set, but if the "options" QDict contained any option that is
significant for any node in the BDS tree emerging from the backing BDS.
This behavior is implemented by this patch.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 75 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index 664f87bad4..640015e1f7 100644
--- a/block.c
+++ b/block.c
@@ -73,6 +73,9 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
const BdrvChildRole *child_role,
Error **errp);
+static bool has_significant_runtime_options(BlockDriverState *bs,
+ const QDict *d);
+
/* If non-zero, use only whitelisted block drivers */
static int use_bdrv_whitelist;
@@ -2249,6 +2252,42 @@ out:
bdrv_refresh_limits(bs, NULL);
}
+/**
+ * Checks whether @options contains any significant option for any of the nodes
+ * in the BDS tree emerging from @bs.
+ */
+static bool is_significant_option_tree(BlockDriverState *bs, QDict *options)
+{
+ BdrvChild *child;
+
+ if (!qdict_size(options)) {
+ /* No need to recurse */
+ return false;
+ }
+
+ if (has_significant_runtime_options(bs, options)) {
+ return true;
+ }
+
+ QLIST_FOREACH(child, &bs->children, next) {
+ QDict *child_options;
+ char *option_prefix;
+
+ option_prefix = g_strdup_printf("%s.", child->name);
+ qdict_extract_subqdict(options, &child_options, option_prefix);
+ g_free(option_prefix);
+
+ if (is_significant_option_tree(child->bs, child_options)) {
+ QDECREF(child_options);
+ return true;
+ }
+
+ QDECREF(child_options);
+ }
+
+ return false;
+}
+
/*
* Opens the backing file for a BlockDriverState if not yet open
*
@@ -2267,7 +2306,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *reference = NULL;
int ret = 0;
BlockDriverState *backing_hd;
- QDict *options;
+ QDict *options, *cloned_options = NULL;
QDict *tmp_parent_options = NULL;
Error *local_err = NULL;
@@ -2297,11 +2336,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
reference = qdict_get_try_str(parent_options, bdref_key);
if (reference || qdict_haskey(options, "file.filename")) {
/* keep backing_filename NULL */
-
- /* FIXME: Should also be set to true if @options contains other runtime
- * options which control the data that is read from the backing
- * BDS */
- bs->backing_overridden = true;
} else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
QDECREF(options);
goto free_exit;
@@ -2322,6 +2356,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
goto free_exit;
}
+ cloned_options = qdict_clone_shallow(options);
+
if (!reference &&
bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
qdict_put_str(options, "driver", bs->backing_format);
@@ -2337,6 +2373,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
}
bdrv_set_aio_context(backing_hd, bdrv_get_aio_context(bs));
+ if (reference || is_significant_option_tree(backing_hd, cloned_options)) {
+ bs->backing_overridden = true;
+ }
+
/* Hook up the backing file link; drop our reference, bs owns the
* backing_hd reference now */
bdrv_set_backing_hd(bs, backing_hd, &local_err);
@@ -2352,6 +2392,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
free_exit:
g_free(backing_filename);
QDECREF(tmp_parent_options);
+ QDECREF(cloned_options);
return ret;
}
@@ -5027,6 +5068,34 @@ static const char *const *significant_options(BlockDriverState *bs,
return (curopt && *curopt) ? curopt : NULL;
}
+/**
+ * Returns true if @d contains any options the block driver of @bs considers to
+ * be significant runtime options.
+ */
+static bool has_significant_runtime_options(BlockDriverState *bs,
+ const QDict *d)
+{
+ const char *const *option_name = NULL;
+
+ while ((option_name = significant_options(bs, option_name))) {
+ assert(strlen(*option_name) > 0);
+ if ((*option_name)[strlen(*option_name) - 1] != '.') {
+ if (qdict_haskey(d, *option_name)) {
+ return true;
+ }
+ } else {
+ const QDictEntry *entry;
+ for (entry = qdict_first(d); entry; entry = qdict_next(d, entry)) {
+ if (strstart(qdict_entry_key(entry), *option_name, NULL)) {
+ return true;
+ }
+ }
+ }
+ }
+
+ return false;
+}
+
/**
* Copies all significant runtime options from bs->options to the given QDict.
* The set of significant option keys is determined by invoking
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 24/26] block/curl: Harmonize option defaults
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (22 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 23/26] block: Fix FIXME from "Add BDS.backing_overridden" Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-06 14:05 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 25/26] block/curl: Implement bdrv_refresh_filename() Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 26/26] block/null: Generate filename even with latency-ns Max Reitz
25 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Both of the defaults we currently have in the curl driver are named
based on a slightly different schema, let's unify that and call both
CURL_BLOCK_OPT_${NAME}_DEFAULT.
While at it, we can add a macro for the third option for which a default
exists, namely "sslverify".
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/curl.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 7c5418324f..4790c7dad5 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -72,8 +72,6 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
#define CURL_NUM_STATES 8
#define CURL_NUM_ACB 8
-#define READ_AHEAD_DEFAULT (256 * 1024)
-#define CURL_TIMEOUT_DEFAULT 5
#define CURL_TIMEOUT_MAX 10000
#define CURL_BLOCK_OPT_URL "url"
@@ -87,6 +85,10 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
#define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
+#define CURL_BLOCK_OPT_READAHEAD_DEFAULT (256 * 1024)
+#define CURL_BLOCK_OPT_SSLVERIFY_DEFAULT true
+#define CURL_BLOCK_OPT_TIMEOUT_DEFAULT 5
+
struct BDRVCURLState;
static bool libcurl_initialized;
@@ -714,7 +716,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
}
s->readahead_size = qemu_opt_get_size(opts, CURL_BLOCK_OPT_READAHEAD,
- READ_AHEAD_DEFAULT);
+ CURL_BLOCK_OPT_READAHEAD_DEFAULT);
if ((s->readahead_size & 0x1ff) != 0) {
error_setg(errp, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512",
s->readahead_size);
@@ -722,13 +724,14 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
}
s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
- CURL_TIMEOUT_DEFAULT);
+ CURL_BLOCK_OPT_TIMEOUT_DEFAULT);
if (s->timeout > CURL_TIMEOUT_MAX) {
error_setg(errp, "timeout parameter is too large or negative");
goto out_noclean;
}
- s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
+ s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY,
+ CURL_BLOCK_OPT_SSLVERIFY_DEFAULT);
cookie = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE);
cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 25/26] block/curl: Implement bdrv_refresh_filename()
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (23 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 24/26] block/curl: Harmonize option defaults Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
2018-02-06 14:07 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 26/26] block/null: Generate filename even with latency-ns Max Reitz
25 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/curl.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/block/curl.c b/block/curl.c
index 4790c7dad5..f4053e207c 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -972,6 +972,23 @@ static int64_t curl_getlength(BlockDriverState *bs)
return s->len;
}
+static void curl_refresh_filename(BlockDriverState *bs)
+{
+ BDRVCURLState *s = bs->opaque;
+
+ /* "readahead" and "timeout" do not change the guest-visible data,
+ * so ignore them */
+ if (s->sslverify != CURL_BLOCK_OPT_SSLVERIFY_DEFAULT ||
+ s->cookie || s->username || s->password || s->proxyusername ||
+ s->proxypassword)
+ {
+ return;
+ }
+
+ pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), s->url);
+}
+
+
static const char *const curl_sgfnt_runtime_opts[] = {
CURL_BLOCK_OPT_URL,
CURL_BLOCK_OPT_SSLVERIFY,
@@ -1000,6 +1017,7 @@ static BlockDriver bdrv_http = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+ .bdrv_refresh_filename = curl_refresh_filename,
.sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
};
@@ -1018,6 +1036,7 @@ static BlockDriver bdrv_https = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+ .bdrv_refresh_filename = curl_refresh_filename,
.sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
};
@@ -1036,6 +1055,7 @@ static BlockDriver bdrv_ftp = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+ .bdrv_refresh_filename = curl_refresh_filename,
.sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
};
@@ -1054,6 +1074,7 @@ static BlockDriver bdrv_ftps = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+ .bdrv_refresh_filename = curl_refresh_filename,
.sgfnt_runtime_opts = curl_sgfnt_runtime_opts,
};
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* [Qemu-devel] [PATCH v8 26/26] block/null: Generate filename even with latency-ns
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
` (24 preceding siblings ...)
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 25/26] block/curl: Implement bdrv_refresh_filename() Max Reitz
@ 2018-02-05 15:18 ` Max Reitz
25 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-05 15:18 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Alberto Garcia, Kevin Wolf
While we cannot represent the latency-ns option in a filename, it is not
a significant option so not being able to should not stop us from
generating a filename nonetheless.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/null.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/block/null.c b/block/null.c
index cca149ce9c..6ffed1c98e 100644
--- a/block/null.c
+++ b/block/null.c
@@ -249,7 +249,8 @@ static void null_refresh_filename(BlockDriverState *bs)
{
/* These options can be ignored */
if (strcmp(qdict_entry_key(e), "filename") &&
- strcmp(qdict_entry_key(e), "driver"))
+ strcmp(qdict_entry_key(e), "driver") &&
+ strcmp(qdict_entry_key(e), NULL_OPT_LATENCY))
{
return;
}
--
2.14.3
^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 04/26] iotests: Drop explicit base blockdev in 191
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 04/26] iotests: Drop explicit base blockdev in 191 Max Reitz
@ 2018-02-06 13:56 ` Alberto Garcia
2018-02-22 14:34 ` Kevin Wolf
1 sibling, 0 replies; 53+ messages in thread
From: Alberto Garcia @ 2018-02-06 13:56 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf
On Mon 05 Feb 2018 04:18:13 PM CET, Max Reitz wrote:
> Overriding the backing image should result in a json:{} pseudo-filename.
> Then, you can no longer use the commit block job with filename
> parameters. Therefore, do not explicitly add the base and override the
> middle image in iotest 191, since we do not need to anyway. This will
> allow us to continue to use the middle image's filename to identify it.
>
> In the long run, we want block-commit to accept node names for base and
> top (just like block-stream does).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 05/26] block: Respect backing bs in bdrv_refresh_filename
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 05/26] block: Respect backing bs in bdrv_refresh_filename Max Reitz
@ 2018-02-06 14:00 ` Alberto Garcia
0 siblings, 0 replies; 53+ messages in thread
From: Alberto Garcia @ 2018-02-06 14:00 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf
On Mon 05 Feb 2018 04:18:14 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 <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 20/26] block: Generically refresh runtime options
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 20/26] block: Generically refresh runtime options Max Reitz
@ 2018-02-06 14:03 ` Alberto Garcia
0 siblings, 0 replies; 53+ messages in thread
From: Alberto Garcia @ 2018-02-06 14:03 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf
On Mon 05 Feb 2018 04:18:29 PM CET, Max Reitz wrote:
> Instead of having every block driver which implements
> bdrv_refresh_filename() copy all of the significant runtime options over
> to bs->full_open_options, implement this process generically in
> bdrv_refresh_filename().
>
> This patch only adds this new generic implementation, it does not remove
> the old functionality. This is done in a follow-up patch.
>
> With this patch, some superfluous information (that should never have
> been there) may be removed from some JSON filenames, as can be seen in
> the change to iotest 110's reference output. In case of 191, backing
> nodes that have not been overridden are now removed from the filename.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 24/26] block/curl: Harmonize option defaults
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 24/26] block/curl: Harmonize option defaults Max Reitz
@ 2018-02-06 14:05 ` Alberto Garcia
0 siblings, 0 replies; 53+ messages in thread
From: Alberto Garcia @ 2018-02-06 14:05 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf
On Mon 05 Feb 2018 04:18:33 PM CET, Max Reitz wrote:
> Both of the defaults we currently have in the curl driver are named
> based on a slightly different schema, let's unify that and call both
> CURL_BLOCK_OPT_${NAME}_DEFAULT.
>
> While at it, we can add a macro for the third option for which a default
> exists, namely "sslverify".
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 25/26] block/curl: Implement bdrv_refresh_filename()
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 25/26] block/curl: Implement bdrv_refresh_filename() Max Reitz
@ 2018-02-06 14:07 ` Alberto Garcia
0 siblings, 0 replies; 53+ messages in thread
From: Alberto Garcia @ 2018-02-06 14:07 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf
On Mon 05 Feb 2018 04:18:34 PM CET, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 21/26] block: Purify .bdrv_refresh_filename()
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 21/26] block: Purify .bdrv_refresh_filename() Max Reitz
@ 2018-02-06 14:50 ` Alberto Garcia
0 siblings, 0 replies; 53+ messages in thread
From: Alberto Garcia @ 2018-02-06 14:50 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf
On Mon 05 Feb 2018 04:18:30 PM CET, Max Reitz wrote:
> Currently, BlockDriver.bdrv_refresh_filename() is supposed to both
> refresh the filename (BDS.exact_filename) and set BDS.full_open_options.
> Now that we have generic code in the central bdrv_refresh_filename() for
> creating BDS.full_open_options, we can drop the latter part from all
> BlockDriver.bdrv_refresh_filename() implementations.
>
> This also means that we can drop all of the existing default code for
> this from the global bdrv_refresh_filename() itself.
>
> Furthermore, we now have to call BlockDriver.bdrv_refresh_filename()
> after having set BDS.full_open_options, because the block driver's
> implementation should now be allowed to depend on BDS.full_open_options
> being set correctly.
>
> Finally, with this patch we can drop the @options parameter from
> BlockDriver.bdrv_refresh_filename(); also, add a comment on this
> function's purpose in block/block_int.h while touching its interface.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver Max Reitz
@ 2018-02-06 15:23 ` Alberto Garcia
2018-02-22 15:19 ` Max Reitz
2018-02-06 19:43 ` Eric Blake
1 sibling, 1 reply; 53+ messages in thread
From: Alberto Garcia @ 2018-02-06 15:23 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf
On Mon 05 Feb 2018 04:18:27 PM CET, Max Reitz wrote:
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -886,6 +886,21 @@ static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
> return 0;
> }
>
> +static const char *const blkdebug_sgfnt_runtime_opts[] = {
> + "config",
> + "inject-error.",
> + "set-state.",
> + "suspend.",
Where did this "suspend." come from?
Berto
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver Max Reitz
2018-02-06 15:23 ` Alberto Garcia
@ 2018-02-06 19:43 ` Eric Blake
2018-02-20 14:51 ` Max Reitz
1 sibling, 1 reply; 53+ messages in thread
From: Eric Blake @ 2018-02-06 19:43 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, Alberto Garcia, qemu-devel
On 02/05/2018 09:18 AM, Max Reitz wrote:
> This new field can be set by block drivers to list the runtime options
> they accept that may influence the contents of the respective BDS. As of
> a follow-up patch, this list will be used by the common
> bdrv_refresh_filename() implementation to decide which options to put
> into BDS.full_open_options (and consequently whether a JSON filename has
> to be created), thus freeing the drivers of having to implement that
> logic themselves.
>
> Additionally, this patch adds the field to all of the block drivers that
> need it and sets it accordingly.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> + /* Pointer to a NULL-terminated array of names of significant options that
> + * can be specified for bdrv_open(). A significant option is one that
> + * changes the data of a BDS.
> + * If this pointer is NULL, the array is considered empty.
> + * "filename" and "driver" are always considered significant. */
> + const char *const *sgfnt_runtime_opts;
Warning: Bikeshedding follows:
s/sgfnt/vital/
might read easier (same number of letters, but has some vowels rthr thn
bng crptc bbrvtn).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver
2018-02-06 19:43 ` Eric Blake
@ 2018-02-20 14:51 ` Max Reitz
2018-02-20 15:15 ` Eric Blake
0 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-20 14:51 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, Alberto Garcia, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]
On 2018-02-06 20:43, Eric Blake wrote:
> On 02/05/2018 09:18 AM, Max Reitz wrote:
>> This new field can be set by block drivers to list the runtime options
>> they accept that may influence the contents of the respective BDS. As of
>> a follow-up patch, this list will be used by the common
>> bdrv_refresh_filename() implementation to decide which options to put
>> into BDS.full_open_options (and consequently whether a JSON filename has
>> to be created), thus freeing the drivers of having to implement that
>> logic themselves.
>>
>> Additionally, this patch adds the field to all of the block drivers that
>> need it and sets it accordingly.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>
>> + /* Pointer to a NULL-terminated array of names of significant
>> options that
>> + * can be specified for bdrv_open(). A significant option is one
>> that
>> + * changes the data of a BDS.
>> + * If this pointer is NULL, the array is considered empty.
>> + * "filename" and "driver" are always considered significant. */
>> + const char *const *sgfnt_runtime_opts;
>
> Warning: Bikeshedding follows:
>
> s/sgfnt/vital/
>
> might read easier (same number of letters, but has some vowels rthr thn
> bng crptc bbrvtn).
So, did we reach any verdict on this? :-)
To sum up IRC (as far as I've understood), I don't quite like "vital" or
"needed" because they sound like those options would be required; but
they usually have defaults so they are not.
I preferred "strong", but you said that might mean something else
(although I still don't know what exactly :-)).
So...?
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver
2018-02-20 14:51 ` Max Reitz
@ 2018-02-20 15:15 ` Eric Blake
0 siblings, 0 replies; 53+ messages in thread
From: Eric Blake @ 2018-02-20 15:15 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, Alberto Garcia, qemu-devel
On 02/20/2018 08:51 AM, Max Reitz wrote:
> On 2018-02-06 20:43, Eric Blake wrote:
>> On 02/05/2018 09:18 AM, Max Reitz wrote:
>>> This new field can be set by block drivers to list the runtime options
>>> they accept that may influence the contents of the respective BDS. As of
>>> a follow-up patch, this list will be used by the common
>>> bdrv_refresh_filename() implementation to decide which options to put
>>> into BDS.full_open_options (and consequently whether a JSON filename has
>>> to be created), thus freeing the drivers of having to implement that
>>> logic themselves.
>>>
>>> Additionally, this patch adds the field to all of the block drivers that
>>> need it and sets it accordingly.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>
>>> + /* Pointer to a NULL-terminated array of names of significant
>>> options that
>>> + * can be specified for bdrv_open(). A significant option is one
>>> that
>>> + * changes the data of a BDS.
>>> + * If this pointer is NULL, the array is considered empty.
>>> + * "filename" and "driver" are always considered significant. */
>>> + const char *const *sgfnt_runtime_opts;
>>
>> Warning: Bikeshedding follows:
>>
>> s/sgfnt/vital/
>>
>> might read easier (same number of letters, but has some vowels rthr thn
>> bng crptc bbrvtn).
>
> So, did we reach any verdict on this? :-)
>
> To sum up IRC (as far as I've understood), I don't quite like "vital" or
> "needed" because they sound like those options would be required; but
> they usually have defaults so they are not.
>
> I preferred "strong", but you said that might mean something else
> (although I still don't know what exactly :-)).
>
> So...?
major? apt?
(the thesaurus mentions other words like meaningful or influential, but
those don't buy you any length reduction or easier abbreviations)
I don't recall what (if anything) I said against strong, so I'm also
fine if that's what you choose.
At the end of the day, it's all bikeshedding - pick a term, and I'll
live with it ;)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 01/26] block/mirror: Small absolute-paths simplification
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 01/26] block/mirror: Small absolute-paths simplification Max Reitz
@ 2018-02-22 12:27 ` Kevin Wolf
2018-02-22 14:43 ` Max Reitz
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2018-02-22 12:27 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Alberto Garcia
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
> When invoking drive-mirror in absolute-paths mode, the target's backing
> BDS is assigned to it in mirror_exit(). The current logic only does so
> if the target does not have that backing BDS already; but it actually
> cannot have a backing BDS at all (the BDS is opened with O_NO_BACKING in
> qmp_drive_mirror()), so just assert that and assign the new backing BDS
> unconditionally.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
Introducing new assumptions that the graph stays unmodified between the
start of a block job and its completion feels a bit adventurous when all
of the blockdev work is moving towards making things more flexible.
If we really do want to enforce this, I guess you finally found a use
case for BLK_PERM_GRAPH_MOD.
Kevin
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 02/26] block: Use children list in bdrv_refresh_filename
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 02/26] block: Use children list in bdrv_refresh_filename Max Reitz
@ 2018-02-22 12:30 ` Kevin Wolf
0 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2018-02-22 12:30 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Alberto Garcia
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
> bdrv_refresh_filename() should invoke itself recursively on all
> children, not just on file.
>
> With that change, we can remove the manual invocations in blkverify,
> quorum, commit, and mirror.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden Max Reitz
@ 2018-02-22 13:39 ` Kevin Wolf
2018-02-22 14:55 ` Max Reitz
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2018-02-22 13:39 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Alberto Garcia
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
> If the backing file is overridden, this most probably does change the
> guest-visible data of a BDS. Therefore, we will need to consider this in
> bdrv_refresh_filename().
>
> Adding a new field to the BDS is not nice, but it is very simple and
> exactly keeps track of whether the backing file has been overridden.
...as long as we manage to actually keep it up to date all the time.
First of all, what I'm missing here (or in fact in the comment in the
code) is a definition what "overridden" really means. "specified by the
user" is kind of vague: You consider the backing file relationship for
snapshot=on as user specified, even though the user wasn't explicit
about this. On the other hand, creating a live snapshot results in a
node that isn't user specified.
Isn't the real question to ask whether the default backing file (taken
from the image header) would result in the same tree? The answer to this
changes after more operations, like qmp_change_backing_file().
Considering that there are so many ways to change the answer, I think
the simplest reliable option isn't a new BDS field that needs to updated
everywhere, but looking at the current value of bs->options and
bs->backing_file and see if they match.
> This commit adds a FIXME which will be remedied by a follow-up commit.
> Until then, the respective piece of code will not result in any behavior
> that is worse than what we currently have.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
Kevin
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 04/26] iotests: Drop explicit base blockdev in 191
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 04/26] iotests: Drop explicit base blockdev in 191 Max Reitz
2018-02-06 13:56 ` Alberto Garcia
@ 2018-02-22 14:34 ` Kevin Wolf
2018-02-22 15:06 ` Max Reitz
1 sibling, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2018-02-22 14:34 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Alberto Garcia
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
> Overriding the backing image should result in a json:{} pseudo-filename.
> Then, you can no longer use the commit block job with filename
> parameters. Therefore, do not explicitly add the base and override the
> middle image in iotest 191, since we do not need to anyway. This will
> allow us to continue to use the middle image's filename to identify it.
>
> In the long run, we want block-commit to accept node names for base and
> top (just like block-stream does).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Is this actually needed once the FIXME in patch 3 is addressed and only
significant fields are considered, or can we revert the patch at the end
of the series?
Once we implement a node-named based blockdev-commit, we'll want to test
both versions here, so it would be good to be able to set a node-name
and still be able to use the filename for the older interface.
Kevin
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 01/26] block/mirror: Small absolute-paths simplification
2018-02-22 12:27 ` Kevin Wolf
@ 2018-02-22 14:43 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-22 14:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]
On 2018-02-22 13:27, Kevin Wolf wrote:
> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
>> When invoking drive-mirror in absolute-paths mode, the target's backing
>> BDS is assigned to it in mirror_exit(). The current logic only does so
>> if the target does not have that backing BDS already; but it actually
>> cannot have a backing BDS at all (the BDS is opened with O_NO_BACKING in
>> qmp_drive_mirror()), so just assert that and assign the new backing BDS
>> unconditionally.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>
> Introducing new assumptions that the graph stays unmodified between the
> start of a block job and its completion feels a bit adventurous when all
> of the blockdev work is moving towards making things more flexible.
But as the commit message hints at, MIRROR_SOURCE_BACKING_CHAIN is used
only by drive-mirror. How flexible do you intend that to be?
Yes, technically you can specify a node name for the newly created image
and then go wild while the block job is still running. But then
something inside me feels like you should see your VM crashing for
that... OTOH, a crash is always bad.
I can't make it crash in the current state because the mirror job
creates a BB and blockdev-snapshot thus refuses to attach a backing file
to the target. But I guess I'll drop this patch anyway (unless I find
it was important for some other patch in this series, which I hope it
wasn't...).
On a totally unrelated note, we need some protocol for hurting the user
if they do something that deserves a bit of pain. Crashing is always
bad and our fault, so it needs to be something else. Removing $HOME?
Marking all attached qcow2 images corrupt even though they aren't so you
have to run a qemu-img check? I like that one.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
2018-02-22 13:39 ` Kevin Wolf
@ 2018-02-22 14:55 ` Max Reitz
2018-02-22 15:12 ` Kevin Wolf
0 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-22 14:55 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 2729 bytes --]
On 2018-02-22 14:39, Kevin Wolf wrote:
> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
>> If the backing file is overridden, this most probably does change the
>> guest-visible data of a BDS. Therefore, we will need to consider this in
>> bdrv_refresh_filename().
>>
>> Adding a new field to the BDS is not nice, but it is very simple and
>> exactly keeps track of whether the backing file has been overridden.
>
> ...as long as we manage to actually keep it up to date all the time.
>
> First of all, what I'm missing here (or in fact in the comment in the
> code) is a definition what "overridden" really means. "specified by the
> user" is kind of vague: You consider the backing file relationship for
> snapshot=on as user specified, even though the user wasn't explicit
> about this. On the other hand, creating a live snapshot results in a
> node that isn't user specified.
>
> Isn't the real question to ask whether the default backing file (taken
> from the image header) would result in the same tree? The answer to this
> changes after more operations, like qmp_change_backing_file().
With you so far.
> Considering that there are so many ways to change the answer, I think
> the simplest reliable option isn't a new BDS field that needs to updated
> everywhere, but looking at the current value of bs->options and
> bs->backing_file and see if they match.
I don't see how that is simple. First, bs->options does not necessarily
reflect the "current options", those would be bs->full_open_options.
And for generating that, we need a way to determine whether the backing
file has been overridden or not, so whether we need to put the backing
options into it or whether we do not.
(I am right that bs->backing_file is what the image header says, right?
So we need to compare it against something that reflects the runtime state.)
What I could see would be comparing bs->backing_file to
bs->backing->bs->filename. But this sounds very hacky to me.
One thing the comes to mind is that it can break whenever
bdrv_refresh_filename() is clever. So you specify
'json:{"driver":"null-co"}' in the image header, and
bdrv_refresh_filename() optimizes that to "null-co://". Now the
filenames differ even though it's still the original filename. So this
wouldn't work very well either.
Max
>> This commit adds a FIXME which will be remedied by a follow-up commit.
>> Until then, the respective piece of code will not result in any behavior
>> that is worse than what we currently have.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>
> Kevin
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 06/26] block: Make path_combine() return the path
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 06/26] block: Make path_combine() return the path Max Reitz
@ 2018-02-22 14:57 ` Kevin Wolf
0 siblings, 0 replies; 53+ messages in thread
From: Kevin Wolf @ 2018-02-22 14:57 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Alberto Garcia
Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
> Besides being safe for arbitrary path lengths, after some follow-up
> patches all callers will want a freshly allocated buffer anyway.
>
> In the meantime, path_combine_deprecated() is added which has the same
> interface as path_combine() had before this patch. All callers to that
> function will be converted in follow-up patches.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 04/26] iotests: Drop explicit base blockdev in 191
2018-02-22 14:34 ` Kevin Wolf
@ 2018-02-22 15:06 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-02-22 15:06 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]
On 2018-02-22 15:34, Kevin Wolf wrote:
> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
>> Overriding the backing image should result in a json:{} pseudo-filename.
>> Then, you can no longer use the commit block job with filename
>> parameters. Therefore, do not explicitly add the base and override the
>> middle image in iotest 191, since we do not need to anyway. This will
>> allow us to continue to use the middle image's filename to identify it.
>>
>> In the long run, we want block-commit to accept node names for base and
>> top (just like block-stream does).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>
> Is this actually needed once the FIXME in patch 3 is addressed and only
> significant fields are considered, or can we revert the patch at the end
> of the series?
The empiric result is "no". :-)
In the series's current state, overriding the backing file always means
that it is being treated as overridden -- even if you override it with
itself and thus the resulting graph is basically the same.
I'm still open to suggestions whether we can do anything about this, but
the only thing that comes to my mind is:
(1) Open bs->backing_file as a BDS.
(2) Run bdrv_refresh_filename() on it (done automatically).
(3) Compare the result with bs->backing->bs->filename.
But you can't just open bs->backing_file twice (that is, have to BDS
that refer to the same file), except maybe with BDRV_O_NO_IO. Still,
that would be a huge hack. (And isn't guaranteed to work, even with
BDRV_O_NO_IO.)
So what we'd need is an off-line bdrv_refresh_filename() that constructs
a filename from filename+options, without a BDS. But honestly, I don't
want to write that just so that we can continue to do blockdev-commit
with filenames. I'd rather break it so we get node names there.
Max
> Once we implement a node-named based blockdev-commit, we'll want to test
> both versions here, so it would be good to be able to set a node-name
> and still be able to use the filename for the older interface.
>
> Kevin
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
2018-02-22 14:55 ` Max Reitz
@ 2018-02-22 15:12 ` Kevin Wolf
2018-02-22 15:17 ` Max Reitz
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2018-02-22 15:12 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]
Am 22.02.2018 um 15:55 hat Max Reitz geschrieben:
> On 2018-02-22 14:39, Kevin Wolf wrote:
> > Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
> >> If the backing file is overridden, this most probably does change the
> >> guest-visible data of a BDS. Therefore, we will need to consider this in
> >> bdrv_refresh_filename().
> >>
> >> Adding a new field to the BDS is not nice, but it is very simple and
> >> exactly keeps track of whether the backing file has been overridden.
> >
> > ...as long as we manage to actually keep it up to date all the time.
> >
> > First of all, what I'm missing here (or in fact in the comment in the
> > code) is a definition what "overridden" really means. "specified by the
> > user" is kind of vague: You consider the backing file relationship for
> > snapshot=on as user specified, even though the user wasn't explicit
> > about this. On the other hand, creating a live snapshot results in a
> > node that isn't user specified.
> >
> > Isn't the real question to ask whether the default backing file (taken
> > from the image header) would result in the same tree? The answer to this
> > changes after more operations, like qmp_change_backing_file().
>
> With you so far.
>
> > Considering that there are so many ways to change the answer, I think
> > the simplest reliable option isn't a new BDS field that needs to updated
> > everywhere, but looking at the current value of bs->options and
> > bs->backing_file and see if they match.
>
> I don't see how that is simple. First, bs->options does not necessarily
> reflect the "current options", those would be bs->full_open_options.
> And for generating that, we need a way to determine whether the backing
> file has been overridden or not, so whether we need to put the backing
> options into it or whether we do not.
For the purpose of this comparison, we need a set of options that
contains the backing file options unconditionally.
> (I am right that bs->backing_file is what the image header says, right?
> So we need to compare it against something that reflects the runtime state.)
I think so, yes.
> What I could see would be comparing bs->backing_file to
> bs->backing->bs->filename. But this sounds very hacky to me.
>
> One thing the comes to mind is that it can break whenever
> bdrv_refresh_filename() is clever. So you specify
> 'json:{"driver":"null-co"}' in the image header, and
> bdrv_refresh_filename() optimizes that to "null-co://". Now the
> filenames differ even though it's still the original filename. So this
> wouldn't work very well either.
On the other hand, the problem with your current approach is that it
results in a JSON filename even if you override the backing file and
specify the same file name as we already have in the image header.
In the future, libvirt is going to manually build the graph, so we will
always have the backing file overridden according to the logic in this
patch. I don't think we want to get JSON filenames for all libvirt
managed VMs, so can we realistically do without any kind of comparison?
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
2018-02-22 15:12 ` Kevin Wolf
@ 2018-02-22 15:17 ` Max Reitz
2018-02-22 16:21 ` Kevin Wolf
0 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-22 15:17 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]
On 2018-02-22 16:12, Kevin Wolf wrote:
> Am 22.02.2018 um 15:55 hat Max Reitz geschrieben:
>> On 2018-02-22 14:39, Kevin Wolf wrote:
>>> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
>>>> If the backing file is overridden, this most probably does change the
>>>> guest-visible data of a BDS. Therefore, we will need to consider this in
>>>> bdrv_refresh_filename().
>>>>
>>>> Adding a new field to the BDS is not nice, but it is very simple and
>>>> exactly keeps track of whether the backing file has been overridden.
>>>
>>> ...as long as we manage to actually keep it up to date all the time.
>>>
>>> First of all, what I'm missing here (or in fact in the comment in the
>>> code) is a definition what "overridden" really means. "specified by the
>>> user" is kind of vague: You consider the backing file relationship for
>>> snapshot=on as user specified, even though the user wasn't explicit
>>> about this. On the other hand, creating a live snapshot results in a
>>> node that isn't user specified.
>>>
>>> Isn't the real question to ask whether the default backing file (taken
>>> from the image header) would result in the same tree? The answer to this
>>> changes after more operations, like qmp_change_backing_file().
>>
>> With you so far.
>>
>>> Considering that there are so many ways to change the answer, I think
>>> the simplest reliable option isn't a new BDS field that needs to updated
>>> everywhere, but looking at the current value of bs->options and
>>> bs->backing_file and see if they match.
>>
>> I don't see how that is simple. First, bs->options does not necessarily
>> reflect the "current options", those would be bs->full_open_options.
>> And for generating that, we need a way to determine whether the backing
>> file has been overridden or not, so whether we need to put the backing
>> options into it or whether we do not.
>
> For the purpose of this comparison, we need a set of options that
> contains the backing file options unconditionally.
>
>> (I am right that bs->backing_file is what the image header says, right?
>> So we need to compare it against something that reflects the runtime state.)
>
> I think so, yes.
>
>> What I could see would be comparing bs->backing_file to
>> bs->backing->bs->filename. But this sounds very hacky to me.
>>
>> One thing the comes to mind is that it can break whenever
>> bdrv_refresh_filename() is clever. So you specify
>> 'json:{"driver":"null-co"}' in the image header, and
>> bdrv_refresh_filename() optimizes that to "null-co://". Now the
>> filenames differ even though it's still the original filename. So this
>> wouldn't work very well either.
>
> On the other hand, the problem with your current approach is that it
> results in a JSON filename even if you override the backing file and
> specify the same file name as we already have in the image header.
Yes.
> In the future, libvirt is going to manually build the graph, so we will
> always have the backing file overridden according to the logic in this
> patch. I don't think we want to get JSON filenames for all libvirt
> managed VMs, so can we realistically do without any kind of comparison?
libvirt doesn't need to query the filename, though, does it?
In my mind, we wanted to phase out filenames and basically only present
them as convenience/legacy information to users who use qemu directly.
I really don't see the point of burdening qemu with simplifying and
niceifying filenames when you want to use node names for everything anyway.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver
2018-02-06 15:23 ` Alberto Garcia
@ 2018-02-22 15:19 ` Max Reitz
2018-02-22 15:30 ` Alberto Garcia
0 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-22 15:19 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf
[-- Attachment #1: Type: text/plain, Size: 624 bytes --]
On 2018-02-06 16:23, Alberto Garcia wrote:
> On Mon 05 Feb 2018 04:18:27 PM CET, Max Reitz wrote:
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -886,6 +886,21 @@ static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
>> return 0;
>> }
>>
>> +static const char *const blkdebug_sgfnt_runtime_opts[] = {
>> + "config",
>> + "inject-error.",
>> + "set-state.",
>> + "suspend.",
>
> Where did this "suspend." come from?
Er, well, er.
After having looked at it a couple of times now, I can't see anything.
I guess I'll remove it and hope for the best...?
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver
2018-02-22 15:19 ` Max Reitz
@ 2018-02-22 15:30 ` Alberto Garcia
0 siblings, 0 replies; 53+ messages in thread
From: Alberto Garcia @ 2018-02-22 15:30 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf
On Thu 22 Feb 2018 04:19:45 PM CET, Max Reitz wrote:
> On 2018-02-06 16:23, Alberto Garcia wrote:
>> On Mon 05 Feb 2018 04:18:27 PM CET, Max Reitz wrote:
>>> --- a/block/blkdebug.c
>>> +++ b/block/blkdebug.c
>>> @@ -886,6 +886,21 @@ static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
>>> return 0;
>>> }
>>>
>>> +static const char *const blkdebug_sgfnt_runtime_opts[] = {
>>> + "config",
>>> + "inject-error.",
>>> + "set-state.",
>>> + "suspend.",
>>
>> Where did this "suspend." come from?
>
> Er, well, er.
>
> After having looked at it a couple of times now, I can't see anything.
> I guess I'll remove it and hope for the best...?
My interpretation is that you saw
enum {
ACTION_INJECT_ERROR,
ACTION_SET_STATE,
ACTION_SUSPEND,
};
and assumed that "suspend" was an option (the other two are). I haven't
seen any string with the "suspend" text anywhere else in the code.
Berto
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
2018-02-22 15:17 ` Max Reitz
@ 2018-02-22 16:21 ` Kevin Wolf
2018-02-22 17:47 ` Max Reitz
0 siblings, 1 reply; 53+ messages in thread
From: Kevin Wolf @ 2018-02-22 16:21 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 5307 bytes --]
Am 22.02.2018 um 16:17 hat Max Reitz geschrieben:
> On 2018-02-22 16:12, Kevin Wolf wrote:
> > Am 22.02.2018 um 15:55 hat Max Reitz geschrieben:
> >> On 2018-02-22 14:39, Kevin Wolf wrote:
> >>> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
> >>>> If the backing file is overridden, this most probably does change the
> >>>> guest-visible data of a BDS. Therefore, we will need to consider this in
> >>>> bdrv_refresh_filename().
> >>>>
> >>>> Adding a new field to the BDS is not nice, but it is very simple and
> >>>> exactly keeps track of whether the backing file has been overridden.
> >>>
> >>> ...as long as we manage to actually keep it up to date all the time.
> >>>
> >>> First of all, what I'm missing here (or in fact in the comment in the
> >>> code) is a definition what "overridden" really means. "specified by the
> >>> user" is kind of vague: You consider the backing file relationship for
> >>> snapshot=on as user specified, even though the user wasn't explicit
> >>> about this. On the other hand, creating a live snapshot results in a
> >>> node that isn't user specified.
> >>>
> >>> Isn't the real question to ask whether the default backing file (taken
> >>> from the image header) would result in the same tree? The answer to this
> >>> changes after more operations, like qmp_change_backing_file().
> >>
> >> With you so far.
> >>
> >>> Considering that there are so many ways to change the answer, I think
> >>> the simplest reliable option isn't a new BDS field that needs to updated
> >>> everywhere, but looking at the current value of bs->options and
> >>> bs->backing_file and see if they match.
> >>
> >> I don't see how that is simple. First, bs->options does not necessarily
> >> reflect the "current options", those would be bs->full_open_options.
> >> And for generating that, we need a way to determine whether the backing
> >> file has been overridden or not, so whether we need to put the backing
> >> options into it or whether we do not.
> >
> > For the purpose of this comparison, we need a set of options that
> > contains the backing file options unconditionally.
> >
> >> (I am right that bs->backing_file is what the image header says, right?
> >> So we need to compare it against something that reflects the runtime state.)
> >
> > I think so, yes.
> >
> >> What I could see would be comparing bs->backing_file to
> >> bs->backing->bs->filename. But this sounds very hacky to me.
> >>
> >> One thing the comes to mind is that it can break whenever
> >> bdrv_refresh_filename() is clever. So you specify
> >> 'json:{"driver":"null-co"}' in the image header, and
> >> bdrv_refresh_filename() optimizes that to "null-co://". Now the
> >> filenames differ even though it's still the original filename. So this
> >> wouldn't work very well either.
So what's the full effect here?
You example says that if you use an overcomplicated way to specify an
image (by using json: instead of an URL), you get back an
overcomplicated filename for the parent image (which includes the
backing file even though it's not really necessary). Sounds fair enough
to me.
Can bad things happen with absolute vs. relative paths?
> > On the other hand, the problem with your current approach is that it
> > results in a JSON filename even if you override the backing file and
> > specify the same file name as we already have in the image header.
>
> Yes.
>
> > In the future, libvirt is going to manually build the graph, so we will
> > always have the backing file overridden according to the logic in this
> > patch. I don't think we want to get JSON filenames for all libvirt
> > managed VMs, so can we realistically do without any kind of comparison?
>
> libvirt doesn't need to query the filename, though, does it?
I know that libvirt uses the output in qemu-img info. And I learnt about
that because they were surprised that json: filenames you get there
can't necessarily be fed to QMP (because they contain only strings).
Other than that, I hope they don't. I suppose the filename can end up in
error messages in logfiles, though.
> In my mind, we wanted to phase out filenames and basically only present
> them as convenience/legacy information to users who use qemu directly.
>
> I really don't see the point of burdening qemu with simplifying and
> niceifying filenames when you want to use node names for everything
> anyway.
But if you essentially say "filenames are only for those who don't use
advanced features", then why bother with overridden backing files?
There are two problems I have with this patch: The first is that it
introduces additional state that needs to be managed correctly in all
future patches that modify the graph, and the second (and worse one) is
that it fails to manage this state correctly even now.
I mentioned snapshots and change-backing-file that can result in a wrong
bs->backing_overridden, and those were only the obvious first places I
had a look at. Even if you fix them, I wouldn't trust my own review to
find all relevant places. And that's a really bad sign for a design.
This is the most important reason why I'm looking for some method to
derive the flag from already existing state.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
2018-02-22 16:21 ` Kevin Wolf
@ 2018-02-22 17:47 ` Max Reitz
2018-06-26 17:34 ` Max Reitz
0 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-02-22 17:47 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 8657 bytes --]
On 2018-02-22 17:21, Kevin Wolf wrote:
> Am 22.02.2018 um 16:17 hat Max Reitz geschrieben:
>> On 2018-02-22 16:12, Kevin Wolf wrote:
>>> Am 22.02.2018 um 15:55 hat Max Reitz geschrieben:
>>>> On 2018-02-22 14:39, Kevin Wolf wrote:
>>>>> Am 05.02.2018 um 16:18 hat Max Reitz geschrieben:
>>>>>> If the backing file is overridden, this most probably does change the
>>>>>> guest-visible data of a BDS. Therefore, we will need to consider this in
>>>>>> bdrv_refresh_filename().
>>>>>>
>>>>>> Adding a new field to the BDS is not nice, but it is very simple and
>>>>>> exactly keeps track of whether the backing file has been overridden.
>>>>>
>>>>> ...as long as we manage to actually keep it up to date all the time.
>>>>>
>>>>> First of all, what I'm missing here (or in fact in the comment in the
>>>>> code) is a definition what "overridden" really means. "specified by the
>>>>> user" is kind of vague: You consider the backing file relationship for
>>>>> snapshot=on as user specified, even though the user wasn't explicit
>>>>> about this. On the other hand, creating a live snapshot results in a
>>>>> node that isn't user specified.
>>>>>
>>>>> Isn't the real question to ask whether the default backing file (taken
>>>>> from the image header) would result in the same tree? The answer to this
>>>>> changes after more operations, like qmp_change_backing_file().
>>>>
>>>> With you so far.
>>>>
>>>>> Considering that there are so many ways to change the answer, I think
>>>>> the simplest reliable option isn't a new BDS field that needs to updated
>>>>> everywhere, but looking at the current value of bs->options and
>>>>> bs->backing_file and see if they match.
>>>>
>>>> I don't see how that is simple. First, bs->options does not necessarily
>>>> reflect the "current options", those would be bs->full_open_options.
>>>> And for generating that, we need a way to determine whether the backing
>>>> file has been overridden or not, so whether we need to put the backing
>>>> options into it or whether we do not.
>>>
>>> For the purpose of this comparison, we need a set of options that
>>> contains the backing file options unconditionally.
>>>
>>>> (I am right that bs->backing_file is what the image header says, right?
>>>> So we need to compare it against something that reflects the runtime state.)
>>>
>>> I think so, yes.
>>>
>>>> What I could see would be comparing bs->backing_file to
>>>> bs->backing->bs->filename. But this sounds very hacky to me.
>>>>
>>>> One thing the comes to mind is that it can break whenever
>>>> bdrv_refresh_filename() is clever. So you specify
>>>> 'json:{"driver":"null-co"}' in the image header, and
>>>> bdrv_refresh_filename() optimizes that to "null-co://". Now the
>>>> filenames differ even though it's still the original filename. So this
>>>> wouldn't work very well either.
>
> So what's the full effect here?
>
> You example says that if you use an overcomplicated way to specify an
> image (by using json: instead of an URL), you get back an
> overcomplicated filename for the parent image (which includes the
> backing file even though it's not really necessary). Sounds fair enough
> to me.
OK, but one issue is that you've used an overcomplicated way for the
backing file; but you get an overcomplicated filename for the overlay.
> Can bad things happen with absolute vs. relative paths?
"Can"? Absolutely. "Do"? I don't know? :-)
Another thing are non-unique URLs. For instance, nbd allows you to
specify "nbd:localhost:10809", but it will generate
"nbd://localhost:10809". (Same for just "nbd://localhost".)
Or of course the good old "file:foo.qcow2".
>>> On the other hand, the problem with your current approach is that it
>>> results in a JSON filename even if you override the backing file and
>>> specify the same file name as we already have in the image header.
>>
>> Yes.
>>
>>> In the future, libvirt is going to manually build the graph, so we will
>>> always have the backing file overridden according to the logic in this
>>> patch. I don't think we want to get JSON filenames for all libvirt
>>> managed VMs, so can we realistically do without any kind of comparison?
>>
>> libvirt doesn't need to query the filename, though, does it?
>
> I know that libvirt uses the output in qemu-img info. And I learnt about
> that because they were surprised that json: filenames you get there
> can't necessarily be fed to QMP (because they contain only strings).
>
> Other than that, I hope they don't. I suppose the filename can end up in
> error messages in logfiles, though.
Fair point. Although it isn't impossible to decrypt json:{} filenames,
it isn't very nice.
(And I suspect most error messages today actually contain the node name,
which is even less useful than json:{} filenames.)
>> In my mind, we wanted to phase out filenames and basically only present
>> them as convenience/legacy information to users who use qemu directly.
>>
>> I really don't see the point of burdening qemu with simplifying and
>> niceifying filenames when you want to use node names for everything
>> anyway.
>
> But if you essentially say "filenames are only for those who don't use
> advanced features", then why bother with overridden backing files?
Because giving overcomplicated information is better than giving plainly
wrong information.
If we return a filename, it has to be correct. Sure, we could just not
return filenames, but even then we would need to have a way of
recognizing when to do that.
But given backwards compatibility and all, I can't imagine a way where
we don't have to deal with the backing file issue in one way or another.
If you want to remove filenames from the internal state and for what
queries return (apart from bs->options for protocol nodes) in 3.0, sign
me up for it.
> There are two problems I have with this patch: The first is that it
> introduces additional state that needs to be managed correctly in all
> future patches that modify the graph, and the second (and worse one) is
> that it fails to manage this state correctly even now.
Well, agreed. I know there were a couple revisions already where I
fixed things.
But then again, I also seem to remember that I've had a discussion about
what to do here with you before, but it's been so long that I can't
remember the actual content.
> I mentioned snapshots and change-backing-file that can result in a wrong
> bs->backing_overridden, and those were only the obvious first places I
> had a look at. Even if you fix them, I wouldn't trust my own review to
> find all relevant places. And that's a really bad sign for a design.
>
> This is the most important reason why I'm looking for some method to
> derive the flag from already existing state.
The only things I can say to that are "I agree" and "I've tried before".
The thing is, I started working on this series in November 2015
(apparently), so I've completely forgotten what exactly it was that I
tried and I only know that my result was "I'll have to add this field".
I concede that result may be wrong, and I myself sure hope so.
So thee two things I see now are:
(1) Compare bs->backing_file against bs->backing->bs->filename. Seems
basically as error-prone as backing_overridden to me, probably not only
because bdrv_refresh_filename() may change .filename.
(2) Keep backing_overridden; but we'd need a central point for changing
bs->backing, i.e. two functions or one function with a boolean
parameter. That parameter tells it whether we want to change the
current backing file, independently of the file's metadata, or whether
we want to change the backing file in accordance with the file's
metadata. In the former case, we'd set backing_overridden, in the
latter, we'd clear it. This function would then be the only one allowed
to modifiy bs->backing. The issue here is that maybe it's still not as
simple as it sounds, and it is still possible to have backing_overridden
set even though the backing node is indeed what it would be without the
@backing option specified -- but I personally don't see a simple way to
support this.
(We could strcmp bs->backing_file and bs->backing->bs->filename and if
they match, we clear backing_overridden even though it is supposed to be
set. But that would mean inconsistent behavior (sometimes you're lucky,
sometimes you aren't), and I don't like that very much. I actually
prefer consistently undesirable behavior over inconsistently desirable
behavior.)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
2018-02-22 17:47 ` Max Reitz
@ 2018-06-26 17:34 ` Max Reitz
2018-06-26 18:19 ` Max Reitz
0 siblings, 1 reply; 53+ messages in thread
From: Max Reitz @ 2018-06-26 17:34 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]
So, while tackling this once more, this is the current state, in short.
I wanted to add a new field BDS.backing_file_canonical which basically
gets set to bs->backing->bs->filename whenever we know that bs->backing
was opened through bs->backing_file. That is,
bs->backing_file_canonical would be bs->backing_file, but after going
through the whole hoop of bdrv_open() and bdrv_refresh_filename().
With such a field, we could compare bs->backing->bs->filename against
bs->backing_file_canonical, and thus see whether the backing BDS matches
the image header filename. I hope this would do the right thing
whenever qemu tries to figure out backing filenames on its own; it will
break, though, when the user just uses some filename and that doesn't
happen to coincide with qemu's generated filenames.
(For instance, if you create a new overlay manually, give it some
backing filename, and that backing filename isn't 1:1 what qemu would
reconstruct; and then you open that file with backing=null, and do a
blockdev-snapshot; then qemu will give you a json:{} filename for the
overlay because due to backing=null, it didn't open the overlay's
backing_file and thus doesn't know how it would look reconstructed. But
I think that's something we could live with.)
((The only real way to fix this I can imagine is to clone the whole
bdrv_parse_filename()/bdrv_open()/bdrv_refresh_filename()
infrastructure, but just for filenames instead of real BDSs. Yeah, no,
I won't do that.))
OK, so that was an idea, but now it turns out that bs->backing_file
actually isn't that image header's idea of the backing file. It appears
to mostly be a clone of bs->backing->bs->filename, but not really.
Honestly, I have no idea what it is, but as a matter of fact, it is
modified by bdrv_backing_attach(), so it will probably usually match
bs->backing->bs->filename (unless there is no bs->backing, in which case
it just retains its old value?). So it pretty much is completely
useless for any comparison here.
So I suppose I'll rename my BDS.backing_file_canonical attempt to
BDS.auto_backing_file, and this will be what the image header contains
(unless we have opened a BDS from it, in which case it will be that
BDS's filename, so it is canonicalized).
Well, or I could just go with backing_overridden, because honestly that
didn't seem so bad.
Max
PS: Or, we could argue that nobody needs filenames anyway and that
they're just for show and debugging nowadays, so nobody actually needs
backing chain information in them. May sound a bit stupid, but then
again nobody has ever complained that that's in fact the current state.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden
2018-06-26 17:34 ` Max Reitz
@ 2018-06-26 18:19 ` Max Reitz
0 siblings, 0 replies; 53+ messages in thread
From: Max Reitz @ 2018-06-26 18:19 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 1606 bytes --]
On 2018-06-26 19:34, Max Reitz wrote:
[...]
> So I suppose I'll rename my BDS.backing_file_canonical attempt to
> BDS.auto_backing_file, and this will be what the image header contains
> (unless we have opened a BDS from it, in which case it will be that
> BDS's filename, so it is canonicalized).
Update: That seems to go down the drain. We (stupidly) allow taking a
backing filename from the image header and then enriching it with
options from the command line (so you could take null-co:// from the
image header and then the user could give backing.size. I won't comment
on how little sense that makes (apart from [1]), but in any case, this
means that "we have opened a BDS from it" is very hard to reliably
recognize, because you need to find out whether the user has given any
strong options for the backing file. That pretty much brings the
complexity back to backing_overridden, I'd say.
Well, or: Whenever the user has given any backing options, the resulting
BDS is categorized as overridden and the filename is not used as a
canonicalized version of auto_backing_file.
I now pretty much fail to see how any of this is better than
backing_overridden, but whatever.
Max
[1] In fact, it's completely broken, because the user-given backing
options only override the backing filename once the user-given options
contain "file.filename". So you actually cannot really override
anything unless your new backing file has a protocol driver with a
"filename" option. Well, you can, but that means creating an own BDS
and then giving a reference to that...
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 53+ messages in thread
end of thread, other threads:[~2018-06-26 18:19 UTC | newest]
Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-05 15:18 [Qemu-devel] [PATCH v8 00/26] block: Fix some filename generation issues Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 01/26] block/mirror: Small absolute-paths simplification Max Reitz
2018-02-22 12:27 ` Kevin Wolf
2018-02-22 14:43 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 02/26] block: Use children list in bdrv_refresh_filename Max Reitz
2018-02-22 12:30 ` Kevin Wolf
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 03/26] block: Add BDS.backing_overridden Max Reitz
2018-02-22 13:39 ` Kevin Wolf
2018-02-22 14:55 ` Max Reitz
2018-02-22 15:12 ` Kevin Wolf
2018-02-22 15:17 ` Max Reitz
2018-02-22 16:21 ` Kevin Wolf
2018-02-22 17:47 ` Max Reitz
2018-06-26 17:34 ` Max Reitz
2018-06-26 18:19 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 04/26] iotests: Drop explicit base blockdev in 191 Max Reitz
2018-02-06 13:56 ` Alberto Garcia
2018-02-22 14:34 ` Kevin Wolf
2018-02-22 15:06 ` Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 05/26] block: Respect backing bs in bdrv_refresh_filename Max Reitz
2018-02-06 14:00 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 06/26] block: Make path_combine() return the path Max Reitz
2018-02-22 14:57 ` Kevin Wolf
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 07/26] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 08/26] block: bdrv_get_full_backing_filename's " Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 09/26] block: Add bdrv_make_absolute_filename() Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 10/26] block: Fix bdrv_find_backing_image() Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 11/26] block: Add bdrv_dirname() Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 12/26] blkverify: Make bdrv_dirname() return NULL Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 13/26] quorum: " Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 14/26] block/nbd: " Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 15/26] block/nfs: Implement bdrv_dirname() Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 16/26] block: Use bdrv_dirname() for relative filenames Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 17/26] iotests: Add quorum case to test 110 Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 18/26] block: Add sgfnt_runtime_opts to BlockDriver Max Reitz
2018-02-06 15:23 ` Alberto Garcia
2018-02-22 15:19 ` Max Reitz
2018-02-22 15:30 ` Alberto Garcia
2018-02-06 19:43 ` Eric Blake
2018-02-20 14:51 ` Max Reitz
2018-02-20 15:15 ` Eric Blake
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 19/26] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 20/26] block: Generically refresh runtime options Max Reitz
2018-02-06 14:03 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 21/26] block: Purify .bdrv_refresh_filename() Max Reitz
2018-02-06 14:50 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 22/26] block: Do not copy exact_filename from format file Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 23/26] block: Fix FIXME from "Add BDS.backing_overridden" Max Reitz
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 24/26] block/curl: Harmonize option defaults Max Reitz
2018-02-06 14:05 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 25/26] block/curl: Implement bdrv_refresh_filename() Max Reitz
2018-02-06 14:07 ` Alberto Garcia
2018-02-05 15:18 ` [Qemu-devel] [PATCH v8 26/26] block/null: Generate filename even with latency-ns Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.