* [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues
@ 2017-09-29 16:53 Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 01/25] block/mirror: Small absolute-paths simplification Max Reitz
` (24 more replies)
0 siblings, 25 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
[If you have read the cover letter in x \in [v2, v5], there is nothing
new here; feel free to skip to the bottom to read the changes from v4.]
There are some issues regarding filename generation right now:
- You always get a JSON filename if you set even a single qcow2-specific
runtime options (as long as it does not have a dot in it, which is a
bug, too, but here it is working in our favor...). That is not nice
and actually breaks the usage of backing files with relative
filenames with such qcow2 BDS.
- As hinted above, you cannot use relative backing filenames with BDS
that have a JSON filename only, even though qemu might be able to
obtain the directory name by walking through the BDS graph to the
protocol level.
- Overriding the backing file at runtime should invalidate the filename
because it actually changes the BDS's data. Therefore, we need to
force a JSON filename in that case, containing the backing file
override.
- Much of our code assumes paths never to exceed PATH_MAX in length.
This is wrong, at least because of JSON filenames. This should be
fixed wherever the opportunity arises.
- If a driver decides to implement bdrv_refresh_filename(), that
implementation has to not only refresh the filename (as one would
think) but it must also refresh the runtime options
(bs->full_open_options). That is stupid. (I'm allowed to say that
because I'm to blame for it.)
This series is enclosed by four patches (two at the front, two at the
back) that fix more or less general issues. They are included because:
- Patch 1 is required so that in patch 3 it's obvious why we don't need
to set backing_overriden there or call bdrv_refresh_filename()
- Patch 2 is already reviewed, so I might just as well keep it.
- Patches 24 and 25 are basically general bug fixes. Their connection to
this series is obvious, however, I think, and they depend on the rest
of the series, so I decided to just put them in.
Patches 3 and 4 address the third issue above, and patch 23 adds
something that's missing from patch 3. It cannot be squashed into patch
3, however, because it depends on functionality introduced by patches 18
to 22.
Consequently, patch 3 introduces a FIXME that is resolved by patch 23.
Patches 5 to 9 address the fourth issue above, and are also necessary
preparation for the following patches.
Patches 10 to 16 address the second issue above, patch 17 adds a test
case. They implement a bdrv_dirname() function that returns the base
directory of a BDS by walking through the BDS graph to the protocol
layer and then trying to obtain a path based on that BDS's
exact_filename. This obviously fails if exact_filename on the protocol
layer is not set.
This behavior can be overriden either by any block driver along the way
implementing bdrv_dirname() itself or by the user through the new
'base-directory' node option. This may allow us to resolve relative
filenames even if the reference BDS only has a JSON filename.
Patches 18 to 22 address both the first and last issues above. They add
a field called "sgfnt_runtime_opts" to the BlockDriver structure. Block
drivers may point this to an array containing all of the runtime options
they accept that may change their BDS's data (i.e. that are
"significant"). bdrv_refresh_filename() will use this list to generate
bs->full_open_options itself (with only a little help by the block
driver, if necessary, through the .bdrv_gather_child_options()
function). This not only simplifies the process significantly, but also
results in the default implementation generating JSON filenames only
when really necessary.
v6: Rebased on my block branch, and:
- Patches 6, 7, 8, 15: Generally treat a NULL filename as no filename --
that means, we should usually accept NULL filenames in these
filename-handling functions and not return empty filenames but NULL
instead
- Patch 18: Added significant options for the throttle driver
git-backport-diff against v5:
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/25:[----] [--] 'block/mirror: Small absolute-paths simplification'
002/25:[----] [-C] 'block: Use children list in bdrv_refresh_filename'
003/25:[----] [--] 'block: Add BDS.backing_overridden'
004/25:[----] [--] 'block: Respect backing bs in bdrv_refresh_filename'
005/25:[----] [--] 'block: Make path_combine() return the path'
006/25:[0042] [FC] 'block: bdrv_get_full_backing_filename_from_...'s ret. val.'
007/25:[0007] [FC] 'block: bdrv_get_full_backing_filename's ret. val.'
008/25:[0012] [FC] 'block: Add bdrv_make_absolute_filename()'
009/25:[----] [--] 'block: Fix bdrv_find_backing_image()'
010/25:[----] [--] 'block: Add bdrv_dirname()'
011/25:[----] [--] 'blkverify: Make bdrv_dirname() return NULL'
012/25:[----] [--] 'quorum: Make bdrv_dirname() return NULL'
013/25:[----] [--] 'block/nbd: Make bdrv_dirname() return NULL'
014/25:[----] [--] 'block/nfs: Implement bdrv_dirname()'
015/25:[0010] [FC] 'block: Use bdrv_dirname() for relative filenames'
016/25:[----] [--] 'block: Add 'base-directory' BDS option'
017/25:[----] [--] 'iotests: Add quorum case to test 110'
018/25:[0004] [FC] 'block: Add sgfnt_runtime_opts to BlockDriver'
019/25:[----] [--] 'block: Add BlockDriver.bdrv_gather_child_options'
020/25:[----] [--] 'block: Generically refresh runtime options'
021/25:[----] [-C] 'block: Purify .bdrv_refresh_filename()'
022/25:[----] [--] 'block: Do not copy exact_filename from format file'
023/25:[----] [-C] 'block: Fix FIXME from "Add BDS.backing_overridden"'
024/25:[----] [--] 'block/curl: Implement bdrv_refresh_filename()'
025/25:[----] [--] 'block/null: Generate filename even with latency-ns'
Max Reitz (25):
block/mirror: Small absolute-paths simplification
block: Use children list in bdrv_refresh_filename
block: Add BDS.backing_overridden
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
block: Add 'base-directory' BDS option
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: Implement bdrv_refresh_filename()
block/null: Generate filename even with latency-ns
qapi/block-core.json | 9 +
include/block/block.h | 15 +-
include/block/block_int.h | 36 ++-
block.c | 519 +++++++++++++++++++++++++++++-------------
block/blkdebug.c | 58 ++---
block/blkverify.c | 29 +--
block/commit.c | 3 +-
block/crypto.c | 5 +
block/curl.c | 39 ++++
block/gluster.c | 19 ++
block/iscsi.c | 18 ++
block/mirror.c | 19 +-
block/nbd.c | 46 ++--
block/nfs.c | 46 ++--
block/null.c | 33 ++-
block/qapi.c | 12 +-
block/quorum.c | 58 +++--
block/rbd.c | 5 +
block/replication.c | 5 +
block/sheepdog.c | 12 +
block/ssh.c | 5 +
block/throttle.c | 4 +
block/vmdk.c | 25 +-
block/vpc.c | 4 +
block/vvfat.c | 4 +
block/vxhs.c | 8 +
blockdev.c | 16 ++
tests/qemu-iotests/051.out | 8 +-
tests/qemu-iotests/051.pc.out | 8 +-
tests/qemu-iotests/110 | 51 ++++-
tests/qemu-iotests/110.out | 14 +-
31 files changed, 789 insertions(+), 344 deletions(-)
--
2.13.6
^ permalink raw reply [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 01/25] block/mirror: Small absolute-paths simplification
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 02/25] block: Use children list in bdrv_refresh_filename Max Reitz
` (23 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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 351b80ca2c..25e9354b4d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -533,12 +533,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 02/25] block: Use children list in bdrv_refresh_filename
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 01/25] block/mirror: Small absolute-paths simplification Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 03/25] block: Add BDS.backing_overridden Max Reitz
` (22 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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 5c65fac672..5721168d1d 100644
--- a/block.c
+++ b/block.c
@@ -4877,16 +4877,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 8f0e83578a..1df8db5428 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -246,7 +246,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 25e9354b4d..7aa873d86f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1078,7 +1078,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 03/25] block: Add BDS.backing_overridden
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 01/25] block/mirror: Small absolute-paths simplification Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 02/25] block: Use children list in bdrv_refresh_filename Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 04/25] block: Respect backing bs in bdrv_refresh_filename Max Reitz
` (21 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
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 99abe2ce74..2b816ec601 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -603,6 +603,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 5721168d1d..95258316ec 100644
--- a/block.c
+++ b/block.c
@@ -2196,6 +2196,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;
@@ -2396,6 +2401,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);
@@ -2526,6 +2534,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");
}
@@ -4888,6 +4897,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 7aa873d86f..cac7490e04 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -540,6 +540,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 56a6b24a0b..b0393e1786 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1781,6 +1781,8 @@ static void external_snapshot_commit(BlkActionState *common)
{
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
+ TransactionAction *action = common->action;
+ bool image_was_existing = false;
/* We don't need (or want) to use the transactional
* bdrv_reopen_multiple() across all the entries at once, because we
@@ -1789,6 +1791,20 @@ static void external_snapshot_commit(BlkActionState *common)
bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
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);
+ }
}
static void external_snapshot_abort(BlkActionState *common)
--
2.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 04/25] block: Respect backing bs in bdrv_refresh_filename
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (2 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 03/25] block: Add BDS.backing_overridden Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 05/25] block: Make path_combine() return the path Max Reitz
` (20 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
Basically, bdrv_refresh_filename() should respect all children of a
BlockDriverState. However, generally those children are driver-specific,
so this function cannot handle the general case. On the other hand,
there are only few drivers which use other children than @file and
@backing (that being vmdk, quorum, and blkverify).
Most block drivers only use @file and/or @backing (if they use any
children at all). Both can be implemented directly in
bdrv_refresh_filename.
The user overriding the file's filename is already handled, however, the
user overriding the backing file is not. If this is done, opening the
BDS with the plain filename of its file will not be correct, so we may
not set bs->exact_filename in that case.
iotest 051 contains test cases for overwriting the backing file, and so
its output changes with this patch applied (which I consider a good
thing).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block.c | 12 +++++++++++-
tests/qemu-iotests/051.out | 8 ++++----
tests/qemu-iotests/051.pc.out | 8 ++++----
3 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index 95258316ec..ba24845960 100644
--- a/block.c
+++ b/block.c
@@ -4928,6 +4928,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 */
@@ -4939,11 +4940,20 @@ void bdrv_refresh_filename(BlockDriverState *bs)
* file BDS. The full options QDict of that file BDS should somehow
* contain a representation of the filename, therefore the following
* suffices without querying the (exact_)filename of this BDS. */
- if (bs->file->bs->full_open_options) {
+ if (bs->file->bs->full_open_options &&
+ (!bs->backing || bs->backing->bs->full_open_options))
+ {
qdict_put_str(opts, "driver", drv->format_name);
QINCREF(bs->file->bs->full_open_options);
qdict_put(opts, "file", bs->file->bs->full_open_options);
+ if (bs->backing) {
+ QINCREF(bs->backing->bs->full_open_options);
+ qdict_put(opts, "backing", bs->backing->bs->full_open_options);
+ } else if (bs->backing_overridden && !bs->backing) {
+ qdict_put(opts, "backing", qstring_new());
+ }
+
bs->full_open_options = opts;
} else {
QDECREF(opts);
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index e3c6eaba57..50d5cd07c8 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -59,7 +59,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
Testing: -drive file=TEST_DIR/t.qcow2,driver=qcow2,backing.file.filename=TEST_DIR/t.qcow2.orig,if=none,id=drive0 -nodefaults
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) info block
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.orig"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
Removable device: not locked, tray closed
Cache mode: writeback
Backing file: TEST_DIR/t.qcow2.orig (chain depth: 1)
@@ -148,7 +148,7 @@ QEMU_PROG: -drive driver=null-co,cache=invalid_value: invalid cache option
Testing: -drive file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) info block
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.base"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
Removable device: not locked, tray closed
Cache mode: writeback
Backing file: TEST_DIR/t.qcow2.base (chain depth: 1)
@@ -168,7 +168,7 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
Testing: -drive file=TEST_DIR/t.qcow2,cache=writethrough,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) info block
-drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2.base"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}} (qcow2)
Removable device: not locked, tray closed
Cache mode: writethrough
Backing file: TEST_DIR/t.qcow2.base (chain depth: 1)
@@ -188,7 +188,7 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
Testing: -drive file=TEST_DIR/t.qcow2,cache=unsafe,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) 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 f2c5622cee..406f83780b 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)
@@ -240,7 +240,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)
@@ -260,7 +260,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)
@@ -280,7 +280,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)
--
2.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 05/25] block: Make path_combine() return the path
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (3 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 04/25] block: Respect backing bs in bdrv_refresh_filename Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-26 12:00 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 06/25] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
` (19 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
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 3c3af462e4..a6ab70fb5e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -491,9 +491,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 ba24845960..32e8437107 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);
}
/*
@@ -287,7 +296,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);
}
}
@@ -4070,8 +4079,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)) {
@@ -4080,8 +4089,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 c665bcc977..5397a4345e 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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 06/25] block: bdrv_get_full_backing_filename_from_...'s ret. val.
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (4 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 05/25] block: Make path_combine() return the path Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-26 13:23 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 07/25] block: bdrv_get_full_backing_filename's " Max Reitz
` (18 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
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 a6ab70fb5e..3002236967 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -484,10 +484,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 32e8437107..4ca56983bb 100644
--- a/block.c
+++ b/block.c
@@ -283,20 +283,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);
}
}
@@ -304,9 +312,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)
@@ -4509,17 +4528,17 @@ void bdrv_img_create(const char *filename, const char *fmt,
size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
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 5397a4345e..c1dbcfe133 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2001,16 +2001,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 07/25] block: bdrv_get_full_backing_filename's ret. val.
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (5 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 06/25] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-26 13:52 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 08/25] block: Add bdrv_make_absolute_filename() Max Reitz
` (17 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
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 3002236967..65b15ecd18 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -482,8 +482,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 4ca56983bb..488abfbfc4 100644
--- a/block.c
+++ b/block.c
@@ -308,24 +308,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)
@@ -2189,7 +2178,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;
@@ -2223,7 +2212,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
@@ -2233,8 +2222,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);
@@ -2254,9 +2242,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: ");
@@ -4062,7 +4049,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;
@@ -4079,21 +4065,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 7fa2437923..949b4b0b90 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -276,18 +276,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 08/25] block: Add bdrv_make_absolute_filename()
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (6 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 07/25] block: bdrv_get_full_backing_filename's " Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-30 14:25 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 09/25] block: Fix bdrv_find_backing_image() Max Reitz
` (16 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
block.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 488abfbfc4..2da242c1e9 100644
--- a/block.c
+++ b/block.c
@@ -308,13 +308,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 09/25] block: Fix bdrv_find_backing_image()
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (7 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 08/25] block: Add bdrv_make_absolute_filename() Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-30 14:47 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 10/25] block: Add bdrv_dirname() Max Reitz
` (15 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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: Eric Blake <eblake@redhat.com>
---
block.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/block.c b/block.c
index 2da242c1e9..17579125de 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
@@ -4067,7 +4058,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);
@@ -4096,22 +4086,31 @@ 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);
+ filename_tmp = bdrv_make_absolute_filename(curr_bs, backing_file,
+ NULL);
+ if (!filename_tmp) {
+ continue;
+ }
/* We are going to compare absolute pathnames */
if (!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);
+ filename_tmp = bdrv_get_full_backing_filename(curr_bs, NULL);
+ if (!filename_tmp) {
+ continue;
+ }
if (!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;
@@ -4122,7 +4121,6 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
g_free(filename_full);
g_free(backing_file_full);
- g_free(filename_tmp);
return retval;
}
--
2.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 10/25] block: Add bdrv_dirname()
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (8 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 09/25] block: Fix bdrv_find_backing_image() Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-31 11:05 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 11/25] blkverify: Make bdrv_dirname() return NULL Max Reitz
` (14 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
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 65b15ecd18..34422e87e7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -486,6 +486,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 2b816ec601..3a71be3734 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 17579125de..d2ef7cbecb 100644
--- a/block.c
+++ b/block.c
@@ -5018,6 +5018,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 11/25] blkverify: Make bdrv_dirname() return NULL
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (9 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 10/25] block: Add bdrv_dirname() Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-31 11:45 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 12/25] quorum: " Max Reitz
` (13 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 12/25] quorum: Make bdrv_dirname() return NULL
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (10 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 11/25] blkverify: Make bdrv_dirname() return NULL Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-31 12:09 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 13/25] block/nbd: " Max Reitz
` (12 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 13/25] block/nbd: Make bdrv_dirname() return NULL
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (11 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 12/25] quorum: " Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 14/25] block/nfs: Implement bdrv_dirname() Max Reitz
` (11 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
block/nbd.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/block/nbd.c b/block/nbd.c
index a50d24b50a..ed921fa333 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -565,6 +565,16 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
bs->full_open_options = opts;
}
+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",
@@ -582,6 +592,7 @@ static BlockDriver bdrv_nbd = {
.bdrv_detach_aio_context = nbd_detach_aio_context,
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
+ .bdrv_dirname = nbd_dirname,
};
static BlockDriver bdrv_nbd_tcp = {
@@ -601,6 +612,7 @@ static BlockDriver bdrv_nbd_tcp = {
.bdrv_detach_aio_context = nbd_detach_aio_context,
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
+ .bdrv_dirname = nbd_dirname,
};
static BlockDriver bdrv_nbd_unix = {
@@ -620,6 +632,7 @@ static BlockDriver bdrv_nbd_unix = {
.bdrv_detach_aio_context = nbd_detach_aio_context,
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
+ .bdrv_dirname = nbd_dirname,
};
static void bdrv_nbd_init(void)
--
2.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 14/25] block/nfs: Implement bdrv_dirname()
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (12 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 13/25] block/nbd: " Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-31 12:29 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 15/25] block: Use bdrv_dirname() for relative filenames Max Reitz
` (10 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
block/nfs.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/block/nfs.c b/block/nfs.c
index 337fcd9c84..0152a9bd32 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -875,6 +875,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)
@@ -908,6 +921,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 15/25] block: Use bdrv_dirname() for relative filenames
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (13 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 14/25] block/nfs: Implement bdrv_dirname() Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-10-31 13:52 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option Max Reitz
` (9 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
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 d2ef7cbecb..fd67b667b7 100644
--- a/block.c
+++ b/block.c
@@ -306,12 +306,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (14 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 15/25] block: Use bdrv_dirname() for relative filenames Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-11-02 14:40 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 17/25] iotests: Add quorum case to test 110 Max Reitz
` (8 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
Using this option, one can directly override what bdrv_dirname() will
return. This is useful if one uses e.g. qcow2 on top of quorum (with
only protocol BDSs under the quorum BDS) and wants to be able to use
relative backing filenames.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qapi/block-core.json | 9 +++++++++
include/block/block_int.h | 2 ++
block.c | 14 ++++++++++++++
3 files changed, 25 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 750bb0c77c..c57741f4c6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3137,6 +3137,14 @@
# (default: off)
# @force-share: force share all permission on added nodes.
# Requires read-only=true. (Since 2.10)
+# @base-directory May be specified for any node. Normally, whenever a filename
+# is specified which is supposed to be relative to this node
+# (such as relative backing filenames), the base directory to be
+# used is the directory the image file of this node is in, which
+# is simply prepended to the relative filename. Using this
+# option, the string which is prepended (i.e. the base
+# directory) can be overridden.
+# (Since 2.10)
#
# Remaining options are determined by the block driver.
#
@@ -3145,6 +3153,7 @@
{ 'union': 'BlockdevOptions',
'base': { 'driver': 'BlockdevDriver',
'*node-name': 'str',
+ '*base-directory': 'str',
'*discard': 'BlockdevDiscardOptions',
'*cache': 'BlockdevCacheOptions',
'*read-only': 'bool',
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3a71be3734..3c53a373e1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -615,6 +615,8 @@ struct BlockDriverState {
QDict *full_open_options;
char exact_filename[PATH_MAX];
+ char *dirname;
+
BdrvChild *backing;
BdrvChild *file;
diff --git a/block.c b/block.c
index fd67b667b7..c055bcce5a 100644
--- a/block.c
+++ b/block.c
@@ -1250,6 +1250,12 @@ QemuOptsList bdrv_runtime_opts = {
.type = QEMU_OPT_BOOL,
.help = "always accept other writers (default: off)",
},
+ {
+ .name = "base-directory",
+ .type = QEMU_OPT_STRING,
+ .help = "String to prepend to filenames relative to this BDS for "
+ "making them absolute",
+ },
{ /* end of list */ }
},
};
@@ -1322,6 +1328,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
trace_bdrv_open_common(bs, filename ?: "", bs->open_flags,
drv->format_name);
+ bs->dirname = g_strdup(qemu_opt_get(opts, "base-directory"));
+
bs->read_only = !(bs->open_flags & BDRV_O_RDWR);
if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
@@ -3383,6 +3391,8 @@ static void bdrv_delete(BlockDriverState *bs)
bdrv_close(bs);
+ g_free(bs->dirname);
+
/* remove from list, if necessary */
if (bs->node_name[0] != '\0') {
QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
@@ -5032,6 +5042,10 @@ char *bdrv_dirname(BlockDriverState *bs, Error **errp)
{
BlockDriver *drv = bs->drv;
+ if (bs->dirname) {
+ return g_strdup(bs->dirname);
+ }
+
if (!drv) {
error_setg(errp, "Node '%s' is ejected", bs->node_name);
return NULL;
--
2.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 17/25] iotests: Add quorum case to test 110
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (15 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver Max Reitz
` (7 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
Test 110 tests relative backing filenames for complex BDS trees. Add
quorum as an example that can never work automatically (without
special-casing if all child nodes have the same base directory), and an
example on how to make it work manually (using the base-directory
option).
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/110 | 48 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/110.out | 12 ++++++++++++
2 files changed, 60 insertions(+)
diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
index ba1b3c6c7d..d96b656b6b 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,53 @@ 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
+
+echo
+
+# Should work fine
+TEST_IMG="json:{
+ 'driver': '$IMGFMT',
+ 'file': {
+ 'driver': 'quorum',
+ 'base-directory': '$TEST_DIR/',
+ '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..e1845d8026 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -19,4 +19,16 @@ 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)
+
+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 (actual path: TEST_DIR/t.IMGFMT.base)
*** done
--
2.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (16 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 17/25] iotests: Add quorum case to test 110 Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-11-02 16:11 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
` (6 subsequent siblings)
24 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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 | 6 ++++++
block/crypto.c | 5 +++++
block/curl.c | 21 +++++++++++++++++++++
block/gluster.c | 19 +++++++++++++++++++
block/iscsi.c | 18 ++++++++++++++++++
block/nbd.c | 14 ++++++++++++++
block/nfs.c | 4 ++++
block/null.c | 9 +++++++++
block/quorum.c | 8 ++++++++
block/rbd.c | 5 +++++
block/replication.c | 5 +++++
block/sheepdog.c | 12 ++++++++++++
block/ssh.c | 5 +++++
block/throttle.c | 4 ++++
block/vpc.c | 4 ++++
block/vvfat.c | 4 ++++
block/vxhs.c | 8 ++++++++
18 files changed, 158 insertions(+)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3c53a373e1..19d8d6801a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -444,6 +444,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 46e53f2f09..976dc43f17 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -905,6 +905,12 @@ 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 = (const char *const[]) { "config",
+ "inject-error.",
+ "set-state.",
+ "suspend.",
+ NULL },
};
static void bdrv_blkdebug_init(void)
diff --git a/block/crypto.c b/block/crypto.c
index 60ddf8623e..7c3681ff04 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -626,6 +626,11 @@ 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 = (const char *const[]) {
+ BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,
+ NULL
+ },
};
static void block_crypto_init(void)
diff --git a/block/curl.c b/block/curl.c
index 2a244e2439..11318a9a29 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -957,6 +957,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",
@@ -971,6 +984,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 = {
@@ -987,6 +1002,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 = {
@@ -1003,6 +1020,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 = {
@@ -1019,6 +1038,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 4683f3b244..5da74e3ccb 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2161,6 +2161,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",
@@ -2193,6 +2207,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)
@@ -2228,6 +2244,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 ed921fa333..0a36394b73 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -575,6 +575,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",
@@ -593,6 +604,7 @@ static BlockDriver bdrv_nbd = {
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_dirname = nbd_dirname,
+ .sgfnt_runtime_opts = nbd_sgfnt_runtime_opts,
};
static BlockDriver bdrv_nbd_tcp = {
@@ -613,6 +625,7 @@ static BlockDriver bdrv_nbd_tcp = {
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_dirname = nbd_dirname,
+ .sgfnt_runtime_opts = nbd_sgfnt_runtime_opts,
};
static BlockDriver bdrv_nbd_unix = {
@@ -633,6 +646,7 @@ static BlockDriver bdrv_nbd_unix = {
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
.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 0152a9bd32..e8cff25fd3 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -923,6 +923,10 @@ static BlockDriver bdrv_nfs = {
.bdrv_refresh_filename = nfs_refresh_filename,
.bdrv_dirname = nfs_dirname,
+ .sgfnt_runtime_opts = (const char *const[]) { "path", "uid",
+ "gid", "server.",
+ NULL },
+
#ifdef LIBNFS_FEATURE_PAGECACHE
.bdrv_invalidate_cache = nfs_invalidate_cache,
#endif
diff --git a/block/null.c b/block/null.c
index dd9c13f9ba..59673d2449 100644
--- a/block/null.c
+++ b/block/null.c
@@ -255,6 +255,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",
@@ -273,6 +280,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 = {
@@ -293,6 +301,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/quorum.c b/block/quorum.c
index e5a844335e..fa7b0f6da6 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1130,6 +1130,14 @@ static BlockDriver bdrv_quorum = {
.is_filter = true,
.bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter,
+
+ .sgfnt_runtime_opts = (const char *const[]) {
+ QUORUM_OPT_VOTE_THRESHOLD,
+ QUORUM_OPT_BLKVERIFY,
+ QUORUM_OPT_REWRITE,
+ QUORUM_OPT_READ_PATTERN,
+ NULL
+ },
};
static void bdrv_quorum_init(void)
diff --git a/block/rbd.c b/block/rbd.c
index 144f350e1f..79db69e767 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1151,6 +1151,11 @@ static BlockDriver bdrv_rbd = {
#ifdef LIBRBD_SUPPORTS_INVALIDATE
.bdrv_invalidate_cache = qemu_rbd_invalidate_cache,
#endif
+
+ .sgfnt_runtime_opts = (const char *const[]) { "pool", "image", "conf",
+ "snapshot", "user",
+ "server.",
+ "password-secret", NULL },
};
static void bdrv_rbd_init(void)
diff --git a/block/replication.c b/block/replication.c
index 3a4e6822e4..68795221fd 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -694,6 +694,11 @@ BlockDriver bdrv_replication = {
.bdrv_recurse_is_first_non_filter = replication_recurse_is_first_non_filter,
.has_variable_length = true,
+ .sgfnt_runtime_opts = (const char *const[]) {
+ REPLICATION_MODE,
+ REPLICATION_TOP_ID,
+ NULL
+ },
};
static void bdrv_replication_init(void)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 696a71442a..1b788534b6 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -3077,6 +3077,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",
@@ -3111,6 +3120,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 = {
@@ -3147,6 +3157,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 = {
@@ -3183,6 +3194,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 b049a16eb9..35387cf20d 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -1209,6 +1209,11 @@ static BlockDriver bdrv_ssh = {
.bdrv_getlength = ssh_getlength,
.bdrv_co_flush_to_disk = ssh_co_flush,
.create_opts = &ssh_create_opts,
+ .sgfnt_runtime_opts = (const char *const[]) {
+ "host", "port", "path", "user",
+ "host_key_check", "server.",
+ NULL
+ },
};
static void bdrv_ssh_init(void)
diff --git a/block/throttle.c b/block/throttle.c
index 5bca76300f..93341f8151 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -227,6 +227,10 @@ static BlockDriver bdrv_throttle = {
.bdrv_co_get_block_status = bdrv_co_get_block_status_from_file,
.is_filter = true,
+ .sgfnt_runtime_opts = (const char *const[]) {
+ QEMU_OPT_THROTTLE_GROUP_NAME,
+ NULL
+ },
};
static void bdrv_throttle_init(void)
diff --git a/block/vpc.c b/block/vpc.c
index 1576d7b595..f351a652b3 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1103,6 +1103,10 @@ static BlockDriver bdrv_vpc = {
.create_opts = &vpc_create_opts,
.bdrv_has_zero_init = vpc_has_zero_init,
+ .sgfnt_runtime_opts = (const char *const[]) {
+ VPC_OPT_SIZE_CALC,
+ NULL
+ },
};
static void bdrv_vpc_init(void)
diff --git a/block/vvfat.c b/block/vvfat.c
index a0f2335894..c667f03a14 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3252,6 +3252,10 @@ 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 = (const char *const[]) { "dir", "fat-type",
+ "floppy", "label", "rw",
+ NULL },
};
static void bdrv_vvfat_init(void)
diff --git a/block/vxhs.c b/block/vxhs.c
index 75cc6c8672..08cdc3b525 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -565,6 +565,14 @@ static BlockDriver bdrv_vxhs = {
.bdrv_getlength = vxhs_getlength,
.bdrv_aio_readv = vxhs_aio_readv,
.bdrv_aio_writev = vxhs_aio_writev,
+ .sgfnt_runtime_opts = (const char *const[]) {
+ VXHS_OPT_VDISK_ID,
+ "tls-creds",
+ VXHS_OPT_HOST,
+ VXHS_OPT_PORT,
+ VXHS_OPT_SERVER".",
+ NULL
+ },
};
static void bdrv_vxhs_init(void)
--
2.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (17 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-11-08 13:29 ` Alberto Garcia
2017-11-08 17:18 ` Kevin Wolf
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 20/25] block: Generically refresh runtime options Max Reitz
` (5 subsequent siblings)
24 siblings, 2 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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 want to use differ. See
quorum_gather_child_options() for more information.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/block/block_int.h | 13 +++++++++++++
block/quorum.c | 30 ++++++++++++++++++++++++++++++
block/vmdk.c | 13 +++++++++++++
3 files changed, 56 insertions(+)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 19d8d6801a..65e6500313 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -136,6 +136,19 @@ 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.
+ */
+ 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
* function's return value to them.
diff --git a/block/quorum.c b/block/quorum.c
index fa7b0f6da6..2daf8635f3 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1095,6 +1095,35 @@ 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).
+ */
+
+ 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
@@ -1114,6 +1143,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 c1dbcfe133..46f2fc80e5 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2327,6 +2327,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 */
+ 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),
@@ -2397,6 +2409,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 20/25] block: Generically refresh runtime options
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (18 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 21/25] block: Purify .bdrv_refresh_filename() Max Reitz
` (4 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 116 ++++++++++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/110.out | 4 +-
2 files changed, 117 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index c055bcce5a..55c16ae99c 100644
--- a/block.c
+++ b/block.c
@@ -4874,6 +4874,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", "base-directory", 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;
@@ -5028,9 +5114,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 e1845d8026..7eb199dd5c 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -22,12 +22,12 @@ 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)
-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", "base-directory": "TEST_DIR/", "vote-threshold": 1}}
file format: IMGFMT
virtual size: 64M (67108864 bytes)
backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
--
2.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 21/25] block: Purify .bdrv_refresh_filename()
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (19 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 20/25] block: Generically refresh runtime options Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 22/25] block: Do not copy exact_filename from format file Max Reitz
` (3 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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 | 52 ++++++-----------
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, 63 insertions(+), 272 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 65e6500313..464af1b101 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. A simple format
diff --git a/block.c b/block.c
index 55c16ae99c..415f9c49f0 100644
--- a/block.c
+++ b/block.c
@@ -4960,47 +4960,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
@@ -5018,6 +4977,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;
@@ -5033,90 +4994,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 && !bs->backing) {
- 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
@@ -5142,6 +5023,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 976dc43f17..e0d108f1a7 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -798,52 +798,34 @@ 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))
+ {
+ if (strcmp(qdict_entry_key(e), "config") &&
+ strcmp(qdict_entry_key(e), "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 1df8db5428..e2c617e79d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -244,7 +244,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 cac7490e04..5b4688acdd 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1075,7 +1075,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 0a36394b73..9ce9fad7ea 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -514,12 +514,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) {
@@ -532,8 +529,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);
@@ -547,22 +542,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 char *nbd_dirname(BlockDriverState *bs, Error **errp)
diff --git a/block/nfs.c b/block/nfs.c
index e8cff25fd3..83b62519f2 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -819,14 +819,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),
@@ -844,35 +839,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 59673d2449..65f5d681c0 100644
--- a/block/null.c
+++ b/block/null.c
@@ -241,18 +241,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 2daf8635f3..1b364188cc 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;
@@ -1142,7 +1113,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 22/25] block: Do not copy exact_filename from format file
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (20 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 21/25] block: Purify .bdrv_refresh_filename() Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 23/25] block: Fix FIXME from "Add BDS.backing_overridden" Max Reitz
` (2 subsequent siblings)
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
If the 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>
---
block.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 415f9c49f0..cfcbd3a09f 100644
--- a/block.c
+++ b/block.c
@@ -5034,9 +5034,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 23/25] block: Fix FIXME from "Add BDS.backing_overridden"
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (21 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 22/25] block: Do not copy exact_filename from format file Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 24/25] block/curl: Implement bdrv_refresh_filename() Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 25/25] block/null: Generate filename even with latency-ns Max Reitz
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
block.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 75 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index cfcbd3a09f..83da207268 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;
@@ -2185,6 +2188,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
*
@@ -2203,7 +2242,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;
@@ -2233,11 +2272,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;
@@ -2258,6 +2292,8 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
goto free_exit;
}
+ cloned_options = qdict_clone_shallow(options);
+
if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
qdict_put_str(options, "driver", bs->backing_format);
}
@@ -2272,6 +2308,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);
@@ -2287,6 +2327,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;
}
@@ -4903,6 +4944,34 @@ static const char *const *significant_options(BlockDriverState *bs,
}
/**
+ * 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
* significant_options().
--
2.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 24/25] block/curl: Implement bdrv_refresh_filename()
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (22 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 23/25] block: Fix FIXME from "Add BDS.backing_overridden" Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 25/25] block/null: Generate filename even with latency-ns Max Reitz
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/curl.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/block/curl.c b/block/curl.c
index 11318a9a29..fe57223fda 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -957,6 +957,20 @@ static int64_t curl_getlength(BlockDriverState *bs)
return s->len;
}
+static void curl_refresh_filename(BlockDriverState *bs)
+{
+ BDRVCURLState *s = bs->opaque;
+
+ if (!s->sslverify || 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,
@@ -985,6 +999,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,
};
@@ -1003,6 +1018,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,
};
@@ -1021,6 +1037,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,
};
@@ -1039,6 +1056,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* [Qemu-devel] [PATCH v6 25/25] block/null: Generate filename even with latency-ns
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
` (23 preceding siblings ...)
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 24/25] block/curl: Implement bdrv_refresh_filename() Max Reitz
@ 2017-09-29 16:53 ` Max Reitz
24 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-09-29 16:53 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, Max Reitz, Kevin Wolf, Alberto Garcia, Eric Blake, John Snow
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>
---
block/null.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/block/null.c b/block/null.c
index 65f5d681c0..ca0aa3d528 100644
--- a/block/null.c
+++ b/block/null.c
@@ -250,7 +250,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.13.6
^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 05/25] block: Make path_combine() return the path
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 05/25] block: Make path_combine() return the path Max Reitz
@ 2017-10-26 12:00 ` Alberto Garcia
0 siblings, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-10-26 12:00 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:27 PM CEST, Max Reitz wrote:
> 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>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 06/25] block: bdrv_get_full_backing_filename_from_...'s ret. val.
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 06/25] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
@ 2017-10-26 13:23 ` Alberto Garcia
0 siblings, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-10-26 13:23 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:28 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
> 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>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/25] block: bdrv_get_full_backing_filename's ret. val.
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 07/25] block: bdrv_get_full_backing_filename's " Max Reitz
@ 2017-10-26 13:52 ` Alberto Garcia
0 siblings, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-10-26 13:52 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:29 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
> 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>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 08/25] block: Add bdrv_make_absolute_filename()
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 08/25] block: Add bdrv_make_absolute_filename() Max Reitz
@ 2017-10-30 14:25 ` Alberto Garcia
0 siblings, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-10-30 14:25 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:30 PM CEST, Max Reitz wrote:
> 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>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 09/25] block: Fix bdrv_find_backing_image()
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 09/25] block: Fix bdrv_find_backing_image() Max Reitz
@ 2017-10-30 14:47 ` Alberto Garcia
2017-11-02 15:45 ` Max Reitz
0 siblings, 1 reply; 52+ messages in thread
From: Alberto Garcia @ 2017-10-30 14:47 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:31 PM CEST, Max Reitz wrote:
> @@ -4096,22 +4086,31 @@ 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);
> + filename_tmp = bdrv_make_absolute_filename(curr_bs, backing_file,
> + NULL);
> + if (!filename_tmp) {
> + continue;
> + }
>
> /* We are going to compare absolute pathnames */
> if (!realpath(filename_tmp, filename_full)) {
> + g_free(filename_tmp);
> continue;
> }
> + g_free(filename_tmp);
This feels a bit too verbose, doesn't it? (especially because you're
doing the same thing twice, see below). It could be made a bit shorter,
something like:
bool have_filename = filename_tmp && realpath(filename_tmp, filename_full);
g_free(filename_tmp);
if (!have_filename) {
continue;
}
> - path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
> - curr_bs->backing_file);
> + filename_tmp = bdrv_get_full_backing_filename(curr_bs, NULL);
> + if (!filename_tmp) {
> + continue;
> + }
>
> if (!realpath(filename_tmp, backing_file_full)) {
> + g_free(filename_tmp);
> continue;
> }
> + g_free(filename_tmp);
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 10/25] block: Add bdrv_dirname()
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 10/25] block: Add bdrv_dirname() Max Reitz
@ 2017-10-31 11:05 ` Alberto Garcia
0 siblings, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-10-31 11:05 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:32 PM CEST, Max Reitz wrote:
> 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>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 11/25] blkverify: Make bdrv_dirname() return NULL
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 11/25] blkverify: Make bdrv_dirname() return NULL Max Reitz
@ 2017-10-31 11:45 ` Alberto Garcia
0 siblings, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-10-31 11:45 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:33 PM CEST, Max Reitz wrote:
> 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>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 12/25] quorum: Make bdrv_dirname() return NULL
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 12/25] quorum: " Max Reitz
@ 2017-10-31 12:09 ` Alberto Garcia
0 siblings, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-10-31 12:09 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:34 PM CEST, Max Reitz wrote:
> 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>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 14/25] block/nfs: Implement bdrv_dirname()
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 14/25] block/nfs: Implement bdrv_dirname() Max Reitz
@ 2017-10-31 12:29 ` Alberto Garcia
0 siblings, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-10-31 12:29 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:36 PM CEST, Max Reitz wrote:
> 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>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 15/25] block: Use bdrv_dirname() for relative filenames
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 15/25] block: Use bdrv_dirname() for relative filenames Max Reitz
@ 2017-10-31 13:52 ` Alberto Garcia
0 siblings, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-10-31 13:52 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:37 PM CEST, Max Reitz wrote:
> 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>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option Max Reitz
@ 2017-11-02 14:40 ` Alberto Garcia
2017-11-02 16:07 ` Max Reitz
0 siblings, 1 reply; 52+ messages in thread
From: Alberto Garcia @ 2017-11-02 14:40 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:38 PM CEST, Max Reitz wrote:
> Using this option, one can directly override what bdrv_dirname() will
> return. This is useful if one uses e.g. qcow2 on top of quorum (with
> only protocol BDSs under the quorum BDS) and wants to be able to use
> relative backing filenames.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Who would be using this option in practice? I understand that management
tools should be using always absolute filenames, and this API doesn't
seem so convenient for humans.
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 09/25] block: Fix bdrv_find_backing_image()
2017-10-30 14:47 ` Alberto Garcia
@ 2017-11-02 15:45 ` Max Reitz
2017-11-02 15:59 ` Max Reitz
0 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-11-02 15:45 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]
On 2017-10-30 15:47, Alberto Garcia wrote:
> On Fri 29 Sep 2017 06:53:31 PM CEST, Max Reitz wrote:
>> @@ -4096,22 +4086,31 @@ 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);
>> + filename_tmp = bdrv_make_absolute_filename(curr_bs, backing_file,
>> + NULL);
>> + if (!filename_tmp) {
>> + continue;
>> + }
>>
>> /* We are going to compare absolute pathnames */
>> if (!realpath(filename_tmp, filename_full)) {
>> + g_free(filename_tmp);
>> continue;
>> }
>> + g_free(filename_tmp);
>
> This feels a bit too verbose, doesn't it? (especially because you're
> doing the same thing twice, see below). It could be made a bit shorter,
> something like:
>
> bool have_filename = filename_tmp && realpath(filename_tmp, filename_full);
> g_free(filename_tmp);
> if (!have_filename) {
> continue;
> }
Well, but then again
if (filename_tmp && realpath(filename_tmp, filename_full)) {
g_free(filename_tmp);
continue;
}
g_free(filename_tmp);
has the same length. :-)
(Actually, overall it'd be one line shorter because I'd have to use an
own line for the "bool have_filename" declaration (to put it at the
start of the block).)
So I'll do that, yes.
Thanks for reviewing (and this suggestion)!
Max
>
>> - path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
>> - curr_bs->backing_file);
>> + filename_tmp = bdrv_get_full_backing_filename(curr_bs, NULL);
>> + if (!filename_tmp) {
>> + continue;
>> + }
>>
>> if (!realpath(filename_tmp, backing_file_full)) {
>> + g_free(filename_tmp);
>> continue;
>> }
>> + g_free(filename_tmp);
>
> Berto
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 09/25] block: Fix bdrv_find_backing_image()
2017-11-02 15:45 ` Max Reitz
@ 2017-11-02 15:59 ` Max Reitz
0 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-11-02 15:59 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]
On 2017-11-02 16:45, Max Reitz wrote:
> On 2017-10-30 15:47, Alberto Garcia wrote:
>> On Fri 29 Sep 2017 06:53:31 PM CEST, Max Reitz wrote:
>>> @@ -4096,22 +4086,31 @@ 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);
>>> + filename_tmp = bdrv_make_absolute_filename(curr_bs, backing_file,
>>> + NULL);
>>> + if (!filename_tmp) {
>>> + continue;
>>> + }
>>>
>>> /* We are going to compare absolute pathnames */
>>> if (!realpath(filename_tmp, filename_full)) {
>>> + g_free(filename_tmp);
>>> continue;
>>> }
>>> + g_free(filename_tmp);
>>
>> This feels a bit too verbose, doesn't it? (especially because you're
>> doing the same thing twice, see below). It could be made a bit shorter,
>> something like:
>>
>> bool have_filename = filename_tmp && realpath(filename_tmp, filename_full);
>> g_free(filename_tmp);
>> if (!have_filename) {
>> continue;
>> }
>
> Well, but then again
>
> if (filename_tmp && realpath(filename_tmp, filename_full)) {
(err, I mean ||, of course.)
> g_free(filename_tmp);
> continue;
> }
> g_free(filename_tmp);
>
> has the same length. :-)
>
> (Actually, overall it'd be one line shorter because I'd have to use an
> own line for the "bool have_filename" declaration (to put it at the
> start of the block).)
>
> So I'll do that, yes.
>
> Thanks for reviewing (and this suggestion)!
>
> Max
>
>>
>>> - path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
>>> - curr_bs->backing_file);
>>> + filename_tmp = bdrv_get_full_backing_filename(curr_bs, NULL);
>>> + if (!filename_tmp) {
>>> + continue;
>>> + }
>>>
>>> if (!realpath(filename_tmp, backing_file_full)) {
>>> + g_free(filename_tmp);
>>> continue;
>>> }
>>> + g_free(filename_tmp);
>>
>> Berto
>>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option
2017-11-02 14:40 ` Alberto Garcia
@ 2017-11-02 16:07 ` Max Reitz
2017-11-02 22:06 ` Eric Blake
0 siblings, 1 reply; 52+ messages in thread
From: Max Reitz @ 2017-11-02 16:07 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
[-- Attachment #1: Type: text/plain, Size: 724 bytes --]
On 2017-11-02 15:40, Alberto Garcia wrote:
> On Fri 29 Sep 2017 06:53:38 PM CEST, Max Reitz wrote:
>> Using this option, one can directly override what bdrv_dirname() will
>> return. This is useful if one uses e.g. qcow2 on top of quorum (with
>> only protocol BDSs under the quorum BDS) and wants to be able to use
>> relative backing filenames.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>
> Who would be using this option in practice? I understand that management
> tools should be using always absolute filenames, and this API doesn't
> seem so convenient for humans.
Hmmm. I seem to recall Eric liked it a couple of years ago when he was
still more on the libvirt side of things. :-)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver Max Reitz
@ 2017-11-02 16:11 ` Alberto Garcia
2017-11-02 16:18 ` Max Reitz
0 siblings, 1 reply; 52+ messages in thread
From: Alberto Garcia @ 2017-11-02 16:11 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:40 PM CEST, Max Reitz wrote:
> 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;
> };
Is sgfnt a common acronym? I actually had to look it up...
> +static const char *const iscsi_sgfnt_runtime_opts[] = {
> + "transport",
> + "portal",
> + "target",
> + "user",
> + "password",
> + "password-secret",
> + "lun",
> + "initiator-name",
> + "header-digest",
> +
> + NULL
> +};
Ideally it would be nice to have constants for all these options, but
this is a pre-existing problem.
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver
2017-11-02 16:11 ` Alberto Garcia
@ 2017-11-02 16:18 ` Max Reitz
2017-11-08 13:01 ` Alberto Garcia
2017-11-08 17:14 ` Kevin Wolf
0 siblings, 2 replies; 52+ messages in thread
From: Max Reitz @ 2017-11-02 16:18 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]
On 2017-11-02 17:11, Alberto Garcia wrote:
> On Fri 29 Sep 2017 06:53:40 PM CEST, Max Reitz wrote:
>> 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;
>> };
>
> Is sgfnt a common acronym? I actually had to look it up...
I'm open for other suggestions to shorten "significant". :-)
(Actually, I can't remember where I got it from. I think I just made it
up, but then I don't know why I didn't use sgfcnt -- maybe because cnt
looks either like count (or like something else, but that's a different
matter)...?)
Max
>> +static const char *const iscsi_sgfnt_runtime_opts[] = {
>> + "transport",
>> + "portal",
>> + "target",
>> + "user",
>> + "password",
>> + "password-secret",
>> + "lun",
>> + "initiator-name",
>> + "header-digest",
>> +
>> + NULL
>> +};
>
> Ideally it would be nice to have constants for all these options, but
> this is a pre-existing problem.
>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
>
> Berto
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option
2017-11-02 16:07 ` Max Reitz
@ 2017-11-02 22:06 ` Eric Blake
2017-11-07 12:07 ` Alberto Garcia
0 siblings, 1 reply; 52+ messages in thread
From: Eric Blake @ 2017-11-02 22:06 UTC (permalink / raw)
To: Max Reitz, Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, John Snow
[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]
On 11/02/2017 11:07 AM, Max Reitz wrote:
> On 2017-11-02 15:40, Alberto Garcia wrote:
>> On Fri 29 Sep 2017 06:53:38 PM CEST, Max Reitz wrote:
>>> Using this option, one can directly override what bdrv_dirname() will
>>> return. This is useful if one uses e.g. qcow2 on top of quorum (with
>>> only protocol BDSs under the quorum BDS) and wants to be able to use
>>> relative backing filenames.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>
>> Who would be using this option in practice? I understand that management
>> tools should be using always absolute filenames, and this API doesn't
>> seem so convenient for humans.
>
> Hmmm. I seem to recall Eric liked it a couple of years ago when he was
> still more on the libvirt side of things. :-)
Ideally, libvirt should be using node names rather than filenames
(whether absolute or relative); but I still think it could be something
that turns out to be useful.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option
2017-11-02 22:06 ` Eric Blake
@ 2017-11-07 12:07 ` Alberto Garcia
2017-11-08 17:09 ` Kevin Wolf
0 siblings, 1 reply; 52+ messages in thread
From: Alberto Garcia @ 2017-11-07 12:07 UTC (permalink / raw)
To: Eric Blake, Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, John Snow
On Thu 02 Nov 2017 11:06:28 PM CET, Eric Blake wrote:
>>>> Using this option, one can directly override what bdrv_dirname()
>>>> will return. This is useful if one uses e.g. qcow2 on top of quorum
>>>> (with only protocol BDSs under the quorum BDS) and wants to be able
>>>> to use relative backing filenames.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>
>>> Who would be using this option in practice? I understand that
>>>management tools should be using always absolute filenames, and this
>>>API doesn't seem so convenient for humans.
>>
>> Hmmm. I seem to recall Eric liked it a couple of years ago when he
>> was still more on the libvirt side of things. :-)
>
> Ideally, libvirt should be using node names rather than filenames
> (whether absolute or relative); but I still think it could be
> something that turns out to be useful.
But what would be the use case? For a management tool it's not more
convenient to use relative file names, or is it?
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver
2017-11-02 16:18 ` Max Reitz
@ 2017-11-08 13:01 ` Alberto Garcia
2017-11-08 17:14 ` Kevin Wolf
1 sibling, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-11-08 13:01 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Thu 02 Nov 2017 05:18:31 PM CET, Max Reitz wrote:
> On 2017-11-02 17:11, Alberto Garcia wrote:
>> On Fri 29 Sep 2017 06:53:40 PM CEST, Max Reitz wrote:
>>> 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;
>>> };
>>
>> Is sgfnt a common acronym? I actually had to look it up...
>
> I'm open for other suggestions to shorten "significant". :-)
Nah, it's ok :)
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
@ 2017-11-08 13:29 ` Alberto Garcia
2017-11-08 17:18 ` Kevin Wolf
1 sibling, 0 replies; 52+ messages in thread
From: Alberto Garcia @ 2017-11-08 13:29 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake, John Snow
On Fri 29 Sep 2017 06:53:41 PM CEST, Max Reitz wrote:
> 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 want to use differ. See
> quorum_gather_child_options() for more information.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option
2017-11-07 12:07 ` Alberto Garcia
@ 2017-11-08 17:09 ` Kevin Wolf
2017-11-08 18:54 ` Max Reitz
0 siblings, 1 reply; 52+ messages in thread
From: Kevin Wolf @ 2017-11-08 17:09 UTC (permalink / raw)
To: Alberto Garcia; +Cc: Eric Blake, Max Reitz, qemu-block, qemu-devel, John Snow
Am 07.11.2017 um 13:07 hat Alberto Garcia geschrieben:
> On Thu 02 Nov 2017 11:06:28 PM CET, Eric Blake wrote:
>
> >>>> Using this option, one can directly override what bdrv_dirname()
> >>>> will return. This is useful if one uses e.g. qcow2 on top of quorum
> >>>> (with only protocol BDSs under the quorum BDS) and wants to be able
> >>>> to use relative backing filenames.
> >>>>
> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>>
> >>> Who would be using this option in practice? I understand that
> >>>management tools should be using always absolute filenames, and this
> >>>API doesn't seem so convenient for humans.
> >>
> >> Hmmm. I seem to recall Eric liked it a couple of years ago when he
> >> was still more on the libvirt side of things. :-)
> >
> > Ideally, libvirt should be using node names rather than filenames
> > (whether absolute or relative); but I still think it could be
> > something that turns out to be useful.
>
> But what would be the use case? For a management tool it's not more
> convenient to use relative file names, or is it?
If we add an external API, we must maintain compatibility in future
versions, so taking the patch comes with a cost. So I agree with you
that we better wait for an actual use case rather than just "could turn
out to be useful to someone maybe".
(Also, so that I don't have to post a second reply to a patch that might
be dropped: "Since: 2.10" wouldn't be quite right any more either way.)
Kevin
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver
2017-11-02 16:18 ` Max Reitz
2017-11-08 13:01 ` Alberto Garcia
@ 2017-11-08 17:14 ` Kevin Wolf
2017-11-08 18:52 ` Max Reitz
1 sibling, 1 reply; 52+ messages in thread
From: Kevin Wolf @ 2017-11-08 17:14 UTC (permalink / raw)
To: Max Reitz; +Cc: Alberto Garcia, qemu-block, qemu-devel, Eric Blake, John Snow
[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]
Am 02.11.2017 um 17:18 hat Max Reitz geschrieben:
> On 2017-11-02 17:11, Alberto Garcia wrote:
> > On Fri 29 Sep 2017 06:53:40 PM CEST, Max Reitz wrote:
> >> 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;
> >> };
> >
> > Is sgfnt a common acronym? I actually had to look it up...
>
> I'm open for other suggestions to shorten "significant". :-)
>
> (Actually, I can't remember where I got it from. I think I just made it
> up, but then I don't know why I didn't use sgfcnt -- maybe because cnt
> looks either like count (or like something else, but that's a different
> matter)...?)
Because in reality you were abbreviating "signifant"? ;-)
Anyway, I found the inline initialisation as part of BlockDriver rather
ugly. The places where you used a separate static const array look
much nicer. Maybe use that consistently in all places?
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
2017-11-08 13:29 ` Alberto Garcia
@ 2017-11-08 17:18 ` Kevin Wolf
2017-11-08 18:52 ` Max Reitz
1 sibling, 1 reply; 52+ messages in thread
From: Kevin Wolf @ 2017-11-08 17:18 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Alberto Garcia, Eric Blake, John Snow
Am 29.09.2017 um 18:53 hat Max Reitz geschrieben:
> 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.
Sounds more like a bug than a feature.
> For quorum, the child names that would be used by the generic
> implementation and the ones that we actually want to use differ. See
> quorum_gather_child_options() for more information.
:-/
What was the conclusion of our discussion at KVM Forum again? I just
remember that this caused problems in other contexts like dynamic
reconfiguration, too.
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> include/block/block_int.h | 13 +++++++++++++
> block/quorum.c | 30 ++++++++++++++++++++++++++++++
> block/vmdk.c | 13 +++++++++++++
> 3 files changed, 56 insertions(+)
Kevin
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options
2017-11-08 17:18 ` Kevin Wolf
@ 2017-11-08 18:52 ` Max Reitz
0 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-11-08 18:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Alberto Garcia, Eric Blake, John Snow
[-- Attachment #1: Type: text/plain, Size: 3456 bytes --]
On 2017-11-08 18:18, Kevin Wolf wrote:
> Am 29.09.2017 um 18:53 hat Max Reitz geschrieben:
>> 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.
>
> Sounds more like a bug than a feature.
Want to fix it? :-)
>> For quorum, the child names that would be used by the generic
>> implementation and the ones that we actually want to use differ. See
>> quorum_gather_child_options() for more information.
>
> :-/
>
> What was the conclusion of our discussion at KVM Forum again? I just
> remember that this caused problems in other contexts like dynamic
> reconfiguration, too.
I might have just winced a little.
The short conclusion was that it's all broken.
The long conclusion... Is that I might try to wrestle with quorum
before this series? :-/
Let's see... The conclusion was that the user needs to specify a unique
name for each quorum child. Then, this name would be equivalent to the
runtime name. Sounds good so far? Well, I might have just screamed a
little when I remembered another issue:
The generic implementation gathers the options like so:
{ /* node options */
"child0": { /* child0 options */ },
"child1": { /* child1 options */ },
... }
However, specifying child options like so doesn't work easily with QAPI
and quorum, for two reasons:
(1) Specifying arbitrary keys with specific values would need new QAPI
infrastructure. Who is going to implement this? :-)
(2) Quorum needs a fixed order for its children, at least for FIFO mode.
A JSON object does not have inherent ordering.
The simple way to fix this is to make the children list a list of
objects which contain the child name and the options:
{ /* quorum node options */
"children": [
{ "child-name": "child0",
"options": { /* child0 options */ } },
{ "child-name": "child1",
"options": { /* child1 options */ } },
... ] }
But this would still require bdrv_gather_child_options() to generate.
The other way would be to add the QAPI infrastructure to make the
generic thing work, and add a "children-order" list or something which
gives order to the children. Then the generic implementation should
work... As long as the quorum driver makes sure to update this list
whenever a child is added or removed.
Maybe the latter is actually the better choice? :-/
But it definitely means more work (because of the QAPI modifications)...
Max
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> include/block/block_int.h | 13 +++++++++++++
>> block/quorum.c | 30 ++++++++++++++++++++++++++++++
>> block/vmdk.c | 13 +++++++++++++
>> 3 files changed, 56 insertions(+)
>
> Kevin
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver
2017-11-08 17:14 ` Kevin Wolf
@ 2017-11-08 18:52 ` Max Reitz
0 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-11-08 18:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Alberto Garcia, qemu-block, qemu-devel, Eric Blake, John Snow
[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]
On 2017-11-08 18:14, Kevin Wolf wrote:
> Am 02.11.2017 um 17:18 hat Max Reitz geschrieben:
>> On 2017-11-02 17:11, Alberto Garcia wrote:
>>> On Fri 29 Sep 2017 06:53:40 PM CEST, Max Reitz wrote:
>>>> 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;
>>>> };
>>>
>>> Is sgfnt a common acronym? I actually had to look it up...
>>
>> I'm open for other suggestions to shorten "significant". :-)
>>
>> (Actually, I can't remember where I got it from. I think I just made it
>> up, but then I don't know why I didn't use sgfcnt -- maybe because cnt
>> looks either like count (or like something else, but that's a different
>> matter)...?)
>
> Because in reality you were abbreviating "signifant"? ;-)
I actually thought the very same, yes. ;-)
> Anyway, I found the inline initialisation as part of BlockDriver rather
> ugly. The places where you used a separate static const array look
> much nicer. Maybe use that consistently in all places?
Sure.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option
2017-11-08 17:09 ` Kevin Wolf
@ 2017-11-08 18:54 ` Max Reitz
0 siblings, 0 replies; 52+ messages in thread
From: Max Reitz @ 2017-11-08 18:54 UTC (permalink / raw)
To: Kevin Wolf, Alberto Garcia; +Cc: Eric Blake, qemu-block, qemu-devel, John Snow
[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]
On 2017-11-08 18:09, Kevin Wolf wrote:
> Am 07.11.2017 um 13:07 hat Alberto Garcia geschrieben:
>> On Thu 02 Nov 2017 11:06:28 PM CET, Eric Blake wrote:
>>
>>>>>> Using this option, one can directly override what bdrv_dirname()
>>>>>> will return. This is useful if one uses e.g. qcow2 on top of quorum
>>>>>> (with only protocol BDSs under the quorum BDS) and wants to be able
>>>>>> to use relative backing filenames.
>>>>>>
>>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>>
>>>>> Who would be using this option in practice? I understand that
>>>>> management tools should be using always absolute filenames, and this
>>>>> API doesn't seem so convenient for humans.
>>>>
>>>> Hmmm. I seem to recall Eric liked it a couple of years ago when he
>>>> was still more on the libvirt side of things. :-)
>>>
>>> Ideally, libvirt should be using node names rather than filenames
>>> (whether absolute or relative); but I still think it could be
>>> something that turns out to be useful.
>>
>> But what would be the use case? For a management tool it's not more
>> convenient to use relative file names, or is it?
>
> If we add an external API, we must maintain compatibility in future
> versions, so taking the patch comes with a cost. So I agree with you
> that we better wait for an actual use case rather than just "could turn
> out to be useful to someone maybe".
OK, fair enough. Also, adding it later would hopefully be as simple as
just sending this patch again.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2017-11-08 18:54 UTC | newest]
Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-29 16:53 [Qemu-devel] [PATCH v6 00/25] block: Fix some filename generation issues Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 01/25] block/mirror: Small absolute-paths simplification Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 02/25] block: Use children list in bdrv_refresh_filename Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 03/25] block: Add BDS.backing_overridden Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 04/25] block: Respect backing bs in bdrv_refresh_filename Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 05/25] block: Make path_combine() return the path Max Reitz
2017-10-26 12:00 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 06/25] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
2017-10-26 13:23 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 07/25] block: bdrv_get_full_backing_filename's " Max Reitz
2017-10-26 13:52 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 08/25] block: Add bdrv_make_absolute_filename() Max Reitz
2017-10-30 14:25 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 09/25] block: Fix bdrv_find_backing_image() Max Reitz
2017-10-30 14:47 ` Alberto Garcia
2017-11-02 15:45 ` Max Reitz
2017-11-02 15:59 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 10/25] block: Add bdrv_dirname() Max Reitz
2017-10-31 11:05 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 11/25] blkverify: Make bdrv_dirname() return NULL Max Reitz
2017-10-31 11:45 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 12/25] quorum: " Max Reitz
2017-10-31 12:09 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 13/25] block/nbd: " Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 14/25] block/nfs: Implement bdrv_dirname() Max Reitz
2017-10-31 12:29 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 15/25] block: Use bdrv_dirname() for relative filenames Max Reitz
2017-10-31 13:52 ` Alberto Garcia
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 16/25] block: Add 'base-directory' BDS option Max Reitz
2017-11-02 14:40 ` Alberto Garcia
2017-11-02 16:07 ` Max Reitz
2017-11-02 22:06 ` Eric Blake
2017-11-07 12:07 ` Alberto Garcia
2017-11-08 17:09 ` Kevin Wolf
2017-11-08 18:54 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 17/25] iotests: Add quorum case to test 110 Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 18/25] block: Add sgfnt_runtime_opts to BlockDriver Max Reitz
2017-11-02 16:11 ` Alberto Garcia
2017-11-02 16:18 ` Max Reitz
2017-11-08 13:01 ` Alberto Garcia
2017-11-08 17:14 ` Kevin Wolf
2017-11-08 18:52 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 19/25] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
2017-11-08 13:29 ` Alberto Garcia
2017-11-08 17:18 ` Kevin Wolf
2017-11-08 18:52 ` Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 20/25] block: Generically refresh runtime options Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 21/25] block: Purify .bdrv_refresh_filename() Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 22/25] block: Do not copy exact_filename from format file Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 23/25] block: Fix FIXME from "Add BDS.backing_overridden" Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 24/25] block/curl: Implement bdrv_refresh_filename() Max Reitz
2017-09-29 16:53 ` [Qemu-devel] [PATCH v6 25/25] 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.