* [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues
@ 2018-08-09 21:34 Max Reitz
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 01/31] block: Use bdrv_refresh_filename() to pull Max Reitz
` (32 more replies)
0 siblings, 33 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:34 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Once more, I’ll spare both me and you another iteration of the cover
letter, so see here:
http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html
(Although this series no longer includes a @base-directory option.)
In regards to the last version, the biggest change is that I dropped
backing_overridden and instead try to compare the filename from the
image header with the filename of the actual backing BDS to find out
whether the backing file has been overridden.
In order that this doesn’t break whenever the header contains a slightly
unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or
“nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something
different from what bdrv_refresh_filename() would generate), when the
reference filename in the BDS (auto_backing_file) is used to open the
backing file, it is updated from the backing BDS's resulting filename.
Changes in v10:
- Patches 2, 21, 24: Accomodate for blklogwrites
- Patch 5: Put bdrv_backing_overridden() into an own function, so I can
use it in a follow-up series that tries to make
bdrv_find_backing_image() work
- Patch 7: Add an optional parameter to node_info() so the caller can
choose whether this function shall return None if the node is not
found, or fail the whole test [Berto]
- Patch 8: Just changed the test number
Backport-diff:
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/31:[----] [--] 'block: Use bdrv_refresh_filename() to pull'
002/31:[0003] [FC] 'block: Use children list in bdrv_refresh_filename'
003/31:[----] [--] 'block: Skip implicit nodes for filename info'
004/31:[----] [--] 'block: Add BDS.auto_backing_file'
005/31:[0020] [FC] 'block: Respect backing bs in bdrv_refresh_filename'
006/31:[----] [--] 'iotests.py: Add filter_imgfmt()'
007/31:[0007] [FC] 'iotests.py: Add node_info()'
008/31:[0002] [FC] 'iotests: Add test for backing file overrides'
009/31:[----] [--] 'block: Make path_combine() return the path'
010/31:[----] [--] 'block: bdrv_get_full_backing_filename_from_...'s ret. val.'
011/31:[----] [--] 'block: bdrv_get_full_backing_filename's ret. val.'
012/31:[----] [--] 'block: Add bdrv_make_absolute_filename()'
013/31:[----] [--] 'block: Fix bdrv_find_backing_image()'
014/31:[----] [--] 'block: Add bdrv_dirname()'
015/31:[----] [--] 'blkverify: Make bdrv_dirname() return NULL'
016/31:[----] [--] 'quorum: Make bdrv_dirname() return NULL'
017/31:[----] [--] 'block/nbd: Make bdrv_dirname() return NULL'
018/31:[----] [--] 'block/nfs: Implement bdrv_dirname()'
019/31:[----] [--] 'block: Use bdrv_dirname() for relative filenames'
020/31:[----] [--] 'iotests: Add quorum case to test 110'
021/31:[0008] [FC] 'block: Add strong_runtime_opts to BlockDriver'
022/31:[----] [--] 'block: Add BlockDriver.bdrv_gather_child_options'
023/31:[----] [--] 'block: Generically refresh runtime options'
024/31:[0023] [FC] 'block: Purify .bdrv_refresh_filename()'
025/31:[----] [--] 'block: Do not copy exact_filename from format file'
026/31:[----] [--] 'block/nvme: Fix bdrv_refresh_filename()'
027/31:[----] [--] 'block/curl: Harmonize option defaults'
028/31:[----] [--] 'block/curl: Implement bdrv_refresh_filename()'
029/31:[----] [--] 'block/null: Generate filename even with latency-ns'
030/31:[----] [--] 'block: BDS options may lack the "driver" option'
031/31:[----] [-C] 'iotests: Test json:{} filenames of internal BDSs'
Max Reitz (31):
block: Use bdrv_refresh_filename() to pull
block: Use children list in bdrv_refresh_filename
block: Skip implicit nodes for filename info
block: Add BDS.auto_backing_file
block: Respect backing bs in bdrv_refresh_filename
iotests.py: Add filter_imgfmt()
iotests.py: Add node_info()
iotests: Add test for backing file overrides
block: Make path_combine() return the path
block: bdrv_get_full_backing_filename_from_...'s ret. val.
block: bdrv_get_full_backing_filename's ret. val.
block: Add bdrv_make_absolute_filename()
block: Fix bdrv_find_backing_image()
block: Add bdrv_dirname()
blkverify: Make bdrv_dirname() return NULL
quorum: Make bdrv_dirname() return NULL
block/nbd: Make bdrv_dirname() return NULL
block/nfs: Implement bdrv_dirname()
block: Use bdrv_dirname() for relative filenames
iotests: Add quorum case to test 110
block: Add strong_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/nvme: Fix bdrv_refresh_filename()
block/curl: Harmonize option defaults
block/curl: Implement bdrv_refresh_filename()
block/null: Generate filename even with latency-ns
block: BDS options may lack the "driver" option
iotests: Test json:{} filenames of internal BDSs
include/block/block.h | 16 +-
include/block/block_int.h | 48 +++-
block.c | 513 ++++++++++++++++++++++------------
block/blkdebug.c | 70 ++---
block/blklogwrites.c | 34 +--
block/blkverify.c | 29 +-
block/commit.c | 3 +-
block/crypto.c | 8 +
block/curl.c | 55 +++-
block/gluster.c | 19 ++
block/iscsi.c | 18 ++
block/mirror.c | 3 +-
block/nbd.c | 46 +--
block/nfs.c | 54 ++--
block/null.c | 32 ++-
block/nvme.c | 27 +-
block/qapi.c | 16 +-
block/qcow.c | 14 +-
block/qcow2.c | 17 +-
block/qed.c | 7 +-
block/quorum.c | 71 +++--
block/raw-format.c | 11 +-
block/rbd.c | 14 +
block/replication.c | 10 +-
block/sheepdog.c | 12 +
block/ssh.c | 12 +
block/throttle.c | 7 +
block/vhdx-log.c | 1 +
block/vmdk.c | 43 ++-
block/vpc.c | 11 +-
block/vvfat.c | 12 +
block/vxhs.c | 11 +
blockdev.c | 8 +
qemu-img.c | 12 +-
tests/qemu-iotests/051.out | 8 +-
tests/qemu-iotests/051.pc.out | 8 +-
tests/qemu-iotests/110 | 29 +-
tests/qemu-iotests/110.out | 9 +-
tests/qemu-iotests/224 | 139 +++++++++
tests/qemu-iotests/224.out | 18 ++
tests/qemu-iotests/228 | 235 ++++++++++++++++
tests/qemu-iotests/228.out | 84 ++++++
tests/qemu-iotests/group | 2 +
tests/qemu-iotests/iotests.py | 13 +
44 files changed, 1393 insertions(+), 416 deletions(-)
create mode 100755 tests/qemu-iotests/224
create mode 100644 tests/qemu-iotests/224.out
create mode 100755 tests/qemu-iotests/228
create mode 100644 tests/qemu-iotests/228.out
--
2.17.1
^ permalink raw reply [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 01/31] block: Use bdrv_refresh_filename() to pull
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
@ 2018-08-09 21:34 ` Max Reitz
2018-08-28 12:13 ` Alberto Garcia
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 02/31] block: Use children list in bdrv_refresh_filename Max Reitz
` (31 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:34 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Before this patch, bdrv_refresh_filename() is used in a pushing manner:
Whenever the BDS graph is modified, the parents of the modified edges
are supposed to be updated (recursively upwards). However, that is
nonviable, considering that we want child changes not to concern
parents.
Also, in the long run we want a pull model anyway: Here, we would have a
bdrv_filename() function which returns a BDS's filename, freshly
constructed.
This patch is an intermediate step. It adds bdrv_refresh_filename()
calls before every place a BDS.filename value is used. The only
exceptions are protocol drivers that use their own filename, which
clearly would not profit from refreshing that filename before.
Also, bdrv_get_encrypted_filename() is removed along the way (as a user
of BDS.filename), since it is completely unused.
In turn, all of the calls to bdrv_refresh_filename() before this patch
are removed, because we no longer have to call this function on graph
changes.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/block/block.h | 1 -
block.c | 31 +++++++++++++++----------------
block/qapi.c | 4 ++++
block/raw-format.c | 1 +
block/replication.c | 2 --
block/vhdx-log.c | 1 +
block/vmdk.c | 6 ++++++
block/vpc.c | 4 +++-
blockdev.c | 8 ++++++++
9 files changed, 38 insertions(+), 20 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 4e0871aaf9..f9f947cb9c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -483,7 +483,6 @@ void bdrv_round_to_clusters(BlockDriverState *bs,
int64_t *cluster_offset,
int64_t *cluster_bytes);
-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,
diff --git a/block.c b/block.c
index 39f373e035..8124d47360 100644
--- a/block.c
+++ b/block.c
@@ -304,8 +304,11 @@ void 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 *backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
+ char *backed;
+ bdrv_refresh_filename(bs);
+
+ backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file,
dest, sz, errp);
}
@@ -959,6 +962,8 @@ static void bdrv_backing_attach(BdrvChild *c)
"node is used as backing hd of '%s'",
bdrv_get_device_or_node_name(parent));
+ bdrv_refresh_filename(backing_hd);
+
parent->open_flags &= ~BDRV_O_NO_BACKING;
pstrcpy(parent->backing_file, sizeof(parent->backing_file),
backing_hd->filename);
@@ -1361,6 +1366,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
}
if (file != NULL) {
+ bdrv_refresh_filename(blk_bs(file));
filename = blk_bs(file)->filename;
} else {
/*
@@ -2252,8 +2258,6 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
bdrv_unref(backing_hd);
}
- bdrv_refresh_filename(bs);
-
out:
bdrv_refresh_limits(bs, NULL);
}
@@ -2767,8 +2771,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
}
}
- bdrv_refresh_filename(bs);
-
/* Check if any unknown options were used */
if (qdict_size(options) != 0) {
const QDictEntry *entry = qdict_first(options);
@@ -3200,6 +3202,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
if (local_err != NULL) {
error_propagate(errp, local_err);
} else {
+ bdrv_refresh_filename(reopen_state->bs);
error_setg(errp, "failed while preparing to reopen image '%s'",
reopen_state->bs->filename);
}
@@ -3737,7 +3740,10 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
/* success - we can delete the intermediate states, and link top->base */
/* TODO Check graph modification op blockers (BLK_PERM_GRAPH_MOD) once
* we've figured out how they should work. */
- backing_file_str = backing_file_str ? backing_file_str : base->filename;
+ if (!backing_file_str) {
+ bdrv_refresh_filename(base);
+ backing_file_str = base->filename;
+ }
QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
/* Check whether we are allowed to switch c from top to base */
@@ -4133,16 +4139,6 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs)
return bs->supported_zero_flags & BDRV_REQ_MAY_UNMAP;
}
-const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
-{
- if (bs->backing && bs->backing->bs->encrypted)
- return bs->backing_file;
- else if (bs->encrypted)
- return bs->filename;
- else
- return NULL;
-}
-
void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size)
{
@@ -4262,6 +4258,9 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
is_protocol = path_has_protocol(backing_file);
+ /* This will recursively refresh everything in the backing chain */
+ bdrv_refresh_filename(bs);
+
for (curr_bs = bs; curr_bs->backing; curr_bs = curr_bs->backing->bs) {
/* If either of the filename paths is actually a protocol, then
diff --git a/block/qapi.c b/block/qapi.c
index 339727f0f4..fc575e5919 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -51,6 +51,8 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
return NULL;
}
+ bdrv_refresh_filename(bs);
+
info = g_malloc0(sizeof(*info));
info->file = g_strdup(bs->filename);
info->ro = bs->read_only;
@@ -264,6 +266,8 @@ void bdrv_query_image_info(BlockDriverState *bs,
goto out;
}
+ bdrv_refresh_filename(bs);
+
info = g_new0(ImageInfo, 1);
info->filename = g_strdup(bs->filename);
info->format = g_strdup(bdrv_get_format_name(bs));
diff --git a/block/raw-format.c b/block/raw-format.c
index 2fd69cdb08..a9ddb362a0 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -436,6 +436,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
bs->file->bs->supported_zero_flags);
if (bs->probed && !bdrv_is_read_only(bs)) {
+ bdrv_refresh_filename(bs->file->bs);
fprintf(stderr,
"WARNING: Image format was not specified for '%s' and probing "
"guessed raw.\n"
diff --git a/block/replication.c b/block/replication.c
index 6349d6958e..d5e39be5eb 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -643,8 +643,6 @@ static void replication_done(void *opaque, int ret)
if (ret == 0) {
s->stage = BLOCK_REPLICATION_DONE;
- /* refresh top bs's filename */
- bdrv_refresh_filename(bs);
s->active_disk = NULL;
s->secondary_disk = NULL;
s->hidden_disk = NULL;
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index d2f1b98199..3f4c11c932 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -803,6 +803,7 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
if (logs.valid) {
if (bs->read_only) {
+ bdrv_refresh_filename(bs);
ret = -EPERM;
error_setg(errp,
"VHDX image file '%s' opened read-only, but "
diff --git a/block/vmdk.c b/block/vmdk.c
index a9d0084e36..87fdd647df 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -479,6 +479,7 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
extent->l1_table,
l1_size);
if (ret < 0) {
+ bdrv_refresh_filename(extent->file->bs);
error_setg_errno(errp, -ret,
"Could not read l1 table from extent '%s'",
extent->file->bs->filename);
@@ -499,6 +500,7 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
extent->l1_backup_table,
l1_size);
if (ret < 0) {
+ bdrv_refresh_filename(extent->file->bs);
error_setg_errno(errp, -ret,
"Could not read l1 backup table from extent '%s'",
extent->file->bs->filename);
@@ -530,6 +532,7 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
if (ret < 0) {
+ bdrv_refresh_filename(file->bs);
error_setg_errno(errp, -ret,
"Could not read header from file '%s'",
file->bs->filename);
@@ -607,6 +610,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
if (ret < 0) {
+ bdrv_refresh_filename(file->bs);
error_setg_errno(errp, -ret,
"Could not read header from file '%s'",
file->bs->filename);
@@ -861,6 +865,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
!desc_file_path[0])
{
+ bdrv_refresh_filename(bs->file->bs);
error_setg(errp, "Cannot use relative extent paths with VMDK "
"descriptor file '%s'", bs->file->bs->filename);
return -EINVAL;
@@ -2220,6 +2225,7 @@ static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent)
{
ImageInfo *info = g_new0(ImageInfo, 1);
+ bdrv_refresh_filename(extent->file->bs);
*info = (ImageInfo){
.filename = g_strdup(extent->file->bs->filename),
.format = g_strdup(extent->type),
diff --git a/block/vpc.c b/block/vpc.c
index bf294abfa7..717809cc24 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -284,9 +284,11 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
checksum = be32_to_cpu(footer->checksum);
footer->checksum = 0;
- if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum)
+ if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum) {
+ bdrv_refresh_filename(bs);
fprintf(stderr, "block-vpc: The header checksum of '%s' is "
"incorrect.\n", bs->filename);
+ }
/* Write 'checksum' back to footer, or else will leave it with zero. */
footer->checksum = cpu_to_be32(checksum);
diff --git a/blockdev.c b/blockdev.c
index 72f5347df5..9b143d9e72 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1627,6 +1627,7 @@ static void external_snapshot_prepare(BlkActionState *common,
error_setg_errno(errp, -size, "bdrv_getlength failed");
goto out;
}
+ bdrv_refresh_filename(state->old_bs);
bdrv_img_create(new_image_file, format,
state->old_bs->filename,
state->old_bs->drv->format_name,
@@ -3163,6 +3164,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
goto out;
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
+ bdrv_refresh_filename(base_bs);
base_name = base_bs->filename;
}
@@ -3251,6 +3253,10 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
top_bs = bs;
if (has_top && top) {
+ /* This strcmp() is just a shortcut, there is no need to
+ * refresh @bs's filename. If it mismatches,
+ * bdrv_find_backing_image() will do the refresh and may still
+ * return @bs. */
if (strcmp(bs->filename, top) != 0) {
top_bs = bdrv_find_backing_image(bs, top);
}
@@ -3398,6 +3404,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
if (backup->mode != NEW_IMAGE_MODE_EXISTING) {
assert(backup->format);
if (source) {
+ bdrv_refresh_filename(source);
bdrv_img_create(backup->target, backup->format, source->filename,
source->drv->format_name, NULL,
size, flags, false, &local_err);
@@ -3748,6 +3755,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
break;
case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
/* create new image with backing file */
+ bdrv_refresh_filename(source);
bdrv_img_create(arg->target, format,
source->filename,
source->drv->format_name,
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 02/31] block: Use children list in bdrv_refresh_filename
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 01/31] block: Use bdrv_refresh_filename() to pull Max Reitz
@ 2018-08-09 21:34 ` Max Reitz
2018-08-16 19:02 ` Eric Blake
2018-08-28 12:27 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 03/31] block: Skip implicit nodes for filename info Max Reitz
` (30 subsequent siblings)
32 siblings, 2 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:34 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
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, mirror, and blklogwrites.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 9 +++++----
block/blklogwrites.c | 3 ---
block/blkverify.c | 3 ---
block/commit.c | 1 -
block/mirror.c | 1 -
block/quorum.c | 1 -
6 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/block.c b/block.c
index 8124d47360..3b72363489 100644
--- a/block.c
+++ b/block.c
@@ -5174,16 +5174,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/blklogwrites.c b/block/blklogwrites.c
index ff98cd5533..8f1b589d54 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -285,9 +285,6 @@ static void blk_log_writes_refresh_filename(BlockDriverState *bs,
{
BDRVBlkLogWritesState *s = bs->opaque;
- /* bs->file->bs has already been refreshed */
- bdrv_refresh_filename(s->log_file->bs);
-
if (bs->file->bs->full_open_options
&& s->log_file->bs->full_open_options)
{
diff --git a/block/blkverify.c b/block/blkverify.c
index 89bf4386e3..035d77b64a 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -285,9 +285,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 e1814d9693..26db55d800 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -234,7 +234,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 b48c3f8cf5..e473c434ad 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1421,7 +1421,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 9152da8c58..03388590f3 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1080,7 +1080,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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 03/31] block: Skip implicit nodes for filename info
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 01/31] block: Use bdrv_refresh_filename() to pull Max Reitz
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 02/31] block: Use children list in bdrv_refresh_filename Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file Max Reitz
` (29 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
bdrv_refresh_filename() should simply skip all implicit nodes. They are
supposed to be invisible to the user, so they should not appear in
filename information.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/block.c b/block.c
index 3b72363489..feabe46061 100644
--- a/block.c
+++ b/block.c
@@ -5187,6 +5187,20 @@ void bdrv_refresh_filename(BlockDriverState *bs)
bdrv_refresh_filename(child->bs);
}
+ if (bs->implicit) {
+ /* For implicit nodes, just copy everything from the single child */
+ child = QLIST_FIRST(&bs->children);
+ assert(QLIST_NEXT(child, next) == NULL);
+
+ pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
+ child->bs->exact_filename);
+ pstrcpy(bs->filename, sizeof(bs->filename), child->bs->filename);
+
+ bs->full_open_options = qobject_ref(child->bs->full_open_options);
+
+ return;
+ }
+
if (drv->bdrv_refresh_filename) {
/* Obsolete information is of no use here, so drop the old file name
* information before refreshing it */
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (2 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 03/31] block: Skip implicit nodes for filename info Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-09-05 14:22 ` Alberto Garcia
2018-09-11 8:45 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 05/31] block: Respect backing bs in bdrv_refresh_filename Max Reitz
` (28 subsequent siblings)
32 siblings, 2 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
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().
To see whether it has been overridden, we might want to compare
bs->backing_file and bs->backing->bs->filename. However,
bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
to change the backing child at runtime, without modifying the image
header), so bs->backing_file most of the time simply contains a copy of
bs->backing->bs->filename anyway, so it is useless for such a
comparison.
This patch adds an auto_backing_file BDS field which contains the
backing file path as indicated by the image header, which is not changed
by bdrv_set_backing_hd().
Because of bdrv_refresh_filename() magic, however, a BDS's filename may
differ from what has been specified during bdrv_open(). Then, the
comparison between bs->auto_backing_file and bs->backing->bs->filename
may fail even though bs->backing was opened from bs->auto_backing_file.
To mitigate this, we can copy the real BDS's filename (after the whole
bdrv_open() and bdrv_refresh_filename() process) into
bs->auto_backing_file, if we know the former has been opened based on
the latter. This is only possible if no options modifying the backing
file's behavior have been specified, though. To simplify things, this
patch only copies the filename from the backing file if no options have
been specified for it at all.
Furthermore, there are cases where an overlay is created by qemu which
already contains a BDS's filename (e.g. in blockdev-snapshot-sync). We
do not need to worry about updating the overlay's bs->auto_backing_file
there, because we actually wrote a post-bdrv_refresh_filename() filename
into the image header.
So all in all, there will be false negatives where (as of a future
patch) bdrv_refresh_filename() will assume that the backing file differs
from what was specified in the image header, even though it really does
not. However, these cases should be limited to where (1) the user
actually did override something in the backing chain (e.g. by specifying
options for the backing file), or (2) the user executed a QMP command to
change some node's backing file (e.g. change-backing-file or
block-commit with @backing-file given) where the given filename does not
happen to coincide with qemu's idea of the backing BDS's filename.
Then again, (1) really is limited to -drive. With -blockdev or
blockdev-add, you have to adhere to the schema, so a user cannot give
partial "unimportant" options (e.g. by just setting backing.node-name
and leaving the rest to the image header). Therefore, trying to fix
this would mean trying to fix something for -drive only.
To improve on (2), we would need a full infrastructure to "canonicalize"
an arbitrary filename (+ options), so it can be compared against
another. That seems a bit over the top, considering that filenames
nowadays are there mostly for the user's entertainment.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/block/block_int.h | 4 ++++
block.c | 19 +++++++++++++++++++
block/qcow.c | 7 +++++--
block/qcow2.c | 10 +++++++---
block/qed.c | 7 +++++--
block/vmdk.c | 6 ++++--
6 files changed, 44 insertions(+), 9 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 903b9c1034..5ebe3917e5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -696,6 +696,10 @@ struct BlockDriverState {
char filename[PATH_MAX];
char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
this file image */
+ /* The backing filename indicated by the image header; if we ever
+ * open this file, then this is replaced by the resulting BDS's
+ * filename (i.e. after a bdrv_refresh_filename() run). */
+ char auto_backing_file[PATH_MAX];
char backing_format[16]; /* if non-zero and backing_file exists */
QDict *full_open_options;
diff --git a/block.c b/block.c
index feabe46061..655f662997 100644
--- a/block.c
+++ b/block.c
@@ -2279,6 +2279,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
char *bdref_key_dot;
const char *reference = NULL;
int ret = 0;
+ bool implicit_backing = false;
BlockDriverState *backing_hd;
QDict *options;
QDict *tmp_parent_options = NULL;
@@ -2314,6 +2315,16 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
qobject_unref(options);
goto free_exit;
} else {
+ if (qdict_size(options) == 0) {
+ /* If the user specifies options that do not modify the
+ * backing file's behavior, we might still consider it the
+ * implicit backing file. But it's easier this way, and
+ * just specifying some of the backing BDS's options is
+ * only possible with -drive anyway (otherwise the QAPI
+ * schema forces the user to specify everything). */
+ implicit_backing = !strcmp(bs->auto_backing_file, bs->backing_file);
+ }
+
bdrv_get_full_backing_filename(bs, backing_filename, PATH_MAX,
&local_err);
if (local_err) {
@@ -2347,6 +2358,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
}
bdrv_set_aio_context(backing_hd, bdrv_get_aio_context(bs));
+ if (implicit_backing) {
+ bdrv_refresh_filename(backing_hd);
+ pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+ backing_hd->filename);
+ }
+
/* 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);
@@ -3660,6 +3677,8 @@ int bdrv_change_backing_file(BlockDriverState *bs,
if (ret == 0) {
pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
+ pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+ backing_file ?: "");
}
return ret;
}
diff --git a/block/qcow.c b/block/qcow.c
index 385d935258..20f610ec74 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -31,6 +31,7 @@
#include "qemu/module.h"
#include "qemu/option.h"
#include "qemu/bswap.h"
+#include "qemu/cutils.h"
#include <zlib.h>
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qstring.h"
@@ -295,11 +296,13 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
ret = bdrv_pread(bs->file, header.backing_file_offset,
- bs->backing_file, len);
+ bs->auto_backing_file, len);
if (ret < 0) {
goto fail;
}
- bs->backing_file[len] = '\0';
+ bs->auto_backing_file[len] = '\0';
+ pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+ bs->auto_backing_file);
}
/* Disable migration when qcow images are used */
diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..6ca57b7d21 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1469,13 +1469,15 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
goto fail;
}
ret = bdrv_pread(bs->file, header.backing_file_offset,
- bs->backing_file, len);
+ bs->auto_backing_file, len);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not read backing file name");
goto fail;
}
- bs->backing_file[len] = '\0';
- s->image_backing_file = g_strdup(bs->backing_file);
+ bs->auto_backing_file[len] = '\0';
+ pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+ bs->auto_backing_file);
+ s->image_backing_file = g_strdup(bs->auto_backing_file);
}
/* Internal snapshots */
@@ -2469,6 +2471,8 @@ static int qcow2_change_backing_file(BlockDriverState *bs,
return -EINVAL;
}
+ pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+ backing_file ?: "");
pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_file ?: "");
pstrcpy(bs->backing_format, sizeof(bs->backing_format), backing_fmt ?: "");
diff --git a/block/qed.c b/block/qed.c
index 689ea9d4d5..1419cbe3c9 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -454,11 +454,14 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
}
ret = qed_read_string(bs->file, s->header.backing_filename_offset,
- s->header.backing_filename_size, bs->backing_file,
- sizeof(bs->backing_file));
+ s->header.backing_filename_size,
+ bs->auto_backing_file,
+ sizeof(bs->auto_backing_file));
if (ret < 0) {
return ret;
}
+ pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+ bs->auto_backing_file);
if (s->header.features & QED_F_BACKING_FORMAT_NO_PROBE) {
pstrcpy(bs->backing_format, sizeof(bs->backing_format), "raw");
diff --git a/block/vmdk.c b/block/vmdk.c
index 87fdd647df..09157da1df 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -386,12 +386,14 @@ static int vmdk_parent_open(BlockDriverState *bs)
ret = -EINVAL;
goto out;
}
- if ((end_name - p_name) > sizeof(bs->backing_file) - 1) {
+ if ((end_name - p_name) > sizeof(bs->auto_backing_file) - 1) {
ret = -EINVAL;
goto out;
}
- pstrcpy(bs->backing_file, end_name - p_name + 1, p_name);
+ pstrcpy(bs->auto_backing_file, end_name - p_name + 1, p_name);
+ pstrcpy(bs->backing_file, sizeof(bs->backing_file),
+ bs->auto_backing_file);
}
out:
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 05/31] block: Respect backing bs in bdrv_refresh_filename
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (3 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-28 13:26 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 06/31] iotests.py: Add filter_imgfmt() Max Reitz
` (27 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
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 overriding the backing file, and so
its output changes with this patch applied.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 38 ++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/051.out | 8 ++++----
tests/qemu-iotests/051.pc.out | 8 ++++----
3 files changed, 45 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index 655f662997..5edac648b2 100644
--- a/block.c
+++ b/block.c
@@ -5178,6 +5178,21 @@ static bool append_open_options(QDict *d, BlockDriverState *bs)
return found_any;
}
+/* Note: This function may return false positives; it may return true
+ * even if opening the backing file specified by bs's image header
+ * would result in exactly bs->backing. */
+static bool bdrv_backing_overridden(BlockDriverState *bs)
+{
+ if (bs->backing) {
+ return strcmp(bs->auto_backing_file,
+ bs->backing->bs->filename);
+ } else {
+ /* No backing BDS, so if the image header reports any backing
+ * file, it must have been suppressed */
+ return (bs->auto_backing_file[0] != '\0');
+ }
+}
+
/* 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
@@ -5195,6 +5210,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
BlockDriver *drv = bs->drv;
BdrvChild *child;
QDict *opts;
+ bool backing_overridden;
if (!drv) {
return;
@@ -5220,6 +5236,16 @@ void bdrv_refresh_filename(BlockDriverState *bs)
return;
}
+ backing_overridden = bdrv_backing_overridden(bs);
+
+ if (bs->open_flags & BDRV_O_NO_IO) {
+ /* Without I/O, the backing file does not change anything.
+ * Therefore, in such a case (primarily qemu-img), we can
+ * pretend the backing file has not been overridden even if
+ * it technically has been. */
+ backing_overridden = false;
+ }
+
if (drv->bdrv_refresh_filename) {
/* Obsolete information is of no use here, so drop the old file name
* information before refreshing it */
@@ -5245,6 +5271,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
opts = qdict_new();
has_open_options = append_open_options(opts, bs);
+ has_open_options |= 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 */
@@ -5256,11 +5283,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);
qdict_put(opts, "file",
qobject_ref(bs->file->bs->full_open_options));
+ if (bs->backing) {
+ qdict_put(opts, "backing",
+ qobject_ref(bs->backing->bs->full_open_options));
+ } else if (backing_overridden) {
+ qdict_put_null(opts, "backing");
+ }
+
bs->full_open_options = opts;
} else {
qobject_unref(opts);
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index b7273505c7..5f00b71fbb 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -82,7 +82,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)
@@ -172,7 +172,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)
@@ -192,7 +192,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)
@@ -212,7 +212,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 e9257fe318..8841129146 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -82,7 +82,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)
@@ -244,7 +244,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)
@@ -264,7 +264,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)
@@ -284,7 +284,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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 06/31] iotests.py: Add filter_imgfmt()
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (4 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 05/31] block: Respect backing bs in bdrv_refresh_filename Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 07/31] iotests.py: Add node_info() Max Reitz
` (26 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
tests/qemu-iotests/iotests.py | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4e67fbbe96..5c45788dac 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -246,6 +246,9 @@ def filter_img_info(output, filename):
lines.append(line)
return '\n'.join(lines)
+def filter_imgfmt(msg):
+ return msg.replace(imgfmt, 'IMGFMT')
+
def log(msg, filters=[]):
for flt in filters:
msg = flt(msg)
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 07/31] iotests.py: Add node_info()
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (5 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 06/31] iotests.py: Add filter_imgfmt() Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-28 12:48 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 08/31] iotests: Add test for backing file overrides Max Reitz
` (25 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
This function queries a node; since we cannot do that right now, it
executes query-named-block-nodes and returns the matching node's object.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5c45788dac..6583001ac8 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -465,6 +465,16 @@ class VM(qtest.QEMUQtestMachine):
else:
iotests.log(ev)
+ def node_info(self, node_name, require_existence=True):
+ nodes = self.qmp('query-named-block-nodes')
+ for x in nodes['return']:
+ if x['node-name'] == node_name:
+ return x
+ if require_existence:
+ assert False
+ else:
+ return None
+
index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 08/31] iotests: Add test for backing file overrides
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (6 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 07/31] iotests.py: Add node_info() Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-09-05 14:38 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 09/31] block: Make path_combine() return the path Max Reitz
` (24 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/228 | 235 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/228.out | 84 +++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 320 insertions(+)
create mode 100755 tests/qemu-iotests/228
create mode 100644 tests/qemu-iotests/228.out
diff --git a/tests/qemu-iotests/228 b/tests/qemu-iotests/228
new file mode 100755
index 0000000000..a2200efba5
--- /dev/null
+++ b/tests/qemu-iotests/228
@@ -0,0 +1,235 @@
+#!/usr/bin/env python
+#
+# Test for when a backing file is considered overridden (thus, a
+# json:{} filename is generated for the overlay) and when it is not
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Max Reitz <mreitz@redhat.com>
+
+import iotests
+from iotests import log, qemu_img, filter_testfiles, filter_imgfmt
+
+# Need backing file and change-backing-file support
+iotests.verify_image_format(supported_fmts=['qcow2', 'qed'])
+iotests.verify_platform(['linux'])
+
+
+def log_node_info(node):
+ log('')
+
+ log('bs->filename: ' + node['image']['filename'],
+ filters=[filter_testfiles, filter_imgfmt])
+ log('bs->backing_file: ' + node['backing_file'],
+ filters=[filter_testfiles, filter_imgfmt])
+
+ if 'backing-image' in node['image']:
+ log('bs->backing->bs->filename: ' +
+ node['image']['backing-image']['filename'],
+ filters=[filter_testfiles, filter_imgfmt])
+ else:
+ log('bs->backing: (none)')
+
+ log('')
+
+
+with iotests.FilePath('base.img') as base_img_path, \
+ iotests.FilePath('top.img') as top_img_path, \
+ iotests.VM() as vm:
+
+ assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
+ # Choose a funny way to describe the backing filename
+ assert qemu_img('create', '-f', iotests.imgfmt, '-b',
+ 'file:' + base_img_path, top_img_path) == 0
+
+ vm.launch()
+
+ log('--- Implicit backing file ---')
+ log('')
+
+ vm.qmp_log('blockdev-add',
+ node_name='node0',
+ driver=iotests.imgfmt,
+ file={
+ 'driver': 'file',
+ 'filename': top_img_path
+ },
+ filters=[filter_testfiles, filter_imgfmt])
+
+ # Filename should be plain, and the backing filename should not
+ # contain the "file:" prefix
+ log_node_info(vm.node_info('node0'))
+
+ vm.qmp_log('blockdev-del', node_name='node0')
+
+ log('')
+ log('--- change-backing-file ---')
+ log('')
+
+ vm.qmp_log('blockdev-add',
+ node_name='node0',
+ driver=iotests.imgfmt,
+ file={
+ 'driver': 'file',
+ 'filename': top_img_path
+ },
+ filters=[filter_testfiles, filter_imgfmt])
+
+ # Changing the backing file to a qemu-reported filename should
+ # result in qemu accepting the corresponding BDS as the implicit
+ # backing BDS (and thus not generate a json:{} filename).
+ # So, first, query the backing filename.
+
+ backing_filename = \
+ vm.node_info('node0')['image']['backing-image']['filename']
+
+ # Next, change the backing file to something different
+
+ vm.qmp_log('change-backing-file',
+ image_node_name='node0',
+ device='node0',
+ backing_file='null-co://')
+
+ # Now, verify that we get a json:{} filename
+ # (Image header says "null-co://", actual backing file still is
+ # base_img_path)
+
+ log_node_info(vm.node_info('node0'))
+
+ # Change it back
+ # (To get header and backing file in sync)
+
+ vm.qmp_log('change-backing-file',
+ image_node_name='node0',
+ device='node0',
+ backing_file=backing_filename)
+
+ # And verify that we get our original results
+
+ log_node_info(vm.node_info('node0'))
+
+ # Finally, try a "file:" prefix. While this is actually what we
+ # originally had in the image header, qemu will not reopen the
+ # backing file here, so it cannot verify that this filename
+ # "resolves" to the actual backing BDS's filename and will thus
+ # consider both to be different.
+ # (This may be fixed in the future.)
+
+ vm.qmp_log('change-backing-file',
+ image_node_name='node0',
+ device='node0',
+ backing_file=('file:' + backing_filename))
+
+ # So now we should get a json:{} filename
+
+ log_node_info(vm.node_info('node0'))
+
+ # Remove and re-attach so we can see that (as in our first try),
+ # opening the image anew helps qemu resolve the header backing
+ # filename.
+
+ vm.qmp_log('blockdev-del', node_name='node0')
+
+ vm.qmp_log('blockdev-add',
+ node_name='node0',
+ driver=iotests.imgfmt,
+ file={
+ 'driver': 'file',
+ 'filename': top_img_path
+ },
+ filters=[filter_testfiles, filter_imgfmt])
+
+ log_node_info(vm.node_info('node0'))
+
+ vm.qmp_log('blockdev-del', node_name='node0')
+
+ log('')
+ log('--- Override backing file ---')
+ log('')
+
+ # For this test, we need the plain filename in the image header
+ # (because qemu cannot "canonicalize"/"resolve" the backing
+ # filename unless the backing file is opened implicitly with the
+ # overlay)
+ assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+ top_img_path) == 0
+
+ # You can only reliably override backing options by using a node
+ # reference (or by specifying file.filename, but, well...)
+ vm.qmp_log('blockdev-add', node_name='null', driver='null-co')
+
+ vm.qmp_log('blockdev-add',
+ node_name='node0',
+ driver=iotests.imgfmt,
+ file={
+ 'driver': 'file',
+ 'filename': top_img_path
+ },
+ backing='null',
+ filters=[filter_testfiles, filter_imgfmt])
+
+ # Should get a json:{} filename (and bs->backing_file is
+ # null-co://, because that field actually has not much to do
+ # with the header backing filename (except that it is changed by
+ # change-backing-file))
+
+ log_node_info(vm.node_info('node0'))
+
+ # Detach the backing file by reopening the whole thing
+
+ vm.qmp_log('blockdev-del', node_name='node0')
+ vm.qmp_log('blockdev-del', node_name='null')
+
+ vm.qmp_log('blockdev-add',
+ node_name='node0',
+ driver=iotests.imgfmt,
+ file={
+ 'driver': 'file',
+ 'filename': top_img_path
+ },
+ backing=None,
+ filters=[filter_testfiles, filter_imgfmt])
+
+ # Should get a json:{} filename (because we overrode the backing
+ # file to not be there)
+
+ log_node_info(vm.node_info('node0'))
+
+ # Open the original backing file
+
+ vm.qmp_log('blockdev-add',
+ node_name='original-backing',
+ driver=iotests.imgfmt,
+ file={
+ 'driver': 'file',
+ 'filename': base_img_path
+ },
+ filters=[filter_testfiles, filter_imgfmt])
+
+ # Attach the original backing file to its overlay
+
+ vm.qmp_log('blockdev-snapshot',
+ node='original-backing',
+ overlay='node0')
+
+ # This should give us the original plain result
+
+ log_node_info(vm.node_info('node0'))
+
+ vm.qmp_log('blockdev-del', node_name='node0')
+ vm.qmp_log('blockdev-del', node_name='original-backing')
+
+ vm.shutdown()
diff --git a/tests/qemu-iotests/228.out b/tests/qemu-iotests/228.out
new file mode 100644
index 0000000000..b01de766bc
--- /dev/null
+++ b/tests/qemu-iotests/228.out
@@ -0,0 +1,84 @@
+--- Implicit backing file ---
+
+{'execute': 'blockdev-add', 'arguments': {'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-top.img'}, 'node_name': 'node0'}}
+{u'return': {}}
+
+bs->filename: TEST_DIR/PID-top.img
+bs->backing_file: TEST_DIR/PID-base.img
+bs->backing->bs->filename: TEST_DIR/PID-base.img
+
+{'execute': 'blockdev-del', 'arguments': {'node_name': 'node0'}}
+{u'return': {}}
+
+--- change-backing-file ---
+
+{'execute': 'blockdev-add', 'arguments': {'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-top.img'}, 'node_name': 'node0'}}
+{u'return': {}}
+{'execute': 'change-backing-file', 'arguments': {'device': 'node0', 'image_node_name': 'node0', 'backing_file': 'null-co://'}}
+{u'return': {}}
+
+bs->filename: json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}}
+bs->backing_file: null-co://
+bs->backing->bs->filename: TEST_DIR/PID-base.img
+
+{'execute': 'change-backing-file', 'arguments': {'device': 'node0', 'image_node_name': 'node0', 'backing_file': u'TEST_DIR/PID-base.img'}}
+{u'return': {}}
+
+bs->filename: TEST_DIR/PID-top.img
+bs->backing_file: TEST_DIR/PID-base.img
+bs->backing->bs->filename: TEST_DIR/PID-base.img
+
+{'execute': 'change-backing-file', 'arguments': {'device': 'node0', 'image_node_name': 'node0', 'backing_file': u'file:TEST_DIR/PID-base.img'}}
+{u'return': {}}
+
+bs->filename: json:{"backing": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}}
+bs->backing_file: file:TEST_DIR/PID-base.img
+bs->backing->bs->filename: TEST_DIR/PID-base.img
+
+{'execute': 'blockdev-del', 'arguments': {'node_name': 'node0'}}
+{u'return': {}}
+{'execute': 'blockdev-add', 'arguments': {'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-top.img'}, 'node_name': 'node0'}}
+{u'return': {}}
+
+bs->filename: TEST_DIR/PID-top.img
+bs->backing_file: TEST_DIR/PID-base.img
+bs->backing->bs->filename: TEST_DIR/PID-base.img
+
+{'execute': 'blockdev-del', 'arguments': {'node_name': 'node0'}}
+{u'return': {}}
+
+--- Override backing file ---
+
+{'execute': 'blockdev-add', 'arguments': {'driver': 'null-co', 'node_name': 'null'}}
+{u'return': {}}
+{'execute': 'blockdev-add', 'arguments': {'backing': 'null', 'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-top.img'}, 'node_name': 'node0'}}
+{u'return': {}}
+
+bs->filename: json:{"backing": {"driver": "null-co"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}}
+bs->backing_file: null-co://
+bs->backing->bs->filename: null-co://
+
+{'execute': 'blockdev-del', 'arguments': {'node_name': 'node0'}}
+{u'return': {}}
+{'execute': 'blockdev-del', 'arguments': {'node_name': 'null'}}
+{u'return': {}}
+{'execute': 'blockdev-add', 'arguments': {'backing': None, 'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-top.img'}, 'node_name': 'node0'}}
+{u'return': {}}
+
+bs->filename: json:{"backing": null, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-top.img"}}
+bs->backing_file: TEST_DIR/PID-base.img
+bs->backing: (none)
+
+{'execute': 'blockdev-add', 'arguments': {'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-base.img'}, 'node_name': 'original-backing'}}
+{u'return': {}}
+{'execute': 'blockdev-snapshot', 'arguments': {'node': 'original-backing', 'overlay': 'node0'}}
+{u'return': {}}
+
+bs->filename: TEST_DIR/PID-top.img
+bs->backing_file: TEST_DIR/PID-base.img
+bs->backing->bs->filename: TEST_DIR/PID-base.img
+
+{'execute': 'blockdev-del', 'arguments': {'node_name': 'node0'}}
+{u'return': {}}
+{'execute': 'blockdev-del', 'arguments': {'node_name': 'original-backing'}}
+{u'return': {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b973dc842d..5bfe2e246d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -225,3 +225,4 @@
225 rw auto quick
226 auto quick
227 auto quick
+228 rw auto quick
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 09/31] block: Make path_combine() return the path
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (7 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 08/31] iotests: Add test for backing file overrides Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 10/31] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
` (23 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Besides being safe for arbitrary path lengths, after some follow-up
patches all callers will want a freshly allocated buffer anyway.
In the meantime, path_combine_deprecated() is added which has the same
interface as path_combine() had before this patch. All callers to that
function will be converted in follow-up patches.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
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 f9f947cb9c..13d583dfd6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -494,9 +494,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 5edac648b2..3c62c22454 100644
--- a/block.c
+++ b/block.c
@@ -152,53 +152,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);
}
/*
@@ -297,7 +306,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);
}
}
@@ -4304,8 +4313,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)) {
@@ -4314,8 +4323,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 09157da1df..42b0f12021 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -873,8 +873,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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 10/31] block: bdrv_get_full_backing_filename_from_...'s ret. val.
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (8 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 09/31] block: Make path_combine() return the path Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 11/31] block: bdrv_get_full_backing_filename's " Max Reitz
` (22 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Make bdrv_get_full_backing_filename_from_filename() return an allocated
string instead of placing the result in a caller-provided buffer.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block.h | 7 +++---
block.c | 53 ++++++++++++++++++++++++++++++-------------
block/vmdk.c | 9 ++++----
qemu-img.c | 12 ++++------
4 files changed, 48 insertions(+), 33 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 13d583dfd6..efb767d284 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -487,10 +487,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 3c62c22454..741a5ff7e6 100644
--- a/block.c
+++ b/block.c
@@ -293,20 +293,29 @@ 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);
}
}
@@ -314,12 +323,24 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz,
Error **errp)
{
char *backed;
+ char *full_name;
+ Error *local_error = NULL;
bdrv_refresh_filename(bs);
backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
- 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)
@@ -4802,17 +4823,17 @@ void bdrv_img_create(const char *filename, const char *fmt,
size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, img_size);
if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
BlockDriverState *bs;
- char *full_backing = g_new0(char, PATH_MAX);
+ char *full_backing;
int back_flags;
QDict *backing_options = NULL;
- bdrv_get_full_backing_filename_from_filename(filename, backing_file,
- full_backing, PATH_MAX,
- &local_err);
+ full_backing =
+ bdrv_get_full_backing_filename_from_filename(filename, backing_file,
+ &local_err);
if (local_err) {
- g_free(full_backing);
goto out;
}
+ assert(full_backing);
/* backing files always opened read-only */
back_flags = flags;
diff --git a/block/vmdk.c b/block/vmdk.c
index 42b0f12021..1b1a829fa1 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2028,16 +2028,15 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts
}
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);
diff --git a/qemu-img.c b/qemu-img.c
index 1acddf693c..0752bbe4d9 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3329,18 +3329,14 @@ static int img_rebase(int argc, char **argv)
overlay_filename = bs->exact_filename[0] ? bs->exact_filename
: bs->filename;
- out_real_path = g_malloc(PATH_MAX);
-
- bdrv_get_full_backing_filename_from_filename(overlay_filename,
- out_baseimg,
- out_real_path,
- PATH_MAX,
- &local_err);
+ out_real_path =
+ bdrv_get_full_backing_filename_from_filename(overlay_filename,
+ out_baseimg,
+ &local_err);
if (local_err) {
error_reportf_err(local_err,
"Could not resolve backing filename: ");
ret = -1;
- g_free(out_real_path);
goto out;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 11/31] block: bdrv_get_full_backing_filename's ret. val.
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (9 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 10/31] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 12/31] block: Add bdrv_make_absolute_filename() Max Reitz
` (21 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Make bdrv_get_full_backing_filename() return an allocated string instead
of placing the result in a caller-provided buffer.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block.h | 3 +--
block.c | 48 +++++++++++++++----------------------------
block/qapi.c | 12 ++---------
3 files changed, 20 insertions(+), 43 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index efb767d284..4cec58bb3e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -485,8 +485,7 @@ void bdrv_round_to_clusters(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 741a5ff7e6..369c8fa027 100644
--- a/block.c
+++ b/block.c
@@ -319,28 +319,16 @@ 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;
- char *full_name;
- Error *local_error = NULL;
bdrv_refresh_filename(bs);
backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
-
- 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)
@@ -2305,7 +2293,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;
@@ -2340,7 +2328,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 */
} else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
qobject_unref(options);
goto free_exit;
@@ -2355,8 +2343,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
implicit_backing = !strcmp(bs->auto_backing_file, bs->backing_file);
}
- 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);
@@ -2377,9 +2364,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: ");
@@ -4295,7 +4281,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;
@@ -4315,21 +4300,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 fc575e5919..430d4b24d4 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -291,18 +291,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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 12/31] block: Add bdrv_make_absolute_filename()
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (10 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 11/31] block: bdrv_get_full_backing_filename's " Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 13/31] block: Fix bdrv_find_backing_image() Max Reitz
` (20 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
This is a general function for making a filename that is relative to a
certain BDS absolute.
It calls bdrv_get_full_backing_filename_from_filename() for now, but
that will be changed in a follow-up patch.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 369c8fa027..6606eeb7f4 100644
--- a/block.c
+++ b/block.c
@@ -319,16 +319,29 @@ 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;
+ char *bs_filename;
- bdrv_refresh_filename(bs);
+ bdrv_refresh_filename(relative_to);
+
+ bs_filename = relative_to->exact_filename[0]
+ ? relative_to->exact_filename
+ : relative_to->filename;
- backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
- return bdrv_get_full_backing_filename_from_filename(backed,
- bs->backing_file,
- errp);
+ return bdrv_get_full_backing_filename_from_filename(bs_filename,
+ filename ?: "", 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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 13/31] block: Fix bdrv_find_backing_image()
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (11 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 12/31] block: Add bdrv_make_absolute_filename() Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 14/31] block: Add bdrv_dirname() Max Reitz
` (19 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
bdrv_find_backing_image() should use bdrv_get_full_backing_filename() or
bdrv_make_absolute_filename() instead of trying to do what those
functions do by itself.
path_combine_deprecated() can now be dropped, so let's do that.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 33 ++++++++++-----------------------
1 file changed, 10 insertions(+), 23 deletions(-)
diff --git a/block.c b/block.c
index 6606eeb7f4..136594cb9e 100644
--- a/block.c
+++ b/block.c
@@ -201,15 +201,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
@@ -4301,13 +4292,9 @@ 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);
- /* This will recursively refresh everything in the backing chain */
- bdrv_refresh_filename(bs);
-
for (curr_bs = bs; curr_bs->backing; curr_bs = curr_bs->backing->bs) {
/* If either of the filename paths is actually a protocol, then
@@ -4333,22 +4320,23 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
} else {
/* If not an absolute filename path, make it relative to the current
* image's filename path */
- path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
- backing_file);
-
- /* We are going to compare absolute pathnames */
- if (!realpath(filename_tmp, filename_full)) {
+ filename_tmp = bdrv_make_absolute_filename(curr_bs, backing_file,
+ NULL);
+ /* We are going to compare canonicalized absolute pathnames */
+ if (!filename_tmp || !realpath(filename_tmp, filename_full)) {
+ g_free(filename_tmp);
continue;
}
+ g_free(filename_tmp);
/* We need to make sure the backing filename we are comparing against
* is relative to the current image filename (or absolute) */
- path_combine_deprecated(filename_tmp, PATH_MAX, curr_bs->filename,
- curr_bs->backing_file);
-
- if (!realpath(filename_tmp, backing_file_full)) {
+ filename_tmp = bdrv_get_full_backing_filename(curr_bs, NULL);
+ if (!filename_tmp || !realpath(filename_tmp, backing_file_full)) {
+ g_free(filename_tmp);
continue;
}
+ g_free(filename_tmp);
if (strcmp(backing_file_full, filename_full) == 0) {
retval = curr_bs->backing->bs;
@@ -4359,7 +4347,6 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
g_free(filename_full);
g_free(backing_file_full);
- g_free(filename_tmp);
return retval;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 14/31] block: Add bdrv_dirname()
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (12 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 13/31] block: Fix bdrv_find_backing_image() Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 15/31] blkverify: Make bdrv_dirname() return NULL Max Reitz
` (18 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
This function may be implemented by block drivers to derive a directory
name from a BDS. Concatenating this g_free()-able string with a relative
filename must result in a valid (not necessarily existing) filename, so
this is a function that should generally be not implemented by format
drivers, because this is protocol-specific.
If a BDS's driver does not implement this function, bdrv_dirname() will
fall through to the BDS's file if it exists. If it does not, the
exact_filename field will be used to generate a directory name.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block.h | 1 +
include/block/block_int.h | 7 +++++++
block.c | 27 +++++++++++++++++++++++++++
3 files changed, 35 insertions(+)
diff --git a/include/block/block.h b/include/block/block.h
index 4cec58bb3e..7ef118a704 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -489,6 +489,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 5ebe3917e5..80a947e676 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -141,6 +141,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_preadv)(BlockDriverState *bs,
uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags,
diff --git a/block.c b/block.c
index 136594cb9e..b68297e6f3 100644
--- a/block.c
+++ b/block.c
@@ -5353,6 +5353,33 @@ 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);
+ }
+
+ bdrv_refresh_filename(bs);
+ 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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 15/31] blkverify: Make bdrv_dirname() return NULL
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (13 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 14/31] block: Add bdrv_dirname() Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 16/31] quorum: " Max Reitz
` (17 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
blkverify's BDSs have a file BDS, but we do not want this to be
preferred over the raw node. There is no way to decide between the two
(and not really a reason to, either), so just return NULL in blkverify's
implementation of bdrv_dirname().
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/blkverify.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/block/blkverify.c b/block/blkverify.c
index 035d77b64a..3c7d4c8729 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -313,6 +313,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",
@@ -324,6 +333,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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 16/31] quorum: Make bdrv_dirname() return NULL
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (14 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 15/31] blkverify: Make bdrv_dirname() return NULL Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 17/31] block/nbd: " Max Reitz
` (16 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
While the common implementation for bdrv_dirname() should return NULL
for quorum BDSs already (because they do not have a file node and their
exact_filename field should be empty), there is no reason not to make
that explicit.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/quorum.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/block/quorum.c b/block/quorum.c
index 03388590f3..beec8209d1 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1101,6 +1101,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",
@@ -1109,6 +1119,7 @@ static BlockDriver bdrv_quorum = {
.bdrv_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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 17/31] block/nbd: Make bdrv_dirname() return NULL
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (15 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 16/31] quorum: " Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 18/31] block/nfs: Implement bdrv_dirname() Max Reitz
` (15 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
The generic bdrv_dirname() implementation would be able to generate some
form of directory name for many NBD nodes, but it would be always wrong.
Therefore, we have to explicitly make it an error (until NBD has some
form of specification for export paths, if it ever will).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/nbd.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/block/nbd.c b/block/nbd.c
index e87699fb73..bca127c8f5 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -564,6 +564,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_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_co_block_status = nbd_client_co_block_status,
+ .bdrv_dirname = nbd_dirname,
};
static BlockDriver bdrv_nbd_tcp = {
@@ -602,6 +613,7 @@ static BlockDriver bdrv_nbd_tcp = {
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_co_block_status = nbd_client_co_block_status,
+ .bdrv_dirname = nbd_dirname,
};
static BlockDriver bdrv_nbd_unix = {
@@ -622,6 +634,7 @@ static BlockDriver bdrv_nbd_unix = {
.bdrv_attach_aio_context = nbd_attach_aio_context,
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_co_block_status = nbd_client_co_block_status,
+ .bdrv_dirname = nbd_dirname,
};
static void bdrv_nbd_init(void)
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 18/31] block/nfs: Implement bdrv_dirname()
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (16 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 17/31] block/nbd: " Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-28 12:49 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 19/31] block: Use bdrv_dirname() for relative filenames Max Reitz
` (14 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
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 | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/block/nfs.c b/block/nfs.c
index eab1a2c408..19ee07c321 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -855,6 +855,20 @@ 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) {
+ bdrv_refresh_filename(bs);
+ error_setg(errp, "Cannot generate a base directory for NFS node '%s'",
+ bs->filename);
+ return NULL;
+ }
+
+ return g_strdup_printf("nfs://%s%s/", client->server->host, client->path);
+}
+
#ifdef LIBNFS_FEATURE_PAGECACHE
static void coroutine_fn nfs_co_invalidate_cache(BlockDriverState *bs,
Error **errp)
@@ -889,6 +903,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_co_invalidate_cache = nfs_co_invalidate_cache,
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 19/31] block: Use bdrv_dirname() for relative filenames
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (17 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 18/31] block/nfs: Implement bdrv_dirname() Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-28 13:45 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 20/31] iotests: Add quorum case to test 110 Max Reitz
` (13 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
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, 16 insertions(+), 9 deletions(-)
diff --git a/block.c b/block.c
index b68297e6f3..1fdbf627e0 100644
--- a/block.c
+++ b/block.c
@@ -318,16 +318,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;
+ char *dir, *full_name;
- bdrv_refresh_filename(relative_to);
+ if (!filename || filename[0] == '\0') {
+ return NULL;
+ } else if (path_has_protocol(filename) || path_is_absolute(filename)) {
+ return g_strdup(filename);
+ }
- bs_filename = relative_to->exact_filename[0]
- ? relative_to->exact_filename
- : relative_to->filename;
+ dir = bdrv_dirname(relative_to, errp);
+ if (!dir) {
+ return NULL;
+ }
- return bdrv_get_full_backing_filename_from_filename(bs_filename,
- filename ?: "", errp);
+ 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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 20/31] iotests: Add quorum case to test 110
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (18 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 19/31] block: Use bdrv_dirname() for relative filenames Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 21/31] block: Add strong_runtime_opts to BlockDriver Max Reitz
` (12 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Test 110 tests relative backing filenames for complex BDS trees. Now
that the originally supposedly failing test passes, let us add a new
failing test: Quorum can never work automatically (without detecting
whether all child nodes have the same base directory, but that would be
rather inconsistent behavior).
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
tests/qemu-iotests/110 | 26 ++++++++++++++++++++++++++
tests/qemu-iotests/110.out | 7 +++++++
2 files changed, 33 insertions(+)
diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
index ba1b3c6c7d..08976c8a61 100755
--- a/tests/qemu-iotests/110
+++ b/tests/qemu-iotests/110
@@ -30,6 +30,7 @@ status=1 # failure is the default!
_cleanup()
{
_cleanup_test_img
+ rm -f "$TEST_IMG.copy"
}
trap "_cleanup; exit \$status" 0 1 2 3 15
@@ -87,6 +88,31 @@ echo
# omit the image size; it should work anyway
_make_test_img -b "$TEST_IMG_REL.base"
+echo
+echo '=== Nodes without a common directory ==='
+echo
+
+cp "$TEST_IMG" "$TEST_IMG.copy"
+
+# Should inform us that the actual path of the backing file cannot be determined
+TEST_IMG="json:{
+ 'driver': '$IMGFMT',
+ 'file': {
+ 'driver': 'quorum',
+ 'vote-threshold': 1,
+ 'children': [
+ {
+ 'driver': 'file',
+ 'filename': '$TEST_IMG'
+ },
+ {
+ 'driver': 'file',
+ 'filename': '$TEST_IMG.copy'
+ }
+ ]
+ }
+}" _img_info | _filter_img_info
+
# success, all done
echo '*** done'
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index 5370bc1d26..1d0b2475cc 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -19,4 +19,11 @@ backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
=== Backing name is always relative to the backed image ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=t.IMGFMT.base
+
+=== Nodes without a common directory ===
+
+image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "blkverify": false, "rewrite-corrupted": false, "vote-threshold": 1}}
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+backing file: t.IMGFMT.base (cannot determine actual path)
*** done
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 21/31] block: Add strong_runtime_opts to BlockDriver
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (19 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 20/31] iotests: Add quorum case to test 110 Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-28 14:43 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 22/31] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
` (11 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
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 | 16 ++++++++++++++++
block/blklogwrites.c | 8 ++++++++
block/crypto.c | 8 ++++++++
block/curl.c | 21 +++++++++++++++++++++
block/gluster.c | 19 +++++++++++++++++++
block/iscsi.c | 18 ++++++++++++++++++
block/nbd.c | 14 ++++++++++++++
block/nfs.c | 11 +++++++++++
block/null.c | 9 +++++++++
block/nvme.c | 8 ++++++++
block/qcow.c | 7 +++++++
block/qcow2.c | 7 +++++++
block/quorum.c | 11 +++++++++++
block/raw-format.c | 10 +++++++++-
block/rbd.c | 14 ++++++++++++++
block/replication.c | 8 ++++++++
block/sheepdog.c | 12 ++++++++++++
block/ssh.c | 12 ++++++++++++
block/throttle.c | 7 +++++++
block/vpc.c | 7 +++++++
block/vvfat.c | 12 ++++++++++++
block/vxhs.c | 11 +++++++++++
23 files changed, 256 insertions(+), 1 deletion(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 80a947e676..1f53adfd96 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -516,6 +516,13 @@ struct BlockDriver {
void (*bdrv_register_buf)(BlockDriverState *bs, void *host, size_t size);
void (*bdrv_unregister_buf)(BlockDriverState *bs, void *host);
QLIST_ENTRY(BlockDriver) list;
+
+ /* Pointer to a NULL-terminated array of names of strong options
+ * that can be specified for bdrv_open(). A strong 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 strong. */
+ const char *const *strong_runtime_opts;
};
typedef struct BlockLimits {
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 0759452925..71b4275b98 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -888,6 +888,20 @@ static int blkdebug_reopen_prepare(BDRVReopenState *reopen_state,
return 0;
}
+static const char *const blkdebug_strong_runtime_opts[] = {
+ "config",
+ "inject-error.",
+ "set-state.",
+ "align",
+ "max-transfer",
+ "opt-write-zero",
+ "max-write-zero",
+ "opt-discard",
+ "max-discard",
+
+ NULL
+};
+
static BlockDriver bdrv_blkdebug = {
.format_name = "blkdebug",
.protocol_name = "blkdebug",
@@ -917,6 +931,8 @@ static BlockDriver bdrv_blkdebug = {
= blkdebug_debug_remove_breakpoint,
.bdrv_debug_resume = blkdebug_debug_resume,
.bdrv_debug_is_suspended = blkdebug_debug_is_suspended,
+
+ .strong_runtime_opts = blkdebug_strong_runtime_opts,
};
static void bdrv_blkdebug_init(void)
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 8f1b589d54..1c8e200326 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -518,6 +518,13 @@ blk_log_writes_co_pdiscard(BlockDriverState *bs, int64_t offset, int count)
LOG_DISCARD_FLAG, false);
}
+static const char *const blk_log_writes_strong_runtime_opts[] = {
+ "log-append",
+ "log-sector-size",
+
+ NULL
+};
+
static BlockDriver bdrv_blk_log_writes = {
.format_name = "blklogwrites",
.instance_size = sizeof(BDRVBlkLogWritesState),
@@ -537,6 +544,7 @@ static BlockDriver bdrv_blk_log_writes = {
.bdrv_co_block_status = bdrv_co_block_status_from_file,
.is_filter = true,
+ .strong_runtime_opts = blk_log_writes_strong_runtime_opts,
};
static void bdrv_blk_log_writes_init(void)
diff --git a/block/crypto.c b/block/crypto.c
index 146d81c90a..5731899f17 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -621,6 +621,12 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs)
return spec_info;
}
+static const char *const block_crypto_strong_runtime_opts[] = {
+ BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET,
+
+ NULL
+};
+
BlockDriver bdrv_crypto_luks = {
.format_name = "luks",
.instance_size = sizeof(BlockCrypto),
@@ -640,6 +646,8 @@ BlockDriver bdrv_crypto_luks = {
.bdrv_getlength = block_crypto_getlength,
.bdrv_get_info = block_crypto_get_info_luks,
.bdrv_get_specific_info = block_crypto_get_specific_info_luks,
+
+ .strong_runtime_opts = block_crypto_strong_runtime_opts,
};
static void block_crypto_init(void)
diff --git a/block/curl.c b/block/curl.c
index 229bb84a27..86e45cfde8 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -958,6 +958,19 @@ static int64_t curl_getlength(BlockDriverState *bs)
return s->len;
}
+static const char *const curl_strong_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",
@@ -972,6 +985,8 @@ static BlockDriver bdrv_http = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+
+ .strong_runtime_opts = curl_strong_runtime_opts,
};
static BlockDriver bdrv_https = {
@@ -988,6 +1003,8 @@ static BlockDriver bdrv_https = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+
+ .strong_runtime_opts = curl_strong_runtime_opts,
};
static BlockDriver bdrv_ftp = {
@@ -1004,6 +1021,8 @@ static BlockDriver bdrv_ftp = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+
+ .strong_runtime_opts = curl_strong_runtime_opts,
};
static BlockDriver bdrv_ftps = {
@@ -1020,6 +1039,8 @@ static BlockDriver bdrv_ftps = {
.bdrv_detach_aio_context = curl_detach_aio_context,
.bdrv_attach_aio_context = curl_attach_aio_context,
+
+ .strong_runtime_opts = curl_strong_runtime_opts,
};
static void curl_block_init(void)
diff --git a/block/gluster.c b/block/gluster.c
index 4fd55a9cc5..9f3611f443 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1487,6 +1487,21 @@ static int coroutine_fn qemu_gluster_co_block_status(BlockDriverState *bs,
}
+static const char *const gluster_strong_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",
@@ -1514,6 +1529,7 @@ static BlockDriver bdrv_gluster = {
#endif
.bdrv_co_block_status = qemu_gluster_co_block_status,
.create_opts = &qemu_gluster_create_opts,
+ .strong_runtime_opts = gluster_strong_open_opts,
};
static BlockDriver bdrv_gluster_tcp = {
@@ -1543,6 +1559,7 @@ static BlockDriver bdrv_gluster_tcp = {
#endif
.bdrv_co_block_status = qemu_gluster_co_block_status,
.create_opts = &qemu_gluster_create_opts,
+ .strong_runtime_opts = gluster_strong_open_opts,
};
static BlockDriver bdrv_gluster_unix = {
@@ -1572,6 +1589,7 @@ static BlockDriver bdrv_gluster_unix = {
#endif
.bdrv_co_block_status = qemu_gluster_co_block_status,
.create_opts = &qemu_gluster_create_opts,
+ .strong_runtime_opts = gluster_strong_open_opts,
};
/* rdma is deprecated (actually never supported for volfile fetch).
@@ -1607,6 +1625,7 @@ static BlockDriver bdrv_gluster_rdma = {
#endif
.bdrv_co_block_status = qemu_gluster_co_block_status,
.create_opts = &qemu_gluster_create_opts,
+ .strong_runtime_opts = gluster_strong_open_opts,
};
static void bdrv_gluster_init(void)
diff --git a/block/iscsi.c b/block/iscsi.c
index bb69faf34a..ecee7117ff 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2421,6 +2421,20 @@ static QemuOptsList iscsi_create_opts = {
}
};
+static const char *const iscsi_strong_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",
@@ -2455,6 +2469,8 @@ static BlockDriver bdrv_iscsi = {
.bdrv_detach_aio_context = iscsi_detach_aio_context,
.bdrv_attach_aio_context = iscsi_attach_aio_context,
+
+ .strong_runtime_opts = iscsi_strong_runtime_opts,
};
#if LIBISCSI_API_VERSION >= (20160603)
@@ -2492,6 +2508,8 @@ static BlockDriver bdrv_iser = {
.bdrv_detach_aio_context = iscsi_detach_aio_context,
.bdrv_attach_aio_context = iscsi_attach_aio_context,
+
+ .strong_runtime_opts = iscsi_strong_runtime_opts,
};
#endif
diff --git a/block/nbd.c b/block/nbd.c
index bca127c8f5..5b4e4cf1db 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -574,6 +574,17 @@ static char *nbd_dirname(BlockDriverState *bs, Error **errp)
return NULL;
}
+static const char *const nbd_strong_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_refresh_filename = nbd_refresh_filename,
.bdrv_co_block_status = nbd_client_co_block_status,
.bdrv_dirname = nbd_dirname,
+ .strong_runtime_opts = nbd_strong_runtime_opts,
};
static BlockDriver bdrv_nbd_tcp = {
@@ -614,6 +626,7 @@ static BlockDriver bdrv_nbd_tcp = {
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_co_block_status = nbd_client_co_block_status,
.bdrv_dirname = nbd_dirname,
+ .strong_runtime_opts = nbd_strong_runtime_opts,
};
static BlockDriver bdrv_nbd_unix = {
@@ -635,6 +648,7 @@ static BlockDriver bdrv_nbd_unix = {
.bdrv_refresh_filename = nbd_refresh_filename,
.bdrv_co_block_status = nbd_client_co_block_status,
.bdrv_dirname = nbd_dirname,
+ .strong_runtime_opts = nbd_strong_runtime_opts,
};
static void bdrv_nbd_init(void)
diff --git a/block/nfs.c b/block/nfs.c
index 19ee07c321..6985a44b89 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -878,6 +878,15 @@ static void coroutine_fn nfs_co_invalidate_cache(BlockDriverState *bs,
}
#endif
+static const char *nfs_strong_runtime_opts[] = {
+ "path",
+ "user",
+ "group",
+ "server.",
+
+ NULL
+};
+
static BlockDriver bdrv_nfs = {
.format_name = "nfs",
.protocol_name = "nfs",
@@ -905,6 +914,8 @@ static BlockDriver bdrv_nfs = {
.bdrv_refresh_filename = nfs_refresh_filename,
.bdrv_dirname = nfs_dirname,
+ .strong_runtime_opts = nfs_strong_runtime_opts,
+
#ifdef LIBNFS_FEATURE_PAGECACHE
.bdrv_co_invalidate_cache = nfs_co_invalidate_cache,
#endif
diff --git a/block/null.c b/block/null.c
index 5d610fdfba..07acb4f732 100644
--- a/block/null.c
+++ b/block/null.c
@@ -256,6 +256,13 @@ static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
bs->full_open_options = qobject_ref(opts);
}
+static const char *const null_strong_runtime_opts[] = {
+ BLOCK_OPT_SIZE,
+ NULL_OPT_ZEROES,
+
+ NULL
+};
+
static BlockDriver bdrv_null_co = {
.format_name = "null-co",
.protocol_name = "null-co",
@@ -274,6 +281,7 @@ static BlockDriver bdrv_null_co = {
.bdrv_co_block_status = null_co_block_status,
.bdrv_refresh_filename = null_refresh_filename,
+ .strong_runtime_opts = null_strong_runtime_opts,
};
static BlockDriver bdrv_null_aio = {
@@ -294,6 +302,7 @@ static BlockDriver bdrv_null_aio = {
.bdrv_co_block_status = null_co_block_status,
.bdrv_refresh_filename = null_refresh_filename,
+ .strong_runtime_opts = null_strong_runtime_opts,
};
static void bdrv_null_init(void)
diff --git a/block/nvme.c b/block/nvme.c
index 6f71122bf5..b0548e4e37 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1154,6 +1154,13 @@ static void nvme_unregister_buf(BlockDriverState *bs, void *host)
qemu_vfio_dma_unmap(s->vfio, host);
}
+static const char *const nvme_strong_runtime_opts[] = {
+ NVME_BLOCK_OPT_DEVICE,
+ NVME_BLOCK_OPT_NAMESPACE,
+
+ NULL
+};
+
static BlockDriver bdrv_nvme = {
.format_name = "nvme",
.protocol_name = "nvme",
@@ -1171,6 +1178,7 @@ static BlockDriver bdrv_nvme = {
.bdrv_refresh_filename = nvme_refresh_filename,
.bdrv_refresh_limits = nvme_refresh_limits,
+ .strong_runtime_opts = nvme_strong_runtime_opts,
.bdrv_detach_aio_context = nvme_detach_aio_context,
.bdrv_attach_aio_context = nvme_attach_aio_context,
diff --git a/block/qcow.c b/block/qcow.c
index 20f610ec74..61c3c70b0a 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -1186,6 +1186,12 @@ static QemuOptsList qcow_create_opts = {
}
};
+static const char *const qcow_strong_runtime_opts[] = {
+ "encrypt." BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET,
+
+ NULL
+};
+
static BlockDriver bdrv_qcow = {
.format_name = "qcow",
.instance_size = sizeof(BDRVQcowState),
@@ -1209,6 +1215,7 @@ static BlockDriver bdrv_qcow = {
.bdrv_get_info = qcow_get_info,
.create_opts = &qcow_create_opts,
+ .strong_runtime_opts = qcow_strong_runtime_opts,
};
static void bdrv_qcow_init(void)
diff --git a/block/qcow2.c b/block/qcow2.c
index 6ca57b7d21..97c406d95f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4745,6 +4745,12 @@ static QemuOptsList qcow2_create_opts = {
}
};
+static const char *const qcow2_strong_runtime_opts[] = {
+ "encrypt." BLOCK_CRYPTO_OPT_QCOW_KEY_SECRET,
+
+ NULL
+};
+
BlockDriver bdrv_qcow2 = {
.format_name = "qcow2",
.instance_size = sizeof(BDRVQcow2State),
@@ -4793,6 +4799,7 @@ BlockDriver bdrv_qcow2 = {
.bdrv_inactivate = qcow2_inactivate,
.create_opts = &qcow2_create_opts,
+ .strong_runtime_opts = qcow2_strong_runtime_opts,
.bdrv_co_check = qcow2_co_check,
.bdrv_amend_options = qcow2_amend_options,
diff --git a/block/quorum.c b/block/quorum.c
index beec8209d1..fdc0e7d16d 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1111,6 +1111,15 @@ static char *quorum_dirname(BlockDriverState *bs, Error **errp)
return NULL;
}
+static const char *const quorum_strong_runtime_opts[] = {
+ QUORUM_OPT_VOTE_THRESHOLD,
+ QUORUM_OPT_BLKVERIFY,
+ QUORUM_OPT_REWRITE,
+ QUORUM_OPT_READ_PATTERN,
+
+ NULL
+};
+
static BlockDriver bdrv_quorum = {
.format_name = "quorum",
@@ -1135,6 +1144,8 @@ static BlockDriver bdrv_quorum = {
.is_filter = true,
.bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter,
+
+ .strong_runtime_opts = quorum_strong_runtime_opts,
};
static void bdrv_quorum_init(void)
diff --git a/block/raw-format.c b/block/raw-format.c
index a9ddb362a0..42aebafb8a 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -536,6 +536,13 @@ static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
read_flags, write_flags);
}
+static const char *const raw_strong_runtime_opts[] = {
+ "offset",
+ "size",
+
+ NULL
+};
+
BlockDriver bdrv_raw = {
.format_name = "raw",
.instance_size = sizeof(BDRVRawState),
@@ -566,7 +573,8 @@ BlockDriver bdrv_raw = {
.bdrv_lock_medium = &raw_lock_medium,
.bdrv_co_ioctl = &raw_co_ioctl,
.create_opts = &raw_create_opts,
- .bdrv_has_zero_init = &raw_has_zero_init
+ .bdrv_has_zero_init = &raw_has_zero_init,
+ .strong_runtime_opts = raw_strong_runtime_opts,
};
static void bdrv_raw_init(void)
diff --git a/block/rbd.c b/block/rbd.c
index ca8e5bbace..bb098e66e0 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1172,6 +1172,18 @@ static QemuOptsList qemu_rbd_create_opts = {
}
};
+static const char *const qemu_rbd_strong_runtime_opts[] = {
+ "pool",
+ "image",
+ "conf",
+ "snapshot",
+ "user",
+ "server.",
+ "password-secret",
+
+ NULL
+};
+
static BlockDriver bdrv_rbd = {
.format_name = "rbd",
.instance_size = sizeof(BDRVRBDState),
@@ -1209,6 +1221,8 @@ static BlockDriver bdrv_rbd = {
#ifdef LIBRBD_SUPPORTS_INVALIDATE
.bdrv_co_invalidate_cache = qemu_rbd_co_invalidate_cache,
#endif
+
+ .strong_runtime_opts = qemu_rbd_strong_runtime_opts,
};
static void bdrv_rbd_init(void)
diff --git a/block/replication.c b/block/replication.c
index d5e39be5eb..0bc24c0e12 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -703,6 +703,13 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
aio_context_release(aio_context);
}
+static const char *const replication_strong_runtime_opts[] = {
+ REPLICATION_MODE,
+ REPLICATION_TOP_ID,
+
+ NULL
+};
+
BlockDriver bdrv_replication = {
.format_name = "replication",
.instance_size = sizeof(BDRVReplicationState),
@@ -719,6 +726,7 @@ BlockDriver bdrv_replication = {
.bdrv_recurse_is_first_non_filter = replication_recurse_is_first_non_filter,
.has_variable_length = true,
+ .strong_runtime_opts = replication_strong_runtime_opts,
};
static void bdrv_replication_init(void)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b229a664d9..3cc62d3d68 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -3216,6 +3216,15 @@ static QemuOptsList sd_create_opts = {
}
};
+static const char *const sd_strong_runtime_opts[] = {
+ "vdi",
+ "snap-id",
+ "tag",
+ "server.",
+
+ NULL
+};
+
static BlockDriver bdrv_sheepdog = {
.format_name = "sheepdog",
.protocol_name = "sheepdog",
@@ -3251,6 +3260,7 @@ static BlockDriver bdrv_sheepdog = {
.bdrv_attach_aio_context = sd_attach_aio_context,
.create_opts = &sd_create_opts,
+ .strong_runtime_opts = sd_strong_runtime_opts,
};
static BlockDriver bdrv_sheepdog_tcp = {
@@ -3288,6 +3298,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
.bdrv_attach_aio_context = sd_attach_aio_context,
.create_opts = &sd_create_opts,
+ .strong_runtime_opts = sd_strong_runtime_opts,
};
static BlockDriver bdrv_sheepdog_unix = {
@@ -3325,6 +3336,7 @@ static BlockDriver bdrv_sheepdog_unix = {
.bdrv_attach_aio_context = sd_attach_aio_context,
.create_opts = &sd_create_opts,
+ .strong_runtime_opts = sd_strong_runtime_opts,
};
static void bdrv_sheepdog_init(void)
diff --git a/block/ssh.c b/block/ssh.c
index 7fbc27abdf..350ad463b2 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -1266,6 +1266,17 @@ static int coroutine_fn ssh_co_truncate(BlockDriverState *bs, int64_t offset,
return ssh_grow_file(s, offset, errp);
}
+static const char *const ssh_strong_runtime_opts[] = {
+ "host",
+ "port",
+ "path",
+ "user",
+ "host_key_check",
+ "server.",
+
+ NULL
+};
+
static BlockDriver bdrv_ssh = {
.format_name = "ssh",
.protocol_name = "ssh",
@@ -1282,6 +1293,7 @@ static BlockDriver bdrv_ssh = {
.bdrv_co_truncate = ssh_co_truncate,
.bdrv_co_flush_to_disk = ssh_co_flush,
.create_opts = &ssh_create_opts,
+ .strong_runtime_opts = ssh_strong_runtime_opts,
};
static void bdrv_ssh_init(void)
diff --git a/block/throttle.c b/block/throttle.c
index 636c9764aa..f64dcc27b9 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -227,6 +227,12 @@ static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs)
atomic_dec(&tgm->io_limits_disabled);
}
+static const char *const throttle_strong_runtime_opts[] = {
+ QEMU_OPT_THROTTLE_GROUP_NAME,
+
+ NULL
+};
+
static BlockDriver bdrv_throttle = {
.format_name = "throttle",
.instance_size = sizeof(ThrottleGroupMember),
@@ -259,6 +265,7 @@ static BlockDriver bdrv_throttle = {
.bdrv_co_drain_end = throttle_co_drain_end,
.is_filter = true,
+ .strong_runtime_opts = throttle_strong_runtime_opts,
};
static void bdrv_throttle_init(void)
diff --git a/block/vpc.c b/block/vpc.c
index 717809cc24..b5d8f86422 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1214,6 +1214,12 @@ static QemuOptsList vpc_create_opts = {
}
};
+static const char *const vpc_strong_runtime_opts[] = {
+ VPC_OPT_SIZE_CALC,
+
+ NULL
+};
+
static BlockDriver bdrv_vpc = {
.format_name = "vpc",
.instance_size = sizeof(BDRVVPCState),
@@ -1234,6 +1240,7 @@ static BlockDriver bdrv_vpc = {
.create_opts = &vpc_create_opts,
.bdrv_has_zero_init = vpc_has_zero_init,
+ .strong_runtime_opts = vpc_strong_runtime_opts,
};
static void bdrv_vpc_init(void)
diff --git a/block/vvfat.c b/block/vvfat.c
index fc41841a5c..14c0644d1c 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3244,6 +3244,16 @@ static void vvfat_close(BlockDriverState *bs)
}
}
+static const char *const vvfat_strong_runtime_opts[] = {
+ "dir",
+ "fat-type",
+ "floppy",
+ "label",
+ "rw",
+
+ NULL
+};
+
static BlockDriver bdrv_vvfat = {
.format_name = "vvfat",
.protocol_name = "fat",
@@ -3258,6 +3268,8 @@ static BlockDriver bdrv_vvfat = {
.bdrv_co_preadv = vvfat_co_preadv,
.bdrv_co_pwritev = vvfat_co_pwritev,
.bdrv_co_block_status = vvfat_co_block_status,
+
+ .strong_runtime_opts = vvfat_strong_runtime_opts,
};
static void bdrv_vvfat_init(void)
diff --git a/block/vxhs.c b/block/vxhs.c
index 0cb0a007e9..2e18229ba4 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -556,6 +556,16 @@ static int64_t vxhs_getlength(BlockDriverState *bs)
return vdisk_size;
}
+static const char *const vxhs_strong_runtime_opts[] = {
+ VXHS_OPT_VDISK_ID,
+ "tls-creds",
+ VXHS_OPT_HOST,
+ VXHS_OPT_PORT,
+ VXHS_OPT_SERVER".",
+
+ NULL
+};
+
static BlockDriver bdrv_vxhs = {
.format_name = "vxhs",
.protocol_name = "vxhs",
@@ -567,6 +577,7 @@ static BlockDriver bdrv_vxhs = {
.bdrv_getlength = vxhs_getlength,
.bdrv_aio_preadv = vxhs_aio_preadv,
.bdrv_aio_pwritev = vxhs_aio_pwritev,
+ .strong_runtime_opts = vxhs_strong_runtime_opts,
};
static void bdrv_vxhs_init(void)
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 22/31] block: Add BlockDriver.bdrv_gather_child_options
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (20 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 21/31] block: Add strong_runtime_opts to BlockDriver Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-09-05 12:42 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 23/31] block: Generically refresh runtime options Max Reitz
` (10 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Some follow-up patches will rework the way bs->full_open_options is
refreshed in bdrv_refresh_filename(). The new implementation will remove
the need for the block drivers' bdrv_refresh_filename() implementations
to set bs->full_open_options; instead, it will be generic and use static
information from each block driver.
However, by implementing bdrv_gather_child_options(), block drivers will
still be able to override the way the full_open_options of their
children are incorporated into their own.
We need to implement this function for VMDK because we have to prevent
the generic implementation from gathering the options of all children:
It is not possible to specify options for the extents through the
runtime options.
For quorum, the child names that would be used by the generic
implementation and the ones that we actually (currently) want to use
differ. See quorum_gather_child_options() for more information.
Note that both of these are cases which are not ideal: In case of VMDK
it would probably be nice to be able to specify options for all extents.
In case of quorum, the current runtime option structure is simply broken
and needs to be fixed (but that is left for another patch).
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/block/block_int.h | 24 +++++++++++++++++++++++
block/quorum.c | 40 +++++++++++++++++++++++++++++++++++++++
block/vmdk.c | 19 +++++++++++++++++++
3 files changed, 83 insertions(+)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1f53adfd96..adaaf2572c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -141,6 +141,30 @@ 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.
+ *
+ * @backing_overridden is true when bs->backing seems not to be
+ * the child that would result from opening bs->backing_file.
+ * Therefore, if it is true, the backing child's options should be
+ * gathered; otherwise, there is no need since the backing child
+ * is the one implied by the image header.
+ *
+ * Note that ideally this function would not be needed. Every
+ * block driver which implements it is probably doing something
+ * shady regarding its runtime option structure.
+ */
+ void (*bdrv_gather_child_options)(BlockDriverState *bs, QDict *target,
+ bool backing_overridden);
+
/*
* Returns an allocated string which is the directory name of this BDS: It
* will be used to make relative filenames absolute by prepending this
diff --git a/block/quorum.c b/block/quorum.c
index fdc0e7d16d..f464260044 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1101,6 +1101,45 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
bs->full_open_options = opts;
}
+static void quorum_gather_child_options(BlockDriverState *bs, QDict *target,
+ bool backing_overridden)
+{
+ BDRVQuorumState *s = bs->opaque;
+ QList *children_list;
+ int i;
+
+ /*
+ * The generic implementation for gathering child options in
+ * bdrv_refresh_filename() would use the names of the children
+ * as specified for bdrv_open_child() or bdrv_attach_child(),
+ * which is "children.%u" with %u being a value
+ * (s->next_child_index) that is incremented each time a new child
+ * is added (and never decremented). Since children can be
+ * deleted at runtime, there may be gaps in that enumeration.
+ * When creating a new quorum BDS and specifying the children for
+ * it through runtime options, the enumeration used there may not
+ * have any gaps, though.
+ *
+ * Therefore, we have to create a new gap-less enumeration here
+ * (which we can achieve by simply putting all of the children's
+ * full_open_options into a QList).
+ *
+ * XXX: Note that there are issues with the current child option
+ * structure quorum uses (such as the fact that children do
+ * not really have unique permanent names). Therefore, this
+ * is going to have to change in the future and ideally we
+ * want quorum to be covered by the generic implementation.
+ */
+
+ children_list = qlist_new();
+ qdict_put(target, "children", children_list);
+
+ for (i = 0; i < s->num_children; i++) {
+ qlist_append(children_list,
+ qobject_ref(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
@@ -1128,6 +1167,7 @@ static BlockDriver bdrv_quorum = {
.bdrv_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 1b1a829fa1..399ab0fdd0 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -27,6 +27,7 @@
#include "qapi/error.h"
#include "block/block_int.h"
#include "sysemu/block-backend.h"
+#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
@@ -2356,6 +2357,23 @@ static int vmdk_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return 0;
}
+static void vmdk_gather_child_options(BlockDriverState *bs, QDict *target,
+ bool backing_overridden)
+{
+ /* No children but file and backing can be explicitly specified (TODO) */
+ qdict_put(target, "file",
+ qobject_ref(bs->file->bs->full_open_options));
+
+ if (backing_overridden) {
+ if (bs->backing) {
+ qdict_put(target, "backing",
+ qobject_ref(bs->backing->bs->full_open_options));
+ } else {
+ qdict_put_null(target, "backing");
+ }
+ }
+}
+
static QemuOptsList vmdk_create_opts = {
.name = "vmdk-create-opts",
.head = QTAILQ_HEAD_INITIALIZER(vmdk_create_opts.head),
@@ -2426,6 +2444,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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 23/31] block: Generically refresh runtime options
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (21 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 22/31] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-09-05 12:46 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 24/31] block: Purify .bdrv_refresh_filename() Max Reitz
` (9 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Instead of having every block driver which implements
bdrv_refresh_filename() copy all of the strong 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 | 2 +-
2 files changed, 116 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 1fdbf627e0..0821efbb2c 100644
--- a/block.c
+++ b/block.c
@@ -5159,6 +5159,92 @@ out:
return to_replace_bs;
}
+/**
+ * Iterates through the list of runtime option keys that are said to
+ * be "strong" for a BDS. An option is called "strong" if it changes
+ * a BDS's data. For example, the null block driver's "size" and
+ * "read-zeroes" options are strong, 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 strong.
+ */
+static const char *const *strong_options(BlockDriverState *bs,
+ const char *const *curopt)
+{
+ static const char *const global_options[] = {
+ "driver", "filename", NULL
+ };
+
+ if (!curopt) {
+ return &global_options[0];
+ }
+
+ curopt++;
+ if (curopt == &global_options[ARRAY_SIZE(global_options) - 1] && bs->drv) {
+ curopt = bs->drv->strong_runtime_opts;
+ }
+
+ return (curopt && *curopt) ? curopt : NULL;
+}
+
+/**
+ * Copies all strong runtime options from bs->options to the given
+ * QDict. The set of strong option keys is determined by invoking
+ * strong_options().
+ *
+ * Returns true iff any strong 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 strong options prevents the generation of
+ * a plain filename.
+ */
+static bool append_strong_runtime_options(QDict *d, BlockDriverState *bs)
+{
+ bool found_any = false;
+ const char *const *option_name = NULL;
+
+ if (!bs->drv) {
+ return false;
+ }
+
+ while ((option_name = strong_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;
+ }
+
+ qdict_put_obj(d, *option_name, qobject_ref(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)) {
+ qdict_put_obj(d, qdict_entry_key(entry),
+ qobject_ref(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;
@@ -5349,9 +5435,37 @@ void bdrv_refresh_filename(BlockDriverState *bs)
bs->full_open_options = opts;
}
+ /* Gather the options QDict */
+ opts = qdict_new();
+ append_strong_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, backing_overridden);
+ } else {
+ QLIST_FOREACH(child, &bs->children, next) {
+ if (child->role == &child_backing && !backing_overridden) {
+ /* We can skip the backing BDS if it has not been overridden */
+ continue;
+ }
+
+ qdict_put(opts, child->name,
+ qobject_ref(child->bs->full_open_options));
+ }
+
+ if (backing_overridden && !bs->backing) {
+ /* Force no backing file */
+ qdict_put_null(opts, "backing");
+ }
+ }
+
+ qobject_unref(bs->full_open_options);
+ bs->full_open_options = opts;
+
if (bs->exact_filename[0]) {
pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
- } else if (bs->full_open_options) {
+ } else {
QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
snprintf(bs->filename, sizeof(bs->filename), "json:%s",
qstring_get_str(json));
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index 1d0b2475cc..46e6a60510 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -22,7 +22,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=t.IMGFMT.b
=== Nodes without a common directory ===
-image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "blkverify": false, "rewrite-corrupted": false, "vote-threshold": 1}}
+image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "vote-threshold": 1}}
file format: IMGFMT
virtual size: 64M (67108864 bytes)
backing file: t.IMGFMT.base (cannot determine actual path)
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 24/31] block: Purify .bdrv_refresh_filename()
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (22 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 23/31] block: Generically refresh runtime options Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-09-05 12:49 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 25/31] block: Do not copy exact_filename from format file Max Reitz
` (8 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
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.
This completely obsoletes blklogwrite's implementation of
.bdrv_refresh_filename().
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
include/block/block_int.h | 6 +-
block.c | 145 ++++++--------------------------------
block/blkdebug.c | 54 ++++++--------
block/blklogwrites.c | 23 ------
block/blkverify.c | 16 +----
block/commit.c | 2 +-
block/mirror.c | 2 +-
block/nbd.c | 23 +-----
block/nfs.c | 36 +---------
block/null.c | 22 +++---
block/nvme.c | 22 +++---
block/quorum.c | 30 --------
12 files changed, 80 insertions(+), 301 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index adaaf2572c..90217512b5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -139,7 +139,11 @@ struct BlockDriver {
Error **errp);
int (*bdrv_make_empty)(BlockDriverState *bs);
- void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
+ /*
+ * Refreshes the bs->exact_filename field. If that is impossible,
+ * bs->exact_filename has to be left empty.
+ */
+ void (*bdrv_refresh_filename)(BlockDriverState *bs);
/*
* Gathers the open options for all children into @target.
diff --git a/block.c b/block.c
index 0821efbb2c..2d3e07b7a7 100644
--- a/block.c
+++ b/block.c
@@ -5245,47 +5245,6 @@ static bool append_strong_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;
- }
-
- qdict_put_obj(d, qdict_entry_key(entry),
- qobject_ref(qdict_entry_value(entry)));
- found_any = true;
- }
-
- return found_any;
-}
-
/* Note: This function may return false positives; it may return true
* even if opening the backing file specified by bs's image header
* would result in exactly bs->backing. */
@@ -5319,6 +5278,8 @@ void bdrv_refresh_filename(BlockDriverState *bs)
BdrvChild *child;
QDict *opts;
bool backing_overridden;
+ bool generate_json_filename; /* Whether our default implementation should
+ fill exact_filename (false) or not (true) */
if (!drv) {
return;
@@ -5354,90 +5315,10 @@ void bdrv_refresh_filename(BlockDriverState *bs)
backing_overridden = false;
}
- 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) {
- qobject_unref(bs->full_open_options);
- bs->full_open_options = NULL;
- }
-
- opts = qdict_new();
- append_open_options(opts, bs);
- drv->bdrv_refresh_filename(bs, opts);
- qobject_unref(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) {
- qobject_unref(bs->full_open_options);
- bs->full_open_options = NULL;
- }
-
- opts = qdict_new();
- has_open_options = append_open_options(opts, bs);
- has_open_options |= 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);
- qdict_put(opts, "file",
- qobject_ref(bs->file->bs->full_open_options));
-
- if (bs->backing) {
- qdict_put(opts, "backing",
- qobject_ref(bs->backing->bs->full_open_options));
- } else if (backing_overridden) {
- qdict_put_null(opts, "backing");
- }
-
- bs->full_open_options = opts;
- } else {
- qobject_unref(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_strong_runtime_options(opts, bs);
+ generate_json_filename = append_strong_runtime_options(opts, bs);
+ generate_json_filename |= backing_overridden;
if (drv->bdrv_gather_child_options) {
/* Some block drivers may not want to present all of their children's
@@ -5463,6 +5344,24 @@ void bdrv_refresh_filename(BlockDriverState *bs)
qobject_unref(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 71b4275b98..1ea835c2b9 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -811,51 +811,37 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
return bdrv_getlength(bs->file->bs);
}
-static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
+static void blkdebug_refresh_filename(BlockDriverState *bs)
{
BDRVBlkdebugState *s = bs->opaque;
- QDict *opts;
const QDictEntry *e;
- bool force_json = false;
-
- for (e = qdict_first(options); e; e = qdict_next(options, e)) {
- if (strcmp(qdict_entry_key(e), "config") &&
- strcmp(qdict_entry_key(e), "x-image"))
- {
- force_json = true;
- break;
- }
- }
+ int ret;
- if (force_json && !bs->file->bs->full_open_options) {
- /* The config file cannot be recreated, so creating a plain filename
- * is impossible */
+ if (!bs->file->bs->exact_filename[0]) {
return;
}
- if (!force_json && bs->file->bs->exact_filename[0]) {
- int ret = snprintf(bs->exact_filename, sizeof(bs->exact_filename),
- "blkdebug:%s:%s", s->config_file ?: "",
- bs->file->bs->exact_filename);
- if (ret >= sizeof(bs->exact_filename)) {
- /* An overflow makes the filename unusable, so do not report any */
- bs->exact_filename[0] = 0;
+ for (e = qdict_first(bs->full_open_options); e;
+ e = qdict_next(bs->full_open_options, e))
+ {
+ /* Real child options are under "image", but "x-image" may
+ * contain a filename */
+ if (strcmp(qdict_entry_key(e), "config") &&
+ strcmp(qdict_entry_key(e), "image") &&
+ strcmp(qdict_entry_key(e), "x-image") &&
+ strcmp(qdict_entry_key(e), "driver"))
+ {
+ return;
}
}
- opts = qdict_new();
- qdict_put_str(opts, "driver", "blkdebug");
-
- qdict_put(opts, "image", qobject_ref(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")) {
- qdict_put_obj(opts, qdict_entry_key(e),
- qobject_ref(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/blklogwrites.c b/block/blklogwrites.c
index 1c8e200326..eb2b4901a5 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -280,28 +280,6 @@ static int64_t blk_log_writes_getlength(BlockDriverState *bs)
return bdrv_getlength(bs->file->bs);
}
-static void blk_log_writes_refresh_filename(BlockDriverState *bs,
- QDict *options)
-{
- BDRVBlkLogWritesState *s = bs->opaque;
-
- if (bs->file->bs->full_open_options
- && s->log_file->bs->full_open_options)
- {
- QDict *opts = qdict_new();
- qdict_put_str(opts, "driver", "blklogwrites");
-
- qobject_ref(bs->file->bs->full_open_options);
- qdict_put_obj(opts, "file", QOBJECT(bs->file->bs->full_open_options));
- qobject_ref(s->log_file->bs->full_open_options);
- qdict_put_obj(opts, "log",
- QOBJECT(s->log_file->bs->full_open_options));
- qdict_put_int(opts, "log-sector-size", s->sectorsize);
-
- bs->full_open_options = opts;
- }
-}
-
static void blk_log_writes_child_perm(BlockDriverState *bs, BdrvChild *c,
const BdrvChildRole *role,
BlockReopenQueue *ro_q,
@@ -532,7 +510,6 @@ static BlockDriver bdrv_blk_log_writes = {
.bdrv_open = blk_log_writes_open,
.bdrv_close = blk_log_writes_close,
.bdrv_getlength = blk_log_writes_getlength,
- .bdrv_refresh_filename = blk_log_writes_refresh_filename,
.bdrv_child_perm = blk_log_writes_child_perm,
.bdrv_refresh_limits = blk_log_writes_refresh_limits,
diff --git a/block/blkverify.c b/block/blkverify.c
index 3c7d4c8729..3ff77ff49a 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -281,24 +281,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");
-
- qdict_put(opts, "raw",
- qobject_ref(bs->file->bs->full_open_options));
- qdict_put(opts, "test",
- qobject_ref(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 26db55d800..14788b0708 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -232,7 +232,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 e473c434ad..b4287a1e2b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1414,7 +1414,7 @@ static int coroutine_fn bdrv_mirror_top_pdiscard(BlockDriverState *bs,
NULL, 0);
}
-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 5b4e4cf1db..fcc9e897e5 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -513,12 +513,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) {
@@ -531,8 +528,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);
@@ -546,22 +541,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 6985a44b89..531903610b 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -799,14 +799,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),
@@ -824,35 +819,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 07acb4f732..c681e7e20b 100644
--- a/block/null.c
+++ b/block/null.c
@@ -243,17 +243,23 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs,
return ret;
}
-static void null_refresh_filename(BlockDriverState *bs, QDict *opts)
+static void null_refresh_filename(BlockDriverState *bs)
{
- 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 = qobject_ref(opts);
+ snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
+ bs->drv->format_name);
}
static const char *const null_strong_runtime_opts[] = {
diff --git a/block/nvme.c b/block/nvme.c
index b0548e4e37..9e893ab5fa 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1071,17 +1071,23 @@ static int nvme_reopen_prepare(BDRVReopenState *reopen_state,
return 0;
}
-static void nvme_refresh_filename(BlockDriverState *bs, QDict *opts)
+static void nvme_refresh_filename(BlockDriverState *bs)
{
- 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 = qobject_ref(opts);
+ snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
+ bs->drv->format_name);
}
static void nvme_refresh_limits(BlockDriverState *bs, Error **errp)
diff --git a/block/quorum.c b/block/quorum.c
index f464260044..9f60b60a56 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1072,35 +1072,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++) {
- qlist_append(children,
- qobject_ref(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,
bool backing_overridden)
{
@@ -1166,7 +1137,6 @@ static BlockDriver bdrv_quorum = {
.bdrv_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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 25/31] block: Do not copy exact_filename from format file
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (23 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 24/31] block: Purify .bdrv_refresh_filename() Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-09-05 13:23 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 26/31] block/nvme: Fix bdrv_refresh_filename() Max Reitz
` (7 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
If a format BDS's file BDS is in turn a format BDS, we cannot simply use
the same filename, because when opening a BDS tree based on a filename
alone, qemu will create only one format node on top of one protocol node
(disregarding a potential backing file).
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index 2d3e07b7a7..c4aec7ea65 100644
--- a/block.c
+++ b/block.c
@@ -5355,9 +5355,21 @@ 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 (strong) 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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 26/31] block/nvme: Fix bdrv_refresh_filename()
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (24 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 25/31] block: Do not copy exact_filename from format file Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-28 13:50 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 27/31] block/curl: Harmonize option defaults Max Reitz
` (6 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Currently, nvme's bdrv_refresh_filename() is an exact copy of null's
implementation. However, for null, "null-co://" and "null-aio://" are
indeed valid filenames -- for nvme, they are not, as a device address is
still required.
The correct implementation should generate a filename of the form
"nvme://[PCI address]/[namespace]" (as the comment above
nvme_parse_filename() describes).
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/nvme.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index 9e893ab5fa..d8c55faff3 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -111,6 +111,9 @@ typedef struct {
/* Total size of mapped qiov, accessed under dma_map_lock */
int dma_map_count;
+
+ /* PCI address (required for nvme_refresh_filename()) */
+ char *device;
} BDRVNVMeState;
#define NVME_BLOCK_OPT_DEVICE "device"
@@ -558,6 +561,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
qemu_co_mutex_init(&s->dma_map_lock);
qemu_co_queue_init(&s->dma_flush_queue);
+ s->device = g_strdup(device);
s->nsid = namespace;
s->aio_context = bdrv_get_aio_context(bs);
ret = event_notifier_init(&s->irq_notifier, 0);
@@ -743,6 +747,8 @@ static void nvme_close(BlockDriverState *bs)
false, NULL, NULL);
qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
qemu_vfio_close(s->vfio);
+
+ g_free(s->device);
}
static int nvme_file_open(BlockDriverState *bs, QDict *options, int flags,
@@ -1073,21 +1079,10 @@ static int nvme_reopen_prepare(BDRVReopenState *reopen_state,
static void nvme_refresh_filename(BlockDriverState *bs)
{
- 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;
- }
- }
+ BDRVNVMeState *s = bs->opaque;
- snprintf(bs->exact_filename, sizeof(bs->exact_filename), "%s://",
- bs->drv->format_name);
+ snprintf(bs->exact_filename, sizeof(bs->exact_filename), "nvme://%s/%i",
+ s->device, s->nsid);
}
static void nvme_refresh_limits(BlockDriverState *bs, Error **errp)
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 27/31] block/curl: Harmonize option defaults
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (25 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 26/31] block/nvme: Fix bdrv_refresh_filename() Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 28/31] block/curl: Implement bdrv_refresh_filename() Max Reitz
` (5 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Both of the defaults we currently have in the curl driver are named
based on a slightly different schema, let's unify that and call both
CURL_BLOCK_OPT_${NAME}_DEFAULT.
While at it, we can add a macro for the third option for which a default
exists, namely "sslverify".
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/curl.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/block/curl.c b/block/curl.c
index 86e45cfde8..13fca02aba 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -73,8 +73,6 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
#define CURL_NUM_STATES 8
#define CURL_NUM_ACB 8
-#define READ_AHEAD_DEFAULT (256 * 1024)
-#define CURL_TIMEOUT_DEFAULT 5
#define CURL_TIMEOUT_MAX 10000
#define CURL_BLOCK_OPT_URL "url"
@@ -88,6 +86,10 @@ static CURLMcode __curl_multi_socket_action(CURLM *multi_handle,
#define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
#define CURL_BLOCK_OPT_PROXY_PASSWORD_SECRET "proxy-password-secret"
+#define CURL_BLOCK_OPT_READAHEAD_DEFAULT (256 * 1024)
+#define CURL_BLOCK_OPT_SSLVERIFY_DEFAULT true
+#define CURL_BLOCK_OPT_TIMEOUT_DEFAULT 5
+
struct BDRVCURLState;
static bool libcurl_initialized;
@@ -706,7 +708,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
}
s->readahead_size = qemu_opt_get_size(opts, CURL_BLOCK_OPT_READAHEAD,
- READ_AHEAD_DEFAULT);
+ CURL_BLOCK_OPT_READAHEAD_DEFAULT);
if ((s->readahead_size & 0x1ff) != 0) {
error_setg(errp, "HTTP_READAHEAD_SIZE %zd is not a multiple of 512",
s->readahead_size);
@@ -714,13 +716,14 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags,
}
s->timeout = qemu_opt_get_number(opts, CURL_BLOCK_OPT_TIMEOUT,
- CURL_TIMEOUT_DEFAULT);
+ CURL_BLOCK_OPT_TIMEOUT_DEFAULT);
if (s->timeout > CURL_TIMEOUT_MAX) {
error_setg(errp, "timeout parameter is too large or negative");
goto out_noclean;
}
- s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
+ s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY,
+ CURL_BLOCK_OPT_SSLVERIFY_DEFAULT);
cookie = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE);
cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 28/31] block/curl: Implement bdrv_refresh_filename()
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (26 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 27/31] block/curl: Harmonize option defaults Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 29/31] block/null: Generate filename even with latency-ns Max Reitz
` (4 subsequent siblings)
32 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/curl.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/block/curl.c b/block/curl.c
index 13fca02aba..fcb8ad501b 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -961,6 +961,23 @@ static int64_t curl_getlength(BlockDriverState *bs)
return s->len;
}
+static void curl_refresh_filename(BlockDriverState *bs)
+{
+ BDRVCURLState *s = bs->opaque;
+
+ /* "readahead" and "timeout" do not change the guest-visible data,
+ * so ignore them */
+ if (s->sslverify != CURL_BLOCK_OPT_SSLVERIFY_DEFAULT ||
+ s->cookie || s->username || s->password || s->proxyusername ||
+ s->proxypassword)
+ {
+ return;
+ }
+
+ pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), s->url);
+}
+
+
static const char *const curl_strong_runtime_opts[] = {
CURL_BLOCK_OPT_URL,
CURL_BLOCK_OPT_SSLVERIFY,
@@ -989,6 +1006,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,
.strong_runtime_opts = curl_strong_runtime_opts,
};
@@ -1007,6 +1025,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,
.strong_runtime_opts = curl_strong_runtime_opts,
};
@@ -1025,6 +1044,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,
.strong_runtime_opts = curl_strong_runtime_opts,
};
@@ -1043,6 +1063,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,
.strong_runtime_opts = curl_strong_runtime_opts,
};
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 29/31] block/null: Generate filename even with latency-ns
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (27 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 28/31] block/curl: Implement bdrv_refresh_filename() Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-28 13:33 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 30/31] block: BDS options may lack the "driver" option Max Reitz
` (3 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
While we cannot represent the latency-ns option in a filename, it is not
a strong 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 c681e7e20b..24c52a15de 100644
--- a/block/null.c
+++ b/block/null.c
@@ -252,7 +252,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.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 30/31] block: BDS options may lack the "driver" option
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (28 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 29/31] block/null: Generate filename even with latency-ns Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-08-28 14:44 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 31/31] iotests: Test json:{} filenames of internal BDSs Max Reitz
` (2 subsequent siblings)
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
When BDSs are created by qemu itself (e.g. as filters in block jobs),
they may not have a "driver" option in their options QDict. When
generating a json:{} filename, however, it must always be present.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/block.c b/block.c
index c4aec7ea65..5118d992c3 100644
--- a/block.c
+++ b/block.c
@@ -5242,6 +5242,12 @@ static bool append_strong_runtime_options(QDict *d, BlockDriverState *bs)
}
}
+ if (!qdict_haskey(d, "driver")) {
+ /* Drivers created with bdrv_new_open_driver() may not have a
+ * @driver option. Add it here. */
+ qdict_put_str(d, "driver", bs->drv->format_name);
+ }
+
return found_any;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH for-3.1 v10 31/31] iotests: Test json:{} filenames of internal BDSs
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (29 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 30/31] block: BDS options may lack the "driver" option Max Reitz
@ 2018-08-09 21:35 ` Max Reitz
2018-09-05 14:44 ` Alberto Garcia
2018-08-15 3:43 ` [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues no-reply
2018-09-10 15:18 ` Kevin Wolf
32 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-09 21:35 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf, Eric Blake, Alberto Garcia
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/224 | 139 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/224.out | 18 +++++
tests/qemu-iotests/group | 1 +
3 files changed, 158 insertions(+)
create mode 100755 tests/qemu-iotests/224
create mode 100644 tests/qemu-iotests/224.out
diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
new file mode 100755
index 0000000000..f1b7aa801b
--- /dev/null
+++ b/tests/qemu-iotests/224
@@ -0,0 +1,139 @@
+#!/usr/bin/env python
+#
+# Test json:{} filenames with qemu-internal BDSs
+# (the one of commit, to be precise)
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Max Reitz <mreitz@redhat.com>
+
+import iotests
+from iotests import log, qemu_img, qemu_io_silent, filter_testfiles, \
+ filter_imgfmt
+import json
+
+# Need backing file support (for arbitrary backing formats)
+iotests.verify_image_format(supported_fmts=['qcow2', 'qcow', 'qed'])
+iotests.verify_platform(['linux'])
+
+
+# There are two variations of this test:
+# (1) We do not set filter_node_name. In that case, the commit_top
+# driver should not appear anywhere.
+# (2) We do set filter_node_name. In that case, it should appear.
+#
+# This for loop executes both.
+for filter_node_name in False, True:
+ log('')
+ log('--- filter_node_name: %s ---' % filter_node_name)
+ log('')
+
+ with iotests.FilePath('base.img') as base_img_path, \
+ iotests.FilePath('mid.img') as mid_img_path, \
+ iotests.FilePath('top.img') as top_img_path, \
+ iotests.VM() as vm:
+
+ assert qemu_img('create', '-f', iotests.imgfmt,
+ base_img_path, '64M') == 0
+ assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+ mid_img_path) == 0
+ assert qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
+ top_img_path) == 0
+
+ # Something to commit
+ assert qemu_io_silent(mid_img_path, '-c', 'write -P 1 0 1M') == 0
+
+ vm.launch()
+
+ # Change the bottom-most image's backing file (to null-co://)
+ # to enforce json:{} filenames
+ vm.qmp_log('blockdev-add',
+ node_name='top',
+ driver=iotests.imgfmt,
+ file={
+ 'driver': 'file',
+ 'filename': top_img_path
+ },
+ backing={
+ 'node-name': 'mid',
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': mid_img_path
+ },
+ 'backing': {
+ 'node-name': 'base',
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': base_img_path
+ },
+ 'backing': {
+ 'driver': 'null-co'
+ }
+ }
+ },
+ filters=[filter_testfiles, filter_imgfmt])
+
+ # As long as block-commit does not accept node names, we have to
+ # get our mid/base filenames here
+ mid_name = vm.node_info('mid')['image']['filename']
+ base_name = vm.node_info('base')['image']['filename']
+
+ #assert mid_name[:5] == 'json:'
+ #assert base_name[:5] == 'json:'
+
+ # Start the block job
+ if filter_node_name:
+ vm.qmp_log('block-commit',
+ job_id='commit',
+ device='top',
+ filter_node_name='filter_node',
+ top=mid_name,
+ base=base_name,
+ speed=1,
+ filters=[filter_testfiles, filter_imgfmt])
+ else:
+ vm.qmp_log('block-commit',
+ job_id='commit',
+ device='top',
+ top=mid_name,
+ base=base_name,
+ speed=1,
+ filters=[filter_testfiles, filter_imgfmt])
+
+ vm.qmp_log('job-pause', id='commit')
+
+ # Get and parse top's json:{} filename
+ top_name = vm.node_info('top')['image']['filename']
+
+ vm.shutdown()
+
+ #assert top_name[:5] == 'json:'
+ #top_options = json.loads(top_name[5:])
+
+ #if filter_node_name:
+ # # This should be present and set
+ # assert top_options['backing']['driver'] == 'commit_top'
+ # # And the mid image is commit_top's backing image
+ # mid_options = top_options['backing']['backing']
+ #else:
+ # # The mid image should appear as the immediate backing BDS
+ # # of top
+ # mid_options = top_options['backing']
+
+ #assert mid_options['driver'] == iotests.imgfmt
+ #assert mid_options['file']['filename'] == mid_img_path
diff --git a/tests/qemu-iotests/224.out b/tests/qemu-iotests/224.out
new file mode 100644
index 0000000000..0230ae2c54
--- /dev/null
+++ b/tests/qemu-iotests/224.out
@@ -0,0 +1,18 @@
+
+--- filter_node_name: False ---
+
+{'execute': 'blockdev-add', 'arguments': {'backing': {'backing': {'backing': {'driver': 'null-co'}, 'node-name': 'base', 'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-base.img'}}, 'node-name': 'mid', 'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-mid.img'}}, 'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-top.img'}, 'node_name': 'top'}}
+{u'return': {}}
+{'execute': 'block-commit', 'arguments': {'device': 'top', 'top': u'json:{"backing": {"backing": {"driver": "null-co"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}}', 'base': u'json:{"backing": {"driver": "null-co"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}}', 'speed': 1, 'job_id': 'commit'}}
+{u'return': {}}
+{'execute': 'job-pause', 'arguments': {'id': 'commit'}}
+{u'return': {}}
+
+--- filter_node_name: True ---
+
+{'execute': 'blockdev-add', 'arguments': {'backing': {'backing': {'backing': {'driver': 'null-co'}, 'node-name': 'base', 'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-base.img'}}, 'node-name': 'mid', 'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-mid.img'}}, 'driver': 'IMGFMT', 'file': {'driver': 'file', 'filename': 'TEST_DIR/PID-top.img'}, 'node_name': 'top'}}
+{u'return': {}}
+{'execute': 'block-commit', 'arguments': {'job_id': 'commit', 'top': u'json:{"backing": {"backing": {"driver": "null-co"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-mid.img"}}', 'base': u'json:{"backing": {"driver": "null-co"}, "driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/PID-base.img"}}', 'device': 'top', 'filter_node_name': 'filter_node', 'speed': 1}}
+{u'return': {}}
+{'execute': 'job-pause', 'arguments': {'id': 'commit'}}
+{u'return': {}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5bfe2e246d..3bf4f13527 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -222,6 +222,7 @@
221 rw auto quick
222 rw auto quick
223 rw auto quick
+224 rw auto quick
225 rw auto quick
226 auto quick
227 auto quick
--
2.17.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (30 preceding siblings ...)
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 31/31] iotests: Test json:{} filenames of internal BDSs Max Reitz
@ 2018-08-15 3:43 ` no-reply
2018-08-16 0:01 ` Max Reitz
2018-09-10 15:18 ` Kevin Wolf
32 siblings, 1 reply; 67+ messages in thread
From: no-reply @ 2018-08-15 3:43 UTC (permalink / raw)
To: mreitz; +Cc: famz, qemu-block, kwolf, berto, qemu-devel
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180809213528.14738-1-mreitz@redhat.com
Subject: [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
4d2a6801e3 iotests: Test json:{} filenames of internal BDSs
92861b0d23 block: BDS options may lack the "driver" option
5e539a99ee block/null: Generate filename even with latency-ns
3011e18097 block/curl: Implement bdrv_refresh_filename()
c3d3ba04c4 block/curl: Harmonize option defaults
0f00168fdc block/nvme: Fix bdrv_refresh_filename()
10fd7a823b block: Do not copy exact_filename from format file
1705480785 block: Purify .bdrv_refresh_filename()
d5face8a37 block: Generically refresh runtime options
d83b993208 block: Add BlockDriver.bdrv_gather_child_options
38384107dc block: Add strong_runtime_opts to BlockDriver
b9b3310f5d iotests: Add quorum case to test 110
292e6e5c96 block: Use bdrv_dirname() for relative filenames
f1fd748aca block/nfs: Implement bdrv_dirname()
8dfbf896b6 block/nbd: Make bdrv_dirname() return NULL
a5187853f6 quorum: Make bdrv_dirname() return NULL
4f62d050cd blkverify: Make bdrv_dirname() return NULL
f4ae7225b0 block: Add bdrv_dirname()
9b5ad84b84 block: Fix bdrv_find_backing_image()
324ade1026 block: Add bdrv_make_absolute_filename()
68def7295f block: bdrv_get_full_backing_filename's ret. val.
e5e55fcbb3 block: bdrv_get_full_backing_filename_from_...'s ret. val.
b434ec5b34 block: Make path_combine() return the path
a3542a966b iotests: Add test for backing file overrides
6affaa70a0 iotests.py: Add node_info()
13d2e98fe3 iotests.py: Add filter_imgfmt()
a46abb22e3 block: Respect backing bs in bdrv_refresh_filename
8b0cd321f7 block: Add BDS.auto_backing_file
0b9c999e16 block: Skip implicit nodes for filename info
b4b7a10cc2 block: Use children list in bdrv_refresh_filename
92210c34b4 block: Use bdrv_refresh_filename() to pull
=== OUTPUT BEGIN ===
Checking PATCH 1/31: block: Use bdrv_refresh_filename() to pull...
Checking PATCH 2/31: block: Use children list in bdrv_refresh_filename...
Checking PATCH 3/31: block: Skip implicit nodes for filename info...
Checking PATCH 4/31: block: Add BDS.auto_backing_file...
Checking PATCH 5/31: block: Respect backing bs in bdrv_refresh_filename...
ERROR: return is not a function, parentheses are not required
#46: FILE: block.c:5192:
+ return (bs->auto_backing_file[0] != '\0');
total: 1 errors, 0 warnings, 136 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 6/31: iotests.py: Add filter_imgfmt()...
Checking PATCH 7/31: iotests.py: Add node_info()...
Checking PATCH 8/31: iotests: Add test for backing file overrides...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#10:
new file mode 100755
total: 0 errors, 1 warnings, 323 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 9/31: block: Make path_combine() return the path...
Checking PATCH 10/31: block: bdrv_get_full_backing_filename_from_...'s ret. val....
Checking PATCH 11/31: block: bdrv_get_full_backing_filename's ret. val....
Checking PATCH 12/31: block: Add bdrv_make_absolute_filename()...
Checking PATCH 13/31: block: Fix bdrv_find_backing_image()...
Checking PATCH 14/31: block: Add bdrv_dirname()...
Checking PATCH 15/31: blkverify: Make bdrv_dirname() return NULL...
Checking PATCH 16/31: quorum: Make bdrv_dirname() return NULL...
Checking PATCH 17/31: block/nbd: Make bdrv_dirname() return NULL...
Checking PATCH 18/31: block/nfs: Implement bdrv_dirname()...
Checking PATCH 19/31: block: Use bdrv_dirname() for relative filenames...
Checking PATCH 20/31: iotests: Add quorum case to test 110...
Checking PATCH 21/31: block: Add strong_runtime_opts to BlockDriver...
Checking PATCH 22/31: block: Add BlockDriver.bdrv_gather_child_options...
Checking PATCH 23/31: block: Generically refresh runtime options...
Checking PATCH 24/31: block: Purify .bdrv_refresh_filename()...
Checking PATCH 25/31: block: Do not copy exact_filename from format file...
Checking PATCH 26/31: block/nvme: Fix bdrv_refresh_filename()...
Checking PATCH 27/31: block/curl: Harmonize option defaults...
Checking PATCH 28/31: block/curl: Implement bdrv_refresh_filename()...
Checking PATCH 29/31: block/null: Generate filename even with latency-ns...
Checking PATCH 30/31: block: BDS options may lack the "driver" option...
Checking PATCH 31/31: iotests: Test json:{} filenames of internal BDSs...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#10:
new file mode 100755
total: 0 errors, 1 warnings, 164 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues
2018-08-15 3:43 ` [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues no-reply
@ 2018-08-16 0:01 ` Max Reitz
2018-08-16 6:02 ` Markus Armbruster
0 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-16 0:01 UTC (permalink / raw)
To: qemu-devel; +Cc: famz, qemu-block, kwolf, berto
[-- Attachment #1: Type: text/plain, Size: 821 bytes --]
On 2018-08-15 05:43, no-reply@patchew.org wrote:
> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
[...]
> === OUTPUT BEGIN ===
> Checking PATCH 1/31: block: Use bdrv_refresh_filename() to pull...
> Checking PATCH 2/31: block: Use children list in bdrv_refresh_filename...
> Checking PATCH 3/31: block: Skip implicit nodes for filename info...
> Checking PATCH 4/31: block: Add BDS.auto_backing_file...
> Checking PATCH 5/31: block: Respect backing bs in bdrv_refresh_filename...
> ERROR: return is not a function, parentheses are not required
> #46: FILE: block.c:5192:
> + return (bs->auto_backing_file[0] != '\0');
>
> total: 1 errors, 0 warnings, 136 lines checked
Sure, but I'd hate you personally if you omitted them.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues
2018-08-16 0:01 ` Max Reitz
@ 2018-08-16 6:02 ` Markus Armbruster
2018-08-17 20:03 ` Max Reitz
0 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2018-08-16 6:02 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, kwolf, berto, famz, qemu-block
Max Reitz <mreitz@redhat.com> writes:
> On 2018-08-15 05:43, no-reply@patchew.org wrote:
>> Hi,
>>
>> This series seems to have some coding style problems. See output below for
>> more information:
>
> [...]
>
>> === OUTPUT BEGIN ===
>> Checking PATCH 1/31: block: Use bdrv_refresh_filename() to pull...
>> Checking PATCH 2/31: block: Use children list in bdrv_refresh_filename...
>> Checking PATCH 3/31: block: Skip implicit nodes for filename info...
>> Checking PATCH 4/31: block: Add BDS.auto_backing_file...
>> Checking PATCH 5/31: block: Respect backing bs in bdrv_refresh_filename...
>> ERROR: return is not a function, parentheses are not required
>> #46: FILE: block.c:5192:
>> + return (bs->auto_backing_file[0] != '\0');
>>
>> total: 1 errors, 0 warnings, 136 lines checked
>
> Sure, but I'd hate you personally if you omitted them.
@@
expression E;
@@
- return (E);
+ return E;
You're welcome!
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 02/31] block: Use children list in bdrv_refresh_filename
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 02/31] block: Use children list in bdrv_refresh_filename Max Reitz
@ 2018-08-16 19:02 ` Eric Blake
2018-08-28 12:27 ` Alberto Garcia
1 sibling, 0 replies; 67+ messages in thread
From: Eric Blake @ 2018-08-16 19:02 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Alberto Garcia
On 08/09/2018 04:34 PM, Max Reitz wrote:
> 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, mirror, and blklogwrites.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block.c | 9 +++++----
> block/blklogwrites.c | 3 ---
> block/blkverify.c | 3 ---
> block/commit.c | 1 -
> block/mirror.c | 1 -
> block/quorum.c | 1 -
> 6 files changed, 5 insertions(+), 13 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues
2018-08-16 6:02 ` Markus Armbruster
@ 2018-08-17 20:03 ` Max Reitz
0 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-17 20:03 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, kwolf, berto, famz, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]
On 2018-08-16 08:02, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
>
>> On 2018-08-15 05:43, no-reply@patchew.org wrote:
>>> Hi,
>>>
>>> This series seems to have some coding style problems. See output below for
>>> more information:
>>
>> [...]
>>
>>> === OUTPUT BEGIN ===
>>> Checking PATCH 1/31: block: Use bdrv_refresh_filename() to pull...
>>> Checking PATCH 2/31: block: Use children list in bdrv_refresh_filename...
>>> Checking PATCH 3/31: block: Skip implicit nodes for filename info...
>>> Checking PATCH 4/31: block: Add BDS.auto_backing_file...
>>> Checking PATCH 5/31: block: Respect backing bs in bdrv_refresh_filename...
>>> ERROR: return is not a function, parentheses are not required
>>> #46: FILE: block.c:5192:
>>> + return (bs->auto_backing_file[0] != '\0');
>>>
>>> total: 1 errors, 0 warnings, 136 lines checked
>>
>> Sure, but I'd hate you personally if you omitted them.
>
> @@
> expression E;
> @@
> - return (E);
> + return E;
>
> You're welcome!
Well, I really don't like not putting parenthesis when returning
something that is not an identifier or a function call.
In fact, before I drop the parentheses, I'd drop the "!= '\0'", because
that is optional, too. (And then I could drop the parentheses anyway.)
Or write it with an explicit if-else (return true - return false).
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 01/31] block: Use bdrv_refresh_filename() to pull
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 01/31] block: Use bdrv_refresh_filename() to pull Max Reitz
@ 2018-08-28 12:13 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 12:13 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:34:58 PM CEST, Max Reitz wrote:
> Before this patch, bdrv_refresh_filename() is used in a pushing manner:
> Whenever the BDS graph is modified, the parents of the modified edges
> are supposed to be updated (recursively upwards). However, that is
> nonviable, considering that we want child changes not to concern
> parents.
>
> Also, in the long run we want a pull model anyway: Here, we would have a
> bdrv_filename() function which returns a BDS's filename, freshly
> constructed.
>
> This patch is an intermediate step. It adds bdrv_refresh_filename()
> calls before every place a BDS.filename value is used. The only
> exceptions are protocol drivers that use their own filename, which
> clearly would not profit from refreshing that filename before.
>
> Also, bdrv_get_encrypted_filename() is removed along the way (as a user
> of BDS.filename), since it is completely unused.
>
> In turn, all of the calls to bdrv_refresh_filename() before this patch
> are removed, because we no longer have to call this function on graph
> changes.
>
> 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] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 02/31] block: Use children list in bdrv_refresh_filename
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 02/31] block: Use children list in bdrv_refresh_filename Max Reitz
2018-08-16 19:02 ` Eric Blake
@ 2018-08-28 12:27 ` Alberto Garcia
1 sibling, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 12:27 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:34:59 PM CEST, Max Reitz wrote:
> 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, mirror, and blklogwrites.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 07/31] iotests.py: Add node_info()
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 07/31] iotests.py: Add node_info() Max Reitz
@ 2018-08-28 12:48 ` Alberto Garcia
2018-08-29 9:45 ` Max Reitz
0 siblings, 1 reply; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 12:48 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:04 PM CEST, Max Reitz wrote:
> This function queries a node; since we cannot do that right now, it
> executes query-named-block-nodes and returns the matching node's object.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 5c45788dac..6583001ac8 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -465,6 +465,16 @@ class VM(qtest.QEMUQtestMachine):
> else:
> iotests.log(ev)
>
> + def node_info(self, node_name, require_existence=True):
> + nodes = self.qmp('query-named-block-nodes')
> + for x in nodes['return']:
> + if x['node-name'] == node_name:
> + return x
> + if require_existence:
> + assert False
> + else:
> + return None
> +
I don't think you need the require_existence parameter here... if you
simply return None you'll get an error as soon as you try to use the
return value.
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 18/31] block/nfs: Implement bdrv_dirname()
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 18/31] block/nfs: Implement bdrv_dirname() Max Reitz
@ 2018-08-28 12:49 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 12:49 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:15 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] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 05/31] block: Respect backing bs in bdrv_refresh_filename
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 05/31] block: Respect backing bs in bdrv_refresh_filename Max Reitz
@ 2018-08-28 13:26 ` Alberto Garcia
2018-08-29 9:44 ` Max Reitz
0 siblings, 1 reply; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 13:26 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:02 PM CEST, Max Reitz wrote:
> + backing_overridden = bdrv_backing_overridden(bs);
> +
> + if (bs->open_flags & BDRV_O_NO_IO) {
> + /* Without I/O, the backing file does not change anything.
> + * Therefore, in such a case (primarily qemu-img), we can
> + * pretend the backing file has not been overridden even if
> + * it technically has been. */
> + backing_overridden = false;
> + }
What is this for?
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 29/31] block/null: Generate filename even with latency-ns
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 29/31] block/null: Generate filename even with latency-ns Max Reitz
@ 2018-08-28 13:33 ` Alberto Garcia
2018-08-29 9:46 ` Max Reitz
0 siblings, 1 reply; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 13:33 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:26 PM CEST, Max Reitz wrote:
> While we cannot represent the latency-ns option in a filename, it is not
> a strong option so not being able to should not stop us from generating
> a filename nonetheless.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
I thought I had reviewed this one already ?
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 19/31] block: Use bdrv_dirname() for relative filenames
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 19/31] block: Use bdrv_dirname() for relative filenames Max Reitz
@ 2018-08-28 13:45 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 13:45 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:16 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] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 26/31] block/nvme: Fix bdrv_refresh_filename()
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 26/31] block/nvme: Fix bdrv_refresh_filename() Max Reitz
@ 2018-08-28 13:50 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 13:50 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:23 PM CEST, Max Reitz wrote:
> Currently, nvme's bdrv_refresh_filename() is an exact copy of null's
> implementation. However, for null, "null-co://" and "null-aio://" are
> indeed valid filenames -- for nvme, they are not, as a device address is
> still required.
>
> The correct implementation should generate a filename of the form
> "nvme://[PCI address]/[namespace]" (as the comment above
> nvme_parse_filename() describes).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 21/31] block: Add strong_runtime_opts to BlockDriver
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 21/31] block: Add strong_runtime_opts to BlockDriver Max Reitz
@ 2018-08-28 14:43 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 14:43 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:18 PM CEST, Max Reitz wrote:
> This new field can be set by block drivers to list the runtime options
> they accept that may influence the contents of the respective BDS. As of
> a follow-up patch, this list will be used by the common
> bdrv_refresh_filename() implementation to decide which options to put
> into BDS.full_open_options (and consequently whether a JSON filename has
> to be created), thus freeing the drivers of having to implement that
> logic themselves.
>
> Additionally, this patch adds the field to all of the block drivers that
> need it and sets it accordingly.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 30/31] block: BDS options may lack the "driver" option
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 30/31] block: BDS options may lack the "driver" option Max Reitz
@ 2018-08-28 14:44 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-08-28 14:44 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:27 PM CEST, Max Reitz wrote:
> When BDSs are created by qemu itself (e.g. as filters in block jobs),
> they may not have a "driver" option in their options QDict. When
> generating a json:{} filename, however, it must always be present.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 05/31] block: Respect backing bs in bdrv_refresh_filename
2018-08-28 13:26 ` Alberto Garcia
@ 2018-08-29 9:44 ` Max Reitz
2018-09-04 14:07 ` Alberto Garcia
0 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-08-29 9:44 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
[-- Attachment #1: Type: text/plain, Size: 684 bytes --]
On 2018-08-28 15:26, Alberto Garcia wrote:
> On Thu 09 Aug 2018 11:35:02 PM CEST, Max Reitz wrote:
>> + backing_overridden = bdrv_backing_overridden(bs);
>> +
>> + if (bs->open_flags & BDRV_O_NO_IO) {
>> + /* Without I/O, the backing file does not change anything.
>> + * Therefore, in such a case (primarily qemu-img), we can
>> + * pretend the backing file has not been overridden even if
>> + * it technically has been. */
>> + backing_overridden = false;
>> + }
>
> What is this for?
qemu-img info opens files with BDRV_O_NO_BACKING. Without this check,
you'd get a json:{} filename with "backing": null.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 07/31] iotests.py: Add node_info()
2018-08-28 12:48 ` Alberto Garcia
@ 2018-08-29 9:45 ` Max Reitz
0 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-29 9:45 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]
On 2018-08-28 14:48, Alberto Garcia wrote:
> On Thu 09 Aug 2018 11:35:04 PM CEST, Max Reitz wrote:
>> This function queries a node; since we cannot do that right now, it
>> executes query-named-block-nodes and returns the matching node's object.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> tests/qemu-iotests/iotests.py | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 5c45788dac..6583001ac8 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -465,6 +465,16 @@ class VM(qtest.QEMUQtestMachine):
>> else:
>> iotests.log(ev)
>>
>> + def node_info(self, node_name, require_existence=True):
>> + nodes = self.qmp('query-named-block-nodes')
>> + for x in nodes['return']:
>> + if x['node-name'] == node_name:
>> + return x
>> + if require_existence:
>> + assert False
>> + else:
>> + return None
>> +
>
> I don't think you need the require_existence parameter here... if you
> simply return None you'll get an error as soon as you try to use the
> return value.
Hm! True. I'm not sure I like the non-verbosity of "Did not find key X
in None" (or whatever error Python's going to throw), but then again,
"assert False" is not really any better. So, yep, will change.
Thanks for reviewing,
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 29/31] block/null: Generate filename even with latency-ns
2018-08-28 13:33 ` Alberto Garcia
@ 2018-08-29 9:46 ` Max Reitz
0 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-08-29 9:46 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
[-- Attachment #1: Type: text/plain, Size: 508 bytes --]
On 2018-08-28 15:33, Alberto Garcia wrote:
> On Thu 09 Aug 2018 11:35:26 PM CEST, Max Reitz wrote:
>> While we cannot represent the latency-ns option in a filename, it is not
>> a strong option so not being able to should not stop us from generating
>> a filename nonetheless.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>
> I thought I had reviewed this one already ?
Hm, maybe I forgot to add your R-b tag? Or maybe there was some
non-trivial rebase change... I don't know.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 05/31] block: Respect backing bs in bdrv_refresh_filename
2018-08-29 9:44 ` Max Reitz
@ 2018-09-04 14:07 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-09-04 14:07 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Wed 29 Aug 2018 11:44:52 AM CEST, Max Reitz wrote:
> On 2018-08-28 15:26, Alberto Garcia wrote:
>> On Thu 09 Aug 2018 11:35:02 PM CEST, Max Reitz wrote:
>>> + backing_overridden = bdrv_backing_overridden(bs);
>>> +
>>> + if (bs->open_flags & BDRV_O_NO_IO) {
>>> + /* Without I/O, the backing file does not change anything.
>>> + * Therefore, in such a case (primarily qemu-img), we can
>>> + * pretend the backing file has not been overridden even if
>>> + * it technically has been. */
>>> + backing_overridden = false;
>>> + }
>>
>> What is this for?
>
> qemu-img info opens files with BDRV_O_NO_BACKING. Without this check,
> you'd get a json:{} filename with "backing": null.
Ah, I see.
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 22/31] block: Add BlockDriver.bdrv_gather_child_options
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 22/31] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
@ 2018-09-05 12:42 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-09-05 12:42 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:19 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 (currently) want to use
> differ. See quorum_gather_child_options() for more information.
>
> Note that both of these are cases which are not ideal: In case of VMDK
> it would probably be nice to be able to specify options for all extents.
> In case of quorum, the current runtime option structure is simply broken
> and needs to be fixed (but that is left for another patch).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 23/31] block: Generically refresh runtime options
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 23/31] block: Generically refresh runtime options Max Reitz
@ 2018-09-05 12:46 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-09-05 12:46 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:20 PM CEST, Max Reitz wrote:
> Instead of having every block driver which implements
> bdrv_refresh_filename() copy all of the strong 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>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 24/31] block: Purify .bdrv_refresh_filename()
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 24/31] block: Purify .bdrv_refresh_filename() Max Reitz
@ 2018-09-05 12:49 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-09-05 12:49 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:21 PM CEST, Max Reitz wrote:
> Currently, BlockDriver.bdrv_refresh_filename() is supposed to both
> refresh the filename (BDS.exact_filename) and set BDS.full_open_options.
> Now that we have generic code in the central bdrv_refresh_filename() for
> creating BDS.full_open_options, we can drop the latter part from all
> BlockDriver.bdrv_refresh_filename() implementations.
>
> This also means that we can drop all of the existing default code for
> this from the global bdrv_refresh_filename() itself.
>
> Furthermore, we now have to call BlockDriver.bdrv_refresh_filename()
> after having set BDS.full_open_options, because the block driver's
> implementation should now be allowed to depend on BDS.full_open_options
> being set correctly.
>
> Finally, with this patch we can drop the @options parameter from
> BlockDriver.bdrv_refresh_filename(); also, add a comment on this
> function's purpose in block/block_int.h while touching its interface.
>
> This completely obsoletes blklogwrite's implementation of
> .bdrv_refresh_filename().
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 25/31] block: Do not copy exact_filename from format file
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 25/31] block: Do not copy exact_filename from format file Max Reitz
@ 2018-09-05 13:23 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-09-05 13:23 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:22 PM CEST, Max Reitz wrote:
> If a format BDS's file BDS is in turn a format BDS, we cannot simply use
> the same filename, because when opening a BDS tree based on a filename
> alone, qemu will create only one format node on top of one protocol node
> (disregarding a potential backing file).
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file Max Reitz
@ 2018-09-05 14:22 ` Alberto Garcia
2018-09-07 11:32 ` Max Reitz
2018-09-11 8:45 ` Alberto Garcia
1 sibling, 1 reply; 67+ messages in thread
From: Alberto Garcia @ 2018-09-05 14:22 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
> 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().
>
> To see whether it has been overridden, we might want to compare
> bs->backing_file and bs->backing->bs->filename. However,
> bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
> to change the backing child at runtime, without modifying the image
> header), so bs->backing_file most of the time simply contains a copy of
> bs->backing->bs->filename anyway, so it is useless for such a
> comparison.
What's the point of bs->backing_file then? In what cases is it different
from backing->bs->filename?
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 08/31] iotests: Add test for backing file overrides
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 08/31] iotests: Add test for backing file overrides Max Reitz
@ 2018-09-05 14:38 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-09-05 14:38 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:05 PM CEST, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 31/31] iotests: Test json:{} filenames of internal BDSs
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 31/31] iotests: Test json:{} filenames of internal BDSs Max Reitz
@ 2018-09-05 14:44 ` Alberto Garcia
2018-09-07 11:37 ` Max Reitz
0 siblings, 1 reply; 67+ messages in thread
From: Alberto Garcia @ 2018-09-05 14:44 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:28 PM CEST, Max Reitz wrote:
> + vm.shutdown()
> +
> + #assert top_name[:5] == 'json:'
> + #top_options = json.loads(top_name[5:])
> +
> + #if filter_node_name:
> + # # This should be present and set
> + # assert top_options['backing']['driver'] == 'commit_top'
> + # # And the mid image is commit_top's backing image
> + # mid_options = top_options['backing']['backing']
> + #else:
> + # # The mid image should appear as the immediate backing BDS
> + # # of top
> + # mid_options = top_options['backing']
> +
> + #assert mid_options['driver'] == iotests.imgfmt
> + #assert mid_options['file']['filename'] == mid_img_path
What's with all these commented lines? I don't see how you are checking
the filename in this test, it's also not in the output file.
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file
2018-09-05 14:22 ` Alberto Garcia
@ 2018-09-07 11:32 ` Max Reitz
2018-09-07 12:28 ` Alberto Garcia
0 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-09-07 11:32 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
[-- Attachment #1: Type: text/plain, Size: 3579 bytes --]
On 2018-09-05 16:22, Alberto Garcia wrote:
> On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
>> 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().
>>
>> To see whether it has been overridden, we might want to compare
>> bs->backing_file and bs->backing->bs->filename. However,
>> bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
>> to change the backing child at runtime, without modifying the image
>> header), so bs->backing_file most of the time simply contains a copy of
>> bs->backing->bs->filename anyway, so it is useless for such a
>> comparison.
>
> What's the point of bs->backing_file then? In what cases is it different
> from backing->bs->filename?
Good question! Yes, why?
One thing is when you have detached the backing file, bs->backing_file
will stay the same, even though backing is now NULL. But that is pretty
much useless, I couldn't find any part in the block layer which relies
on this.
The other is... Fun. When you create the BDS, it is different, because
the format driver will just put the filename it got from the image
header there. So imagine you have this configuration:
$ mkdir foo
$ qemu-img create -f qcow2 foo/base.qcow2 64M
$ qemu-img create -f qcow2 -b base.qcow2 foo/top.qcow2
Now when you open foo/top.qcow2, backing_file will contain "base.qcow2"
at first. This is what bdrv_open_backing_file() expects (because that's
what bdrv_get_full_backing_filename() is for).
But when the backing BDS is attached, bdrv_set_backing_hd() (through
bdrv_backing_attach()) will update backing_file to "foo/base.qcow2" (or
probably the absolute equivalent, I can't remember).
That seems really pointless to me, and after more investigation, I
couldn't find any reason for why bdrv_backing_attach() should update
backing_file. Especially considering this behavior breaks things, like
bdrv_find_backing_image(), which again would expect "base.qcow2" (i.e. a
filename relative to the overlay).
So in short, functions in the block layer that query backing_file
generally expect it to be what's in the image header; while on the other
hand functions that set backing_file ignore that expectation half of the
time.
So consequentially, there is a "block: Leave BDS.backing_file constant"
patch in my "block: Deal with filters" series. It makes backing_file
always report what the image header says.
Now you will probably ask: OK, nice, but why do you still need
auto_backing_file then? Wouldn't that change be enough to just use
backing_file?
The issue here is that the image header may contain a filename that
bdrv_refresh_filename() will transform. Say your image header says
"nbd:localhost:10809" for the backing file. So that's what backing_file
will report, too. bdrv_refresh_filename() however will make
backing->bs->filename report "nbd://localhost:10809". So if you'd
compare backing_file against backing->bs->filename, you'd see a
difference and you'd have to assume the backing file was overridden,
when in fact it wasn't.
That's what we still need auto_backing_file for: It does not always
reflect the image header, instead it is supposed to reflect the
bdrv_refresh_filename() result for the BDS that is opened when you open
the backing filename given in the image header.
(At least that would be desirable; we cannot always conform to that
specification, but we'll try at least.)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 31/31] iotests: Test json:{} filenames of internal BDSs
2018-09-05 14:44 ` Alberto Garcia
@ 2018-09-07 11:37 ` Max Reitz
0 siblings, 0 replies; 67+ messages in thread
From: Max Reitz @ 2018-09-07 11:37 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]
On 2018-09-05 16:44, Alberto Garcia wrote:
> On Thu 09 Aug 2018 11:35:28 PM CEST, Max Reitz wrote:
>> + vm.shutdown()
>> +
>> + #assert top_name[:5] == 'json:'
>> + #top_options = json.loads(top_name[5:])
>> +
>> + #if filter_node_name:
>> + # # This should be present and set
>> + # assert top_options['backing']['driver'] == 'commit_top'
>> + # # And the mid image is commit_top's backing image
>> + # mid_options = top_options['backing']['backing']
>> + #else:
>> + # # The mid image should appear as the immediate backing BDS
>> + # # of top
>> + # mid_options = top_options['backing']
>> +
>> + #assert mid_options['driver'] == iotests.imgfmt
>> + #assert mid_options['file']['filename'] == mid_img_path
>
> What's with all these commented lines? I don't see how you are checking
> the filename in this test, it's also not in the output file.
Indeed, what is with them? I have no idea. They shouldn't be commented
out. Sorry.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file
2018-09-07 11:32 ` Max Reitz
@ 2018-09-07 12:28 ` Alberto Garcia
2018-09-07 12:42 ` Max Reitz
0 siblings, 1 reply; 67+ messages in thread
From: Alberto Garcia @ 2018-09-07 12:28 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Fri 07 Sep 2018 01:32:58 PM CEST, Max Reitz wrote:
> On 2018-09-05 16:22, Alberto Garcia wrote:
>> On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
>>> 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().
>>>
>>> To see whether it has been overridden, we might want to compare
>>> bs->backing_file and bs->backing->bs->filename. However,
>>> bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
>>> to change the backing child at runtime, without modifying the image
>>> header), so bs->backing_file most of the time simply contains a copy of
>>> bs->backing->bs->filename anyway, so it is useless for such a
>>> comparison.
>>
>> What's the point of bs->backing_file then? In what cases is it different
>> from backing->bs->filename?
>
> Good question! Yes, why?
>
> One thing is when you have detached the backing file, bs->backing_file
> will stay the same, even though backing is now NULL. But that is
> pretty much useless, I couldn't find any part in the block layer which
> relies on this.
[...]
> But when the backing BDS is attached, bdrv_set_backing_hd() (through
> bdrv_backing_attach()) will update backing_file to "foo/base.qcow2"
> (or probably the absolute equivalent, I can't remember).
But then why do you need bs->backing_file at all? What are the cases
where you need bs->backing_file but you can't use directly the value in
bs->backing->bs->filename because it's wrong or missing?
> So consequentially, there is a "block: Leave BDS.backing_file
> constant" patch in my "block: Deal with filters" series. It makes
> backing_file always report what the image header says.
Ok, so after tha patch bs->backing_file is guaranteed to be exactly as
it's written on the image header.
And bs->auto_backing_file is the backing filename after having called
bdrv_refresh_filename(). And that's supposed to be exactly the same as
bs->backing->bs->filename. So I have the same question again, why do you
need bs->auto_backing_filename and in what cases is it different from
bs->auto_backing_file different from bs->backing->bs->filename?
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file
2018-09-07 12:28 ` Alberto Garcia
@ 2018-09-07 12:42 ` Max Reitz
2018-09-10 16:17 ` Alberto Garcia
0 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-09-07 12:42 UTC (permalink / raw)
To: Alberto Garcia, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
[-- Attachment #1: Type: text/plain, Size: 5282 bytes --]
On 2018-09-07 14:28, Alberto Garcia wrote:
> On Fri 07 Sep 2018 01:32:58 PM CEST, Max Reitz wrote:
>> On 2018-09-05 16:22, Alberto Garcia wrote:
>>> On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
>>>> 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().
>>>>
>>>> To see whether it has been overridden, we might want to compare
>>>> bs->backing_file and bs->backing->bs->filename. However,
>>>> bs->backing_file is changed by bdrv_set_backing_hd() (which is just used
>>>> to change the backing child at runtime, without modifying the image
>>>> header), so bs->backing_file most of the time simply contains a copy of
>>>> bs->backing->bs->filename anyway, so it is useless for such a
>>>> comparison.
>>>
>>> What's the point of bs->backing_file then? In what cases is it different
>>> from backing->bs->filename?
>>
>> Good question! Yes, why?
>>
>> One thing is when you have detached the backing file, bs->backing_file
>> will stay the same, even though backing is now NULL. But that is
>> pretty much useless, I couldn't find any part in the block layer which
>> relies on this.
> [...]
>> But when the backing BDS is attached, bdrv_set_backing_hd() (through
>> bdrv_backing_attach()) will update backing_file to "foo/base.qcow2"
>> (or probably the absolute equivalent, I can't remember).
>
> But then why do you need bs->backing_file at all? What are the cases
> where you need bs->backing_file but you can't use directly the value in
> bs->backing->bs->filename because it's wrong or missing?
The main case is when bs->backing has not been opened yet. Then
bdrv_open_backing_file() needs some place that tells it what image file
to open.
>> So consequentially, there is a "block: Leave BDS.backing_file
>> constant" patch in my "block: Deal with filters" series. It makes
>> backing_file always report what the image header says.
>
> Ok, so after tha patch bs->backing_file is guaranteed to be exactly as
> it's written on the image header.
Yes.
> And bs->auto_backing_file is the backing filename after having called
> bdrv_refresh_filename(). And that's supposed to be exactly the same as
> bs->backing->bs->filename.
No.
It's supposed to be exactly the same, *if* bs->backing->bs has been
opened only based on bs->backing_file.
The user can override the backing file (either as a whole through the
@backing node reference, or at least some of its options as a dict under
@backing), and then bs->auto_backing_filename is not supposed to be the
same as bs->backing->bs->filename.
> So I have the same question again, why do you
> need bs->auto_backing_filename and in what cases is it different from
> bs->auto_backing_file different from bs->backing->bs->filename?
The whole purpose of bs->auto_backing_file is so you can compare it with
bs->backing->bs->filename, see when it differs, and then you know that
the user has changed the backing file from what it would be based on the
image header alone (that is, if the user hadn't specified any @backing
option at all).
This is exactly what the commit message says.
It's just that for this comparison to really make sense, we would need
the refreshed filename of the BDS that is opened when you just take the
image header's backing filename.
If we know that the user has not specified any options that override it,
we copy bs->backing->bs->filename to bs->auto_backing_filename, because
we know it to be exactly that value that we want.
Otherwise, we just have to hope that refreshing the filename does not
change it from what the image header said.
(Now you could ask yourself, if we know the user hasn't specified any
options to override the backing file -- why don't we just store a flag?
I tried that, Kevin wasn't really happy about it because it's hard to
catch all corner cases. There are many ways to change a backing file,
and if you want to find out when a backing file has been changed to
exactly what the image header says (this is the ideal outcome of the
commit block job, for instance), you basically get back to the same
problem of comparing filenames.)
Let me try to summarize what bs->auto_backing_filename is again.
We want to compare it against bs->backing->bs->filename, which is
basically bdrv_filename(bdrv_open(X)). Question is, is X the same as
bs->backing_file? (assuming bs->backing_file is what the image header says)
So ideally, bs->auto_backing_filename needs to be the result of
bdrv_filename(bdrv_open(bs->backing_file)). But that is rather stupid
to do, so we don't do that.
The next best thing is just using bs->backing_file and hoping that
bdrv_refresh_filename() didn't change anything. Sometimes, that hope is
futile, however.
So I try to get bdrv_refresh_filename() squeezed in there sometimes.
When at any point we know that bs->backing->bs is exactly
bdrv_open(bs->backing_file), we know that bs->backing->bs->filename is
bdrv_filename(bdrv_open(bs->backing_file)). So then we can copy it into
bs->auto_backing_file -- *but only if we know that bs->backing->bs ==
bdrv_open(bs->backing_file)!*
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
` (31 preceding siblings ...)
2018-08-15 3:43 ` [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues no-reply
@ 2018-09-10 15:18 ` Kevin Wolf
2018-09-10 16:51 ` Max Reitz
32 siblings, 1 reply; 67+ messages in thread
From: Kevin Wolf @ 2018-09-10 15:18 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Eric Blake, Alberto Garcia
Am 09.08.2018 um 23:34 hat Max Reitz geschrieben:
> Once more, I’ll spare both me and you another iteration of the cover
> letter, so see here:
>
> http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html
>
> (Although this series no longer includes a @base-directory option.)
>
> In regards to the last version, the biggest change is that I dropped
> backing_overridden and instead try to compare the filename from the
> image header with the filename of the actual backing BDS to find out
> whether the backing file has been overridden.
>
> In order that this doesn’t break whenever the header contains a slightly
> unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or
> “nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something
> different from what bdrv_refresh_filename() would generate), when the
> reference filename in the BDS (auto_backing_file) is used to open the
> backing file, it is updated from the backing BDS's resulting filename.
This doesn't seem to apply cleanly any more.
Kevin
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file
2018-09-07 12:42 ` Max Reitz
@ 2018-09-10 16:17 ` Alberto Garcia
0 siblings, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-09-10 16:17 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Fri 07 Sep 2018 02:42:53 PM CEST, Max Reitz wrote:
> The whole purpose of bs->auto_backing_file is so you can compare it
> with bs->backing->bs->filename, see when it differs, and then you know
> that the user has changed the backing file from what it would be based
> on the image header alone (that is, if the user hadn't specified any
> @backing option at all).
[...]
> (Now you could ask yourself, if we know the user hasn't specified any
> options to override the backing file -- why don't we just store a
> flag? I tried that, Kevin wasn't really happy about it because it's
> hard to catch all corner cases. There are many ways to change a
> backing file, and if you want to find out when a backing file has been
> changed to exactly what the image header says (this is the ideal
> outcome of the commit block job, for instance), you basically get back
> to the same problem of comparing filenames.)
Ok, I think this clarifies a lot. Thanks!
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues
2018-09-10 15:18 ` Kevin Wolf
@ 2018-09-10 16:51 ` Max Reitz
2018-09-11 9:10 ` Kevin Wolf
0 siblings, 1 reply; 67+ messages in thread
From: Max Reitz @ 2018-09-10 16:51 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, Eric Blake, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]
On 10.09.18 17:18, Kevin Wolf wrote:
> Am 09.08.2018 um 23:34 hat Max Reitz geschrieben:
>> Once more, I’ll spare both me and you another iteration of the cover
>> letter, so see here:
>>
>> http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html
>>
>> (Although this series no longer includes a @base-directory option.)
>>
>> In regards to the last version, the biggest change is that I dropped
>> backing_overridden and instead try to compare the filename from the
>> image header with the filename of the actual backing BDS to find out
>> whether the backing file has been overridden.
>>
>> In order that this doesn’t break whenever the header contains a slightly
>> unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or
>> “nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something
>> different from what bdrv_refresh_filename() would generate), when the
>> reference filename in the BDS (auto_backing_file) is used to open the
>> backing file, it is updated from the backing BDS's resulting filename.
>
> This doesn't seem to apply cleanly any more.
I know, but is that a "Please send your current v11 to the list"?
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file Max Reitz
2018-09-05 14:22 ` Alberto Garcia
@ 2018-09-11 8:45 ` Alberto Garcia
1 sibling, 0 replies; 67+ messages in thread
From: Alberto Garcia @ 2018-09-11 8:45 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: qemu-devel, Kevin Wolf, Eric Blake
On Thu 09 Aug 2018 11:35:01 PM CEST, Max Reitz wrote:
> @@ -295,11 +296,13 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> goto fail;
> }
> ret = bdrv_pread(bs->file, header.backing_file_offset,
> - bs->backing_file, len);
> + bs->auto_backing_file, len);
> if (ret < 0) {
> goto fail;
> }
> - bs->backing_file[len] = '\0';
> + bs->auto_backing_file[len] = '\0';
> + pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> + bs->auto_backing_file);
> }
The patch looks fine, I just wonder why you don't simply copy
bs->backing_file into bs->auto_backing_file instead of the other way
around, it would make the patch smaller.
Either way,
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues
2018-09-10 16:51 ` Max Reitz
@ 2018-09-11 9:10 ` Kevin Wolf
0 siblings, 0 replies; 67+ messages in thread
From: Kevin Wolf @ 2018-09-11 9:10 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel, Eric Blake, Alberto Garcia
[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]
Am 10.09.2018 um 18:51 hat Max Reitz geschrieben:
> On 10.09.18 17:18, Kevin Wolf wrote:
> > Am 09.08.2018 um 23:34 hat Max Reitz geschrieben:
> >> Once more, I’ll spare both me and you another iteration of the cover
> >> letter, so see here:
> >>
> >> http://lists.nongnu.org/archive/html/qemu-block/2017-09/msg01030.html
> >>
> >> (Although this series no longer includes a @base-directory option.)
> >>
> >> In regards to the last version, the biggest change is that I dropped
> >> backing_overridden and instead try to compare the filename from the
> >> image header with the filename of the actual backing BDS to find out
> >> whether the backing file has been overridden.
> >>
> >> In order that this doesn’t break whenever the header contains a slightly
> >> unusual (“non-canonical”) backing filename (e.g. “file:foo.qcow2” or
> >> “nbd:localhost:10809” instead of “nbd://localhost:10809”, i.e. something
> >> different from what bdrv_refresh_filename() would generate), when the
> >> reference filename in the BDS (auto_backing_file) is used to open the
> >> backing file, it is updated from the backing BDS's resulting filename.
> >
> > This doesn't seem to apply cleanly any more.
>
> I know, but is that a "Please send your current v11 to the list"?
Depends on how urgently you want me to review this. I don't like
reviewing patches directly from emails, without being able to apply
them.
Or maybe if you just can give me the commit ID this is based on? I
wouldn't mind applying it to an old tree as long as I can apply it
somewhere.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 67+ messages in thread
end of thread, other threads:[~2018-09-11 9:10 UTC | newest]
Thread overview: 67+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-09 21:34 [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues Max Reitz
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 01/31] block: Use bdrv_refresh_filename() to pull Max Reitz
2018-08-28 12:13 ` Alberto Garcia
2018-08-09 21:34 ` [Qemu-devel] [PATCH for-3.1 v10 02/31] block: Use children list in bdrv_refresh_filename Max Reitz
2018-08-16 19:02 ` Eric Blake
2018-08-28 12:27 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 03/31] block: Skip implicit nodes for filename info Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 04/31] block: Add BDS.auto_backing_file Max Reitz
2018-09-05 14:22 ` Alberto Garcia
2018-09-07 11:32 ` Max Reitz
2018-09-07 12:28 ` Alberto Garcia
2018-09-07 12:42 ` Max Reitz
2018-09-10 16:17 ` Alberto Garcia
2018-09-11 8:45 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 05/31] block: Respect backing bs in bdrv_refresh_filename Max Reitz
2018-08-28 13:26 ` Alberto Garcia
2018-08-29 9:44 ` Max Reitz
2018-09-04 14:07 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 06/31] iotests.py: Add filter_imgfmt() Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 07/31] iotests.py: Add node_info() Max Reitz
2018-08-28 12:48 ` Alberto Garcia
2018-08-29 9:45 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 08/31] iotests: Add test for backing file overrides Max Reitz
2018-09-05 14:38 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 09/31] block: Make path_combine() return the path Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 10/31] block: bdrv_get_full_backing_filename_from_...'s ret. val Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 11/31] block: bdrv_get_full_backing_filename's " Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 12/31] block: Add bdrv_make_absolute_filename() Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 13/31] block: Fix bdrv_find_backing_image() Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 14/31] block: Add bdrv_dirname() Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 15/31] blkverify: Make bdrv_dirname() return NULL Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 16/31] quorum: " Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 17/31] block/nbd: " Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 18/31] block/nfs: Implement bdrv_dirname() Max Reitz
2018-08-28 12:49 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 19/31] block: Use bdrv_dirname() for relative filenames Max Reitz
2018-08-28 13:45 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 20/31] iotests: Add quorum case to test 110 Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 21/31] block: Add strong_runtime_opts to BlockDriver Max Reitz
2018-08-28 14:43 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 22/31] block: Add BlockDriver.bdrv_gather_child_options Max Reitz
2018-09-05 12:42 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 23/31] block: Generically refresh runtime options Max Reitz
2018-09-05 12:46 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 24/31] block: Purify .bdrv_refresh_filename() Max Reitz
2018-09-05 12:49 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 25/31] block: Do not copy exact_filename from format file Max Reitz
2018-09-05 13:23 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 26/31] block/nvme: Fix bdrv_refresh_filename() Max Reitz
2018-08-28 13:50 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 27/31] block/curl: Harmonize option defaults Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 28/31] block/curl: Implement bdrv_refresh_filename() Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 29/31] block/null: Generate filename even with latency-ns Max Reitz
2018-08-28 13:33 ` Alberto Garcia
2018-08-29 9:46 ` Max Reitz
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 30/31] block: BDS options may lack the "driver" option Max Reitz
2018-08-28 14:44 ` Alberto Garcia
2018-08-09 21:35 ` [Qemu-devel] [PATCH for-3.1 v10 31/31] iotests: Test json:{} filenames of internal BDSs Max Reitz
2018-09-05 14:44 ` Alberto Garcia
2018-09-07 11:37 ` Max Reitz
2018-08-15 3:43 ` [Qemu-devel] [PATCH for-3.1 v10 00/31] block: Fix some filename generation issues no-reply
2018-08-16 0:01 ` Max Reitz
2018-08-16 6:02 ` Markus Armbruster
2018-08-17 20:03 ` Max Reitz
2018-09-10 15:18 ` Kevin Wolf
2018-09-10 16:51 ` Max Reitz
2018-09-11 9:10 ` Kevin Wolf
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.