* [PULL 01/17] qcow2: Assert that host cluster offsets fit in L2 table entries
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 02/17] block: Use a GString in bdrv_perm_names() Max Reitz
` (16 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
The standard cluster descriptor in L2 table entries has a field to
store the host cluster offset. When we need to get that offset from an
entry we use L2E_OFFSET_MASK to ensure that we only use the bits that
belong to that field.
But while that mask is used every time we read from an L2 entry, it
is never used when we write to it. Due to the QCOW_MAX_CLUSTER_OFFSET
limit set in the cluster allocation code QEMU can never produce
offsets that don't fit in that field so any such offset would indicate
a bug in QEMU.
Compressed cluster descriptors contain two fields (host cluster offset
and size of the compressed data) and the situation with them is
similar. In this case the masks are not constant but are stored in the
csize_mask and cluster_offset_mask fields of BDRVQcow2State.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20200113161146.20099-1-berto@igalia.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-cluster.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8982b7b762..e9431f6785 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -777,6 +777,10 @@ int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
(cluster_offset + compressed_size - 1) / QCOW2_COMPRESSED_SECTOR_SIZE -
(cluster_offset / QCOW2_COMPRESSED_SECTOR_SIZE);
+ /* The offset and size must fit in their fields of the L2 table entry */
+ assert((cluster_offset & s->cluster_offset_mask) == cluster_offset);
+ assert((nb_csectors & s->csize_mask) == nb_csectors);
+
cluster_offset |= QCOW_OFLAG_COMPRESSED |
((uint64_t)nb_csectors << s->csize_shift);
@@ -972,6 +976,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
assert(l2_index + m->nb_clusters <= s->l2_slice_size);
for (i = 0; i < m->nb_clusters; i++) {
+ uint64_t offset = cluster_offset + (i << s->cluster_bits);
/* if two concurrent writes happen to the same unallocated cluster
* each write allocates separate cluster and writes data concurrently.
* The first one to complete updates l2 table with pointer to its
@@ -982,8 +987,10 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
old_cluster[j++] = l2_slice[l2_index + i];
}
- l2_slice[l2_index + i] = cpu_to_be64((cluster_offset +
- (i << s->cluster_bits)) | QCOW_OFLAG_COPIED);
+ /* The offset must fit in the offset field of the L2 table entry */
+ assert((offset & L2E_OFFSET_MASK) == offset);
+
+ l2_slice[l2_index + i] = cpu_to_be64(offset | QCOW_OFLAG_COPIED);
}
@@ -1913,6 +1920,9 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
goto fail;
}
+ /* The offset must fit in the offset field */
+ assert((offset & L2E_OFFSET_MASK) == offset);
+
if (l2_refcount > 1) {
/* For shared L2 tables, set the refcount accordingly
* (it is already 1 and needs to be l2_refcount) */
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 02/17] block: Use a GString in bdrv_perm_names()
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
2020-02-06 12:51 ` [PULL 01/17] qcow2: Assert that host cluster offsets fit in L2 table entries Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 03/17] block: fix memleaks in bdrv_refresh_filename Max Reitz
` (15 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
This is a bit more efficient than having to allocate and free memory
for each new permission.
The default size (30) is enough for "consistent read, write, resize".
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-id: 20200110171518.22168-1-berto@igalia.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 6c2b2bd2e2..fe5050c53f 100644
--- a/block.c
+++ b/block.c
@@ -1998,18 +1998,19 @@ char *bdrv_perm_names(uint64_t perm)
{ 0, NULL }
};
- char *result = g_strdup("");
+ GString *result = g_string_sized_new(30);
struct perm_name *p;
for (p = permissions; p->name; p++) {
if (perm & p->perm) {
- char *old = result;
- result = g_strdup_printf("%s%s%s", old, *old ? ", " : "", p->name);
- g_free(old);
+ if (result->len > 0) {
+ g_string_append(result, ", ");
+ }
+ g_string_append(result, p->name);
}
}
- return result;
+ return g_string_free(result, FALSE);
}
/*
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 03/17] block: fix memleaks in bdrv_refresh_filename
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
2020-02-06 12:51 ` [PULL 01/17] qcow2: Assert that host cluster offsets fit in L2 table entries Max Reitz
2020-02-06 12:51 ` [PULL 02/17] block: Use a GString in bdrv_perm_names() Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 04/17] qcow2: Use a GString in report_unsupported_feature() Max Reitz
` (14 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Pan Nengyuan <pannengyuan@huawei.com>
If we call the qmp 'query-block' while qemu is working on
'block-commit', it will cause memleaks, the memory leak stack is as
follow:
Indirect leak of 12360 byte(s) in 3 object(s) allocated from:
#0 0x7f80f0b6d970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
#1 0x7f80ee86049d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
#2 0x55ea95b5bb67 in qdict_new /mnt/sdb/qemu-4.2.0-rc0/qobject/qdict.c:29
#3 0x55ea956cd043 in bdrv_refresh_filename /mnt/sdb/qemu-4.2.0-rc0/block.c:6427
#4 0x55ea956cc950 in bdrv_refresh_filename /mnt/sdb/qemu-4.2.0-rc0/block.c:6399
#5 0x55ea956cc950 in bdrv_refresh_filename /mnt/sdb/qemu-4.2.0-rc0/block.c:6399
#6 0x55ea956cc950 in bdrv_refresh_filename /mnt/sdb/qemu-4.2.0-rc0/block.c:6399
#7 0x55ea958818ea in bdrv_block_device_info /mnt/sdb/qemu-4.2.0-rc0/block/qapi.c:56
#8 0x55ea958879de in bdrv_query_info /mnt/sdb/qemu-4.2.0-rc0/block/qapi.c:392
#9 0x55ea9588b58f in qmp_query_block /mnt/sdb/qemu-4.2.0-rc0/block/qapi.c:578
#10 0x55ea95567392 in qmp_marshal_query_block qapi/qapi-commands-block-core.c:95
Indirect leak of 4120 byte(s) in 1 object(s) allocated from:
#0 0x7f80f0b6d970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
#1 0x7f80ee86049d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
#2 0x55ea95b5bb67 in qdict_new /mnt/sdb/qemu-4.2.0-rc0/qobject/qdict.c:29
#3 0x55ea956cd043 in bdrv_refresh_filename /mnt/sdb/qemu-4.2.0-rc0/block.c:6427
#4 0x55ea956cc950 in bdrv_refresh_filename /mnt/sdb/qemu-4.2.0-rc0/block.c:6399
#5 0x55ea956cc950 in bdrv_refresh_filename /mnt/sdb/qemu-4.2.0-rc0/block.c:6399
#6 0x55ea9569f301 in bdrv_backing_attach /mnt/sdb/qemu-4.2.0-rc0/block.c:1064
#7 0x55ea956a99dd in bdrv_replace_child_noperm /mnt/sdb/qemu-4.2.0-rc0/block.c:2283
#8 0x55ea956b9b53 in bdrv_replace_node /mnt/sdb/qemu-4.2.0-rc0/block.c:4196
#9 0x55ea956b9e49 in bdrv_append /mnt/sdb/qemu-4.2.0-rc0/block.c:4236
#10 0x55ea958c3472 in commit_start /mnt/sdb/qemu-4.2.0-rc0/block/commit.c:306
#11 0x55ea94b68ab0 in qmp_block_commit /mnt/sdb/qemu-4.2.0-rc0/blockdev.c:3459
#12 0x55ea9556a7a7 in qmp_marshal_block_commit qapi/qapi-commands-block-core.c:407
Fixes: bb808d5f5c0978828a974d547e6032402c339555
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
Message-id: 20200116085600.24056-1-pannengyuan@huawei.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block.c b/block.c
index fe5050c53f..9c810534d6 100644
--- a/block.c
+++ b/block.c
@@ -6442,6 +6442,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
child->bs->exact_filename);
pstrcpy(bs->filename, sizeof(bs->filename), child->bs->filename);
+ qobject_unref(bs->full_open_options);
bs->full_open_options = qobject_ref(child->bs->full_open_options);
return;
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 04/17] qcow2: Use a GString in report_unsupported_feature()
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (2 preceding siblings ...)
2020-02-06 12:51 ` [PULL 03/17] block: fix memleaks in bdrv_refresh_filename Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 05/17] iotests: remove 'linux' from default supported platforms Max Reitz
` (13 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
This is a bit more efficient than having to allocate and free memory
for each item.
The default size (60) is enough for all the existing incompatible
features or the "Unknown incompatible feature" message.
Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 20200115135626.19442-1-berto@igalia.com
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index cef9d72b3a..e29fc07068 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -453,16 +453,15 @@ static void cleanup_unknown_header_ext(BlockDriverState *bs)
static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
uint64_t mask)
{
- char *features = g_strdup("");
- char *old;
+ g_autoptr(GString) features = g_string_sized_new(60);
while (table && table->name[0] != '\0') {
if (table->type == QCOW2_FEAT_TYPE_INCOMPATIBLE) {
if (mask & (1ULL << table->bit)) {
- old = features;
- features = g_strdup_printf("%s%s%.46s", old, *old ? ", " : "",
- table->name);
- g_free(old);
+ if (features->len > 0) {
+ g_string_append(features, ", ");
+ }
+ g_string_append_printf(features, "%.46s", table->name);
mask &= ~(1ULL << table->bit);
}
}
@@ -470,14 +469,14 @@ static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
}
if (mask) {
- old = features;
- features = g_strdup_printf("%s%sUnknown incompatible feature: %" PRIx64,
- old, *old ? ", " : "", mask);
- g_free(old);
+ if (features->len > 0) {
+ g_string_append(features, ", ");
+ }
+ g_string_append_printf(features,
+ "Unknown incompatible feature: %" PRIx64, mask);
}
- error_setg(errp, "Unsupported qcow2 feature(s): %s", features);
- g_free(features);
+ error_setg(errp, "Unsupported qcow2 feature(s): %s", features->str);
}
/*
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 05/17] iotests: remove 'linux' from default supported platforms
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (3 preceding siblings ...)
2020-02-06 12:51 ` [PULL 04/17] qcow2: Use a GString in report_unsupported_feature() Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 06/17] iotests: Test 041 only works on certain systems Max Reitz
` (12 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: John Snow <jsnow@redhat.com>
verify_platform will check an explicit whitelist and blacklist instead.
The default will now be assumed to be allowed to run anywhere.
For tests that do not specify their platforms explicitly, this has the effect of
enabling these tests on non-linux platforms. For tests that always specified
linux explicitly, there is no change.
For Python tests on FreeBSD at least; only seven python tests fail:
045 147 149 169 194 199 211
045 and 149 appear to be misconfigurations,
147 and 194 are the AF_UNIX path too long error,
169 and 199 are bitmap migration bugs, and
211 is a bug that shows up on Linux platforms, too.
This is at least good evidence that these tests are not Linux-only. If
they aren't suitable for other platforms, they should be disabled on a
per-platform basis as appropriate.
Therefore, let's switch these on and deal with the failures.
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 20200121095205.26323-2-thuth@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 89aa2df2f3..ead04a1ab5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -931,9 +931,14 @@ def verify_protocol(supported=[], unsupported=[]):
if not_sup or (imgproto in unsupported):
notrun('not suitable for this protocol: %s' % imgproto)
-def verify_platform(supported_oses=['linux']):
- if True not in [sys.platform.startswith(x) for x in supported_oses]:
- notrun('not suitable for this OS: %s' % sys.platform)
+def verify_platform(supported=None, unsupported=None):
+ if unsupported is not None:
+ if any((sys.platform.startswith(x) for x in unsupported)):
+ notrun('not suitable for this OS: %s' % sys.platform)
+
+ if supported is not None:
+ if not any((sys.platform.startswith(x) for x in supported)):
+ notrun('not suitable for this OS: %s' % sys.platform)
def verify_cache_mode(supported_cache_modes=[]):
if supported_cache_modes and (cachemode not in supported_cache_modes):
@@ -1028,7 +1033,8 @@ def execute_unittest(output, verbosity, debug):
sys.stderr.write(out)
def execute_test(test_function=None,
- supported_fmts=[], supported_oses=['linux'],
+ supported_fmts=[],
+ supported_platforms=None,
supported_cache_modes=[], supported_aio_modes={},
unsupported_fmts=[], supported_protocols=[],
unsupported_protocols=[]):
@@ -1046,7 +1052,7 @@ def execute_test(test_function=None,
verbosity = 1
verify_image_format(supported_fmts, unsupported_fmts)
verify_protocol(supported_protocols, unsupported_protocols)
- verify_platform(supported_oses)
+ verify_platform(supported=supported_platforms)
verify_cache_mode(supported_cache_modes)
verify_aio_mode(supported_aio_modes)
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 06/17] iotests: Test 041 only works on certain systems
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (4 preceding siblings ...)
2020-02-06 12:51 ` [PULL 05/17] iotests: remove 'linux' from default supported platforms Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 07/17] iotests: Test 183 does not work on macOS and OpenBSD Max Reitz
` (11 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Thomas Huth <thuth@redhat.com>
041 works fine on Linux, FreeBSD, NetBSD and OpenBSD, but fails on macOS.
Let's mark it as only supported on the systems where we know that it is
working fine.
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 20200121095205.26323-3-thuth@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/041 | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index c07437fda1..0181f7a9b6 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1134,4 +1134,5 @@ class TestOrphanedSource(iotests.QMPTestCase):
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2', 'qed'],
- supported_protocols=['file'])
+ supported_protocols=['file'],
+ supported_platforms=['linux', 'freebsd', 'netbsd', 'openbsd'])
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 07/17] iotests: Test 183 does not work on macOS and OpenBSD
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (5 preceding siblings ...)
2020-02-06 12:51 ` [PULL 06/17] iotests: Test 041 only works on certain systems Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 08/17] iotests: Check for the availability of the required devices in 267 and 127 Max Reitz
` (10 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Thomas Huth <thuth@redhat.com>
In the long run, we might want to add test 183 to the "auto" group
(but it still fails occasionally, so we cannot do that yet). However,
when running 183 in Cirrus-CI on macOS, or with our vm-build-openbsd
target, it currently always fails with an "Timeout waiting for return
on handle 0" error.
Let's mark it as supported only on systems where the test is working
most of the time (i.e. Linux, FreeBSD and NetBSD).
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 20200121095205.26323-4-thuth@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/183 | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
index 64621617f5..acdbefa310 100755
--- a/tests/qemu-iotests/183
+++ b/tests/qemu-iotests/183
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
. ./common.filter
. ./common.qemu
+_supported_os Linux FreeBSD NetBSD
_supported_fmt qcow2 raw qed quorum
_supported_proto file
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 08/17] iotests: Check for the availability of the required devices in 267 and 127
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (6 preceding siblings ...)
2020-02-06 12:51 ` [PULL 07/17] iotests: Test 183 does not work on macOS and OpenBSD Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 09/17] iotests: Skip Python-based tests if QEMU does not support virtio-blk Max Reitz
` (9 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Thomas Huth <thuth@redhat.com>
We are going to enable 127 in the "auto" group, but it only works if
virtio-scsi and scsi-hd are available - which is not the case with
QEMU binaries like qemu-system-tricore for example, so we need a
proper check for the availability of these devices here.
A very similar problem exists in iotest 267 - it has been added to
the "auto" group already, but requires virtio-blk and thus currently
fails with qemu-system-tricore for example. Let's also add aproper
check there.
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 20200121095205.26323-5-thuth@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/127 | 2 ++
tests/qemu-iotests/267 | 2 ++
tests/qemu-iotests/common.rc | 14 ++++++++++++++
3 files changed, 18 insertions(+)
diff --git a/tests/qemu-iotests/127 b/tests/qemu-iotests/127
index b64926ab31..a4fc866038 100755
--- a/tests/qemu-iotests/127
+++ b/tests/qemu-iotests/127
@@ -43,6 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
_supported_fmt qcow2
_supported_proto file
+_require_devices virtio-scsi scsi-hd
+
IMG_SIZE=64K
_make_test_img $IMG_SIZE
diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
index c296877168..3146273eef 100755
--- a/tests/qemu-iotests/267
+++ b/tests/qemu-iotests/267
@@ -46,6 +46,8 @@ _require_drivers copy-on-read
# and generally impossible with external data files
_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
+_require_devices virtio-blk
+
do_run_qemu()
{
echo Testing: "$@"
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9ccde32634..8a6366c09d 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -713,5 +713,19 @@ _require_large_file()
rm "$TEST_IMG"
}
+# Check that a set of devices is available in the QEMU binary
+#
+_require_devices()
+{
+ available=$($QEMU -M none -device help | \
+ grep ^name | sed -e 's/^name "//' -e 's/".*$//')
+ for device
+ do
+ if ! echo "$available" | grep -q "$device" ; then
+ _notrun "$device not available"
+ fi
+ done
+}
+
# make sure this script returns success
true
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 09/17] iotests: Skip Python-based tests if QEMU does not support virtio-blk
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (7 preceding siblings ...)
2020-02-06 12:51 ` [PULL 08/17] iotests: Check for the availability of the required devices in 267 and 127 Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 10/17] iotests: Enable more tests in the 'auto' group to improve test coverage Max Reitz
` (8 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Thomas Huth <thuth@redhat.com>
We are going to enable some of the python-based tests in the "auto" group,
and these tests require virtio-blk to work properly. Running iotests
without virtio-blk likely does not make too much sense anyway, so instead
of adding a check for the availability of virtio-blk to each and every
test (which does not sound very appealing), let's rather add a check for
this a central spot in the "check" script instead (so that it is still
possible to run "make check" for qemu-system-tricore for example).
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 20200121095205.26323-6-thuth@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/check | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 39ed5bc1be..fff5fa956a 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -655,7 +655,15 @@ fi
python_usable=false
if $PYTHON -c 'import sys; sys.exit(0 if sys.version_info >= (3,6) else 1)'
then
- python_usable=true
+ # Our python framework also requires virtio-blk
+ if "$QEMU_PROG" -M none -device help | grep -q virtio-blk >/dev/null 2>&1
+ then
+ python_usable=true
+ else
+ python_unusable_because="Missing virtio-blk in QEMU binary"
+ fi
+else
+ python_unusable_because="Unsupported Python version"
fi
default_machine=$($QEMU_PROG -machine help | sed -n '/(default)/ s/ .*//p')
@@ -843,7 +851,7 @@ do
run_command="$PYTHON $seq"
else
run_command="false"
- echo "Unsupported Python version" > $seq.notrun
+ echo "$python_unusable_because" > $seq.notrun
fi
else
run_command="./$seq"
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 10/17] iotests: Enable more tests in the 'auto' group to improve test coverage
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (8 preceding siblings ...)
2020-02-06 12:51 ` [PULL 09/17] iotests: Skip Python-based tests if QEMU does not support virtio-blk Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 11/17] qcow2: Don't round the L1 table allocation up to the sector size Max Reitz
` (7 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Thomas Huth <thuth@redhat.com>
According to Kevin, tests 030, 040 and 041 are among the most valuable
tests that we have, so we should always run them if possible, even if
they take a little bit longer.
According to Max, it would be good to have a test for iothreads and
migration. 127 and 256 seem to be good candidates for iothreads. For
migration, let's enable 181 and 203 (which also tests iothreads).
(091 would be a good candidate for migration, too, but Alex Bennée
reported that this test fails on ZFS file systems, so it can't be
included yet)
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-id: 20200121095205.26323-7-thuth@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/group | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index e041cc1ee3..9d30a4b6c4 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -51,7 +51,7 @@
027 rw auto quick
028 rw backing quick
029 rw auto quick
-030 rw backing
+030 rw auto backing
031 rw auto quick
032 rw auto quick
033 rw auto quick
@@ -61,8 +61,8 @@
037 rw auto backing quick
038 rw auto backing quick
039 rw auto quick
-040 rw
-041 rw backing
+040 rw auto
+041 rw auto backing
042 rw auto quick
043 rw auto backing
044 rw
@@ -148,7 +148,7 @@
124 rw backing
125 rw
126 rw auto backing
-127 rw backing quick
+127 rw auto backing quick
128 rw quick
129 rw quick
130 rw quick
@@ -197,7 +197,7 @@
177 rw auto quick
178 img
179 rw auto quick
-181 rw migration
+181 rw auto migration
182 rw quick
183 rw migration
184 rw auto quick
@@ -218,7 +218,7 @@
200 rw
201 rw migration
202 rw quick
-203 rw migration
+203 rw auto migration
204 rw quick
205 rw quick
206 rw
@@ -270,7 +270,7 @@
253 rw quick
254 rw backing quick
255 rw quick
-256 rw quick
+256 rw auto quick
257 rw
258 rw quick
260 rw quick
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 11/17] qcow2: Don't round the L1 table allocation up to the sector size
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (9 preceding siblings ...)
2020-02-06 12:51 ` [PULL 10/17] iotests: Enable more tests in the 'auto' group to improve test coverage Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 12/17] qcow2: Tighten cluster_offset alignment assertions Max Reitz
` (6 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
The L1 table is read from disk using the byte-based bdrv_pread() and
is never accessed beyond its last element, so there's no need to
allocate more memory than that.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: b2e27214ec7b03a585931bcf383ee1ac3a641a10.1579374329.git.berto@igalia.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-cluster.c | 5 ++---
block/qcow2-refcount.c | 2 +-
block/qcow2-snapshot.c | 3 +--
block/qcow2.c | 2 +-
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index e9431f6785..0384fb2339 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -124,12 +124,11 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
#endif
new_l1_size2 = sizeof(uint64_t) * new_l1_size;
- new_l1_table = qemu_try_blockalign(bs->file->bs,
- ROUND_UP(new_l1_size2, 512));
+ new_l1_table = qemu_try_blockalign(bs->file->bs, new_l1_size2);
if (new_l1_table == NULL) {
return -ENOMEM;
}
- memset(new_l1_table, 0, ROUND_UP(new_l1_size2, 512));
+ memset(new_l1_table, 0, new_l1_size2);
if (s->l1_size) {
memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index f67ac6b2d8..c963bc8de1 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1262,7 +1262,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
* l1_table_offset when it is the current s->l1_table_offset! Be careful
* when changing this! */
if (l1_table_offset != s->l1_table_offset) {
- l1_table = g_try_malloc0(ROUND_UP(l1_size2, 512));
+ l1_table = g_try_malloc0(l1_size2);
if (l1_size2 && l1_table == NULL) {
ret = -ENOMEM;
goto fail;
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5ab64da1ec..82c32d4c9b 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -1024,8 +1024,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
return ret;
}
new_l1_bytes = sn->l1_size * sizeof(uint64_t);
- new_l1_table = qemu_try_blockalign(bs->file->bs,
- ROUND_UP(new_l1_bytes, 512));
+ new_l1_table = qemu_try_blockalign(bs->file->bs, new_l1_bytes);
if (new_l1_table == NULL) {
return -ENOMEM;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index e29fc07068..83db013814 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1491,7 +1491,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
if (s->l1_size > 0) {
s->l1_table = qemu_try_blockalign(bs->file->bs,
- ROUND_UP(s->l1_size * sizeof(uint64_t), 512));
+ s->l1_size * sizeof(uint64_t));
if (s->l1_table == NULL) {
error_setg(errp, "Could not allocate L1 table");
ret = -ENOMEM;
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 12/17] qcow2: Tighten cluster_offset alignment assertions
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (10 preceding siblings ...)
2020-02-06 12:51 ` [PULL 11/17] qcow2: Don't round the L1 table allocation up to the sector size Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 13/17] qcow2: Use bs->bl.request_alignment when updating an L1 entry Max Reitz
` (5 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
qcow2_alloc_cluster_offset() and qcow2_get_cluster_offset() always
return offsets that are cluster-aligned so don't just check that they
are sector-aligned.
The check in qcow2_co_preadv_task() is also replaced by an assertion
for the same reason.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 558ba339965f858bede4c73ce3f50f0c0493597d.1579374329.git.berto@igalia.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 83db013814..6cb5aee4a5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2167,10 +2167,7 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
offset, bytes, qiov, qiov_offset);
case QCOW2_CLUSTER_NORMAL:
- if ((file_cluster_offset & 511) != 0) {
- return -EIO;
- }
-
+ assert(offset_into_cluster(s, file_cluster_offset) == 0);
if (bs->encrypted) {
return qcow2_co_preadv_encrypted(bs, file_cluster_offset,
offset, bytes, qiov, qiov_offset);
@@ -2506,7 +2503,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
goto out_locked;
}
- assert((cluster_offset & 511) == 0);
+ assert(offset_into_cluster(s, cluster_offset) == 0);
ret = qcow2_pre_write_overlap_check(bs, 0,
cluster_offset + offset_in_cluster,
@@ -3896,7 +3893,7 @@ qcow2_co_copy_range_to(BlockDriverState *bs,
goto fail;
}
- assert((cluster_offset & 511) == 0);
+ assert(offset_into_cluster(s, cluster_offset) == 0);
ret = qcow2_pre_write_overlap_check(bs, 0,
cluster_offset + offset_in_cluster, cur_bytes, true);
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 13/17] qcow2: Use bs->bl.request_alignment when updating an L1 entry
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (11 preceding siblings ...)
2020-02-06 12:51 ` [PULL 12/17] qcow2: Tighten cluster_offset alignment assertions Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 14/17] qcow2: Don't require aligned offsets in qcow2_co_copy_range_from() Max Reitz
` (4 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
When updating an L1 entry the qcow2 driver writes a (512-byte) sector
worth of data to avoid a read-modify-write cycle. Instead of always
writing 512 bytes we should follow the alignment requirements of the
storage backend.
(the only exception is when the alignment is larger than the cluster
size because then we could be overwriting data after the L1 table)
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-id: 71f34d4ae4b367b32fb36134acbf4f4f7ee681f4.1579374329.git.berto@igalia.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-cluster.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 0384fb2339..1947f13a2d 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -216,26 +216,31 @@ static int l2_load(BlockDriverState *bs, uint64_t offset,
}
/*
- * Writes one sector of the L1 table to the disk (can't update single entries
- * and we really don't want bdrv_pread to perform a read-modify-write)
+ * Writes an L1 entry to disk (note that depending on the alignment
+ * requirements this function may write more that just one entry in
+ * order to prevent bdrv_pwrite from performing a read-modify-write)
*/
-#define L1_ENTRIES_PER_SECTOR (512 / 8)
int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
{
BDRVQcow2State *s = bs->opaque;
- uint64_t buf[L1_ENTRIES_PER_SECTOR] = { 0 };
int l1_start_index;
int i, ret;
+ int bufsize = MAX(sizeof(uint64_t),
+ MIN(bs->file->bs->bl.request_alignment, s->cluster_size));
+ int nentries = bufsize / sizeof(uint64_t);
+ g_autofree uint64_t *buf = g_try_new0(uint64_t, nentries);
- l1_start_index = l1_index & ~(L1_ENTRIES_PER_SECTOR - 1);
- for (i = 0; i < L1_ENTRIES_PER_SECTOR && l1_start_index + i < s->l1_size;
- i++)
- {
+ if (buf == NULL) {
+ return -ENOMEM;
+ }
+
+ l1_start_index = QEMU_ALIGN_DOWN(l1_index, nentries);
+ for (i = 0; i < MIN(nentries, s->l1_size - l1_start_index); i++) {
buf[i] = cpu_to_be64(s->l1_table[l1_start_index + i]);
}
ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L1,
- s->l1_table_offset + 8 * l1_start_index, sizeof(buf), false);
+ s->l1_table_offset + 8 * l1_start_index, bufsize, false);
if (ret < 0) {
return ret;
}
@@ -243,7 +248,7 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
BLKDBG_EVENT(bs->file, BLKDBG_L1_UPDATE);
ret = bdrv_pwrite_sync(bs->file,
s->l1_table_offset + 8 * l1_start_index,
- buf, sizeof(buf));
+ buf, bufsize);
if (ret < 0) {
return ret;
}
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 14/17] qcow2: Don't require aligned offsets in qcow2_co_copy_range_from()
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (12 preceding siblings ...)
2020-02-06 12:51 ` [PULL 13/17] qcow2: Use bs->bl.request_alignment when updating an L1 entry Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 15/17] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value Max Reitz
` (3 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
qemu-img's convert_co_copy_range() operates at the sector level and
block_copy() operates at the cluster level so this condition is always
true, but it is not necessary to restrict this here, so let's leave it
to the driver implementation return an error if there is any.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-id: a4264aaee656910c84161a2965f7a501437379ca.1579374329.git.berto@igalia.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 6cb5aee4a5..ff257d1a6c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3828,10 +3828,6 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
case QCOW2_CLUSTER_NORMAL:
child = s->data_file;
copy_offset += offset_into_cluster(s, src_offset);
- if ((copy_offset & 511) != 0) {
- ret = -EIO;
- goto out;
- }
break;
default:
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 15/17] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (13 preceding siblings ...)
2020-02-06 12:51 ` [PULL 14/17] qcow2: Don't require aligned offsets in qcow2_co_copy_range_from() Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 16/17] block/backup-top: fix failure path Max Reitz
` (2 subsequent siblings)
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
This replaces all remaining instances in the qcow2 code.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-id: b5f74b606c2d9873b12d29acdb7fd498029c4025.1579374329.git.berto@igalia.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index ff257d1a6c..ef96606f8d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3272,7 +3272,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
/* Validate options and set default values */
if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
- error_setg(errp, "Image size must be a multiple of 512 bytes");
+ error_setg(errp, "Image size must be a multiple of %u bytes",
+ (unsigned) BDRV_SECTOR_SIZE);
ret = -EINVAL;
goto out;
}
@@ -3946,8 +3947,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
return -ENOTSUP;
}
- if (offset & 511) {
- error_setg(errp, "The new size must be a multiple of 512");
+ if (!QEMU_IS_ALIGNED(offset, BDRV_SECTOR_SIZE)) {
+ error_setg(errp, "The new size must be a multiple of %u",
+ (unsigned) BDRV_SECTOR_SIZE);
return -EINVAL;
}
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 16/17] block/backup-top: fix failure path
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (14 preceding siblings ...)
2020-02-06 12:51 ` [PULL 15/17] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 12:51 ` [PULL 17/17] iotests: add test for backup-top failure on permission activation Max Reitz
2020-02-06 18:58 ` [PULL 00/17] Block patches Peter Maydell
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
We can't access top after call bdrv_backup_top_drop, as it is already
freed at this time.
Also, no needs to unref target child by hand, it will be unrefed on
bdrv_close() automatically.
So, just do bdrv_backup_top_drop if append succeed and one bdrv_unref
otherwise.
Note, that in !appended case bdrv_unref(top) moved into drained section
on source. It doesn't really matter, but just for code simplicity.
Fixes: 7df7868b96404
Cc: qemu-stable@nongnu.org # v4.2.0
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20200121142802.21467-2-vsementsov@virtuozzo.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/backup-top.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/block/backup-top.c b/block/backup-top.c
index 9aed2eb4c0..fa78f3256d 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -190,6 +190,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
filter_node_name,
BDRV_O_RDWR, errp);
+ bool appended = false;
if (!top) {
return NULL;
@@ -212,8 +213,9 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
bdrv_append(top, source, &local_err);
if (local_err) {
error_prepend(&local_err, "Cannot append backup-top filter: ");
- goto append_failed;
+ goto fail;
}
+ appended = true;
/*
* bdrv_append() finished successfully, now we can require permissions
@@ -224,14 +226,14 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
if (local_err) {
error_prepend(&local_err,
"Cannot set permissions for backup-top filter: ");
- goto failed_after_append;
+ goto fail;
}
state->bcs = block_copy_state_new(top->backing, state->target,
cluster_size, write_flags, &local_err);
if (local_err) {
error_prepend(&local_err, "Cannot create block-copy-state: ");
- goto failed_after_append;
+ goto fail;
}
*bcs = state->bcs;
@@ -239,14 +241,15 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
return top;
-failed_after_append:
- state->active = false;
- bdrv_backup_top_drop(top);
+fail:
+ if (appended) {
+ state->active = false;
+ bdrv_backup_top_drop(top);
+ } else {
+ bdrv_unref(top);
+ }
-append_failed:
bdrv_drained_end(source);
- bdrv_unref_child(top, state->target);
- bdrv_unref(top);
error_propagate(errp, local_err);
return NULL;
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PULL 17/17] iotests: add test for backup-top failure on permission activation
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (15 preceding siblings ...)
2020-02-06 12:51 ` [PULL 16/17] block/backup-top: fix failure path Max Reitz
@ 2020-02-06 12:51 ` Max Reitz
2020-02-06 18:58 ` [PULL 00/17] Block patches Peter Maydell
17 siblings, 0 replies; 32+ messages in thread
From: Max Reitz @ 2020-02-06 12:51 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, Peter Maydell, qemu-devel, Max Reitz
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
This test checks that bug is really fixed by previous commit.
Cc: qemu-stable@nongnu.org # v4.2.0
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20200121142802.21467-3-vsementsov@virtuozzo.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/283 | 92 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/283.out | 8 ++++
tests/qemu-iotests/group | 1 +
3 files changed, 101 insertions(+)
create mode 100644 tests/qemu-iotests/283
create mode 100644 tests/qemu-iotests/283.out
diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
new file mode 100644
index 0000000000..293e557bd9
--- /dev/null
+++ b/tests/qemu-iotests/283
@@ -0,0 +1,92 @@
+#!/usr/bin/env python
+#
+# Test for backup-top filter permission activation failure
+#
+# Copyright (c) 2019 Virtuozzo International GmbH.
+#
+# 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/>.
+#
+
+import iotests
+
+# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+size = 1024 * 1024
+
+""" Test description
+
+When performing a backup, all writes on the source subtree must go through the
+backup-top filter so it can copy all data to the target before it is changed.
+backup-top filter is appended above source node, to achieve this thing, so all
+parents of source node are handled. A configuration with side parents of source
+sub-tree with write permission is unsupported (we'd have append several
+backup-top filter like nodes to handle such parents). The test create an
+example of such configuration and checks that a backup is then not allowed
+(blockdev-backup command should fail).
+
+The configuration:
+
+ ┌────────┐ target ┌─────────────┐
+ │ target │ ◀─────── │ backup_top │
+ └────────┘ └─────────────┘
+ │
+ │ backing
+ ▼
+ ┌─────────────┐
+ │ source │
+ └─────────────┘
+ │
+ │ file
+ ▼
+ ┌─────────────┐ write perm ┌───────┐
+ │ base │ ◀──────────── │ other │
+ └─────────────┘ └───────┘
+
+On activation (see .active field of backup-top state in block/backup-top.c),
+backup-top is going to unshare write permission on its source child. Write
+unsharing will be propagated to the "source->base" link and will conflict with
+other node write permission. So permission update will fail and backup job will
+not be started.
+
+Note, that the only thing which prevents backup of running on such
+configuration is default permission propagation scheme. It may be altered by
+different block drivers, so backup will run in invalid configuration. But
+something is better than nothing. Also, before the previous commit (commit
+preceding this test creation), starting backup on such configuration led to
+crash, so current "something" is a lot better, and this test actual goal is
+to check that crash is fixed :)
+"""
+
+vm = iotests.VM()
+vm.launch()
+
+vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
+
+vm.qmp_log('blockdev-add', **{
+ 'node-name': 'source',
+ 'driver': 'blkdebug',
+ 'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
+})
+
+vm.qmp_log('blockdev-add', **{
+ 'node-name': 'other',
+ 'driver': 'blkdebug',
+ 'image': 'base',
+ 'take-child-perms': ['write']
+})
+
+vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
new file mode 100644
index 0000000000..daaf5828c1
--- /dev/null
+++ b/tests/qemu-iotests/283.out
@@ -0,0 +1,8 @@
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": "source"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}}
+{"return": {}}
+{"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}}
+{"error": {"class": "GenericError", "desc": "Cannot set permissions for backup-top filter: Conflicts with use by other as 'image', which uses 'write' on base"}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 9d30a4b6c4..1904223020 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -289,3 +289,4 @@
279 rw backing quick
280 rw migration quick
281 rw quick
+283 auto quick
--
2.24.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PULL 00/17] Block patches
2020-02-06 12:51 [PULL 00/17] Block patches Max Reitz
` (16 preceding siblings ...)
2020-02-06 12:51 ` [PULL 17/17] iotests: add test for backup-top failure on permission activation Max Reitz
@ 2020-02-06 18:58 ` Peter Maydell
17 siblings, 0 replies; 32+ messages in thread
From: Peter Maydell @ 2020-02-06 18:58 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, QEMU Developers, Qemu-block
On Thu, 6 Feb 2020 at 12:51, Max Reitz <mreitz@redhat.com> wrote:
>
> The following changes since commit 418fa86dd465b4fd8394373cf83db8fa65d7611c:
>
> Merge remote-tracking branch 'remotes/stsquad/tags/pull-testing-040220-1' into staging (2020-02-04 18:55:06 +0000)
>
> are available in the Git repository at:
>
> https://github.com/XanClic/qemu.git tags/pull-block-2020-02-06
>
> for you to fetch changes up to a541fcc27c98b96da187c7d4573f3270f3ddd283:
>
> iotests: add test for backup-top failure on permission activation (2020-02-06 13:47:45 +0100)
>
> ----------------------------------------------------------------
> Block patches:
> - Drop BDRV_SECTOR_SIZE from qcow2
> - Allow Python iotests to be added to the auto group
> (and add some)
> - Fix for the backup job
> - Fix memleak in bdrv_refresh_filename()
> - Use GStrings in two places for greater efficiency (than manually
> handling string allocation)
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 32+ messages in thread