From 3d86af858ec081b2b385b6fd5af09e42ca825a96 Mon Sep 17 00:00:00 2001 From: AlexChen Date: Wed, 21 Oct 2020 17:12:52 +0800 Subject: [PATCH 01/15] block: Remove unused include The "qemu-common.h" include is not used, remove it. Reported-by: Euler Robot Signed-off-by: AlexChen Message-Id: <5F8FFB94.3030209@huawei.com> Signed-off-by: Max Reitz --- block/dmg-lzfse.c | 1 - 1 file changed, 1 deletion(-) diff --git a/block/dmg-lzfse.c b/block/dmg-lzfse.c index 19d25bc646..6798cf4fbf 100644 --- a/block/dmg-lzfse.c +++ b/block/dmg-lzfse.c @@ -22,7 +22,6 @@ * THE SOFTWARE. */ #include "qemu/osdep.h" -#include "qemu-common.h" #include "dmg.h" #include From 009cde17a527993b8bc45da831fe0643229a04ee Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Wed, 28 Oct 2020 09:07:34 +0100 Subject: [PATCH 02/15] block: Move bdrv_drain_all_end_quiesce() to block_int.h This function is really an internal helper for bdrv_close(). Update its doc comment to make this clear and make the function private. Signed-off-by: Greg Kurz Message-Id: <160387245480.131299.13430357162209598411.stgit@bahia> Reviewed-by: Stefan Hajnoczi Signed-off-by: Max Reitz --- include/block/block.h | 6 ------ include/block/block_int.h | 9 +++++++++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/include/block/block.h b/include/block/block.h index 4bfe3b546b..c9d7c58765 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -781,12 +781,6 @@ void bdrv_drained_end(BlockDriverState *bs); */ void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter); -/** - * End all quiescent sections started by bdrv_drain_all_begin(). This is - * only needed when deleting a BDS before bdrv_drain_all_end() is called. - */ -void bdrv_drain_all_end_quiesce(BlockDriverState *bs); - /** * End a quiescent section started by bdrv_subtree_drained_begin(). */ diff --git a/include/block/block_int.h b/include/block/block_int.h index 38cad9d15c..95d9333be1 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1407,4 +1407,13 @@ static inline BlockDriverState *bdrv_primary_bs(BlockDriverState *bs) return child_bs(bdrv_primary_child(bs)); } +/** + * End all quiescent sections started by bdrv_drain_all_begin(). This is + * needed when deleting a BDS before bdrv_drain_all_end() is called. + * + * NOTE: this is an internal helper for bdrv_close() *only*. No one else + * should call it. + */ +void bdrv_drain_all_end_quiesce(BlockDriverState *bs); + #endif /* BLOCK_INT_H */ From 3441ad4bc42ce9d9c6004cd013b91da0a454f143 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Wed, 7 Oct 2020 18:13:23 +0200 Subject: [PATCH 03/15] qcow2: Document and enforce the QCowL2Meta invariants The QCowL2Meta structure is used to store information about a part of a write request that touches clusters that need changes in their L2 entries. This happens with newly-allocated clusters or subclusters. This structure has changed a bit since it was first created and its current documentation is not quite up-to-date. A write request can span a region consisting of a combination of clusters of different types, and qcow2_alloc_host_offset() can repeatedly call handle_copied() and handle_alloc() to add more clusters to the mix as long as they all are contiguous on the image file. Because of this a write request has a list of QCowL2Meta structures, one for each part of the request that needs changes in the L2 metadata. Each one of them spans nb_clusters and has two copy-on-write regions located immediately before and after the middle region touched by that part of the write request. Even when those regions themselves are empty their offsets must be correct because they are used to know the location of the middle region. This was not always the case but it is not a problem anymore because the only two places where QCowL2Meta structures are created (calculate_l2_meta() and qcow2_co_truncate()) ensure that the copy-on-write regions are correctly defined, and so do assertions like the ones in perform_cow(). The conditional initialization of the 'written_to' variable is therefore unnecessary and is removed by this patch. Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201007161323.4667-1-berto@igalia.com> Signed-off-by: Max Reitz --- block/qcow2-cluster.c | 5 +++-- block/qcow2.c | 19 +++++++++++++++---- block/qcow2.h | 19 +++++++++++-------- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index aa87d3e99b..485b4cb92e 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1049,6 +1049,8 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice); assert(l2_index + m->nb_clusters <= s->l2_slice_size); + assert(m->cow_end.offset + m->cow_end.nb_bytes <= + m->nb_clusters << s->cluster_bits); for (i = 0; i < m->nb_clusters; i++) { uint64_t offset = cluster_offset + ((uint64_t)i << s->cluster_bits); /* if two concurrent writes happen to the same unallocated cluster @@ -1070,8 +1072,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m) if (has_subclusters(s) && !m->prealloc) { uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i); unsigned written_from = m->cow_start.offset; - unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes ?: - m->nb_clusters << s->cluster_bits; + unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes; int first_sc, last_sc; /* Narrow written_from and written_to down to the current cluster */ written_from = MAX(written_from, i << s->cluster_bits); diff --git a/block/qcow2.c b/block/qcow2.c index 4274806a2a..1b0733238b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2361,15 +2361,26 @@ static bool merge_cow(uint64_t offset, unsigned bytes, continue; } - /* The data (middle) region must be immediately after the - * start region */ + /* + * The write request should start immediately after the first + * COW region. This does not always happen because the area + * touched by the request can be larger than the one defined + * by @m (a single request can span an area consisting of a + * mix of previously unallocated and allocated clusters, that + * is why @l2meta is a list). + */ if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) { + /* In this case the request starts before this region */ + assert(offset < l2meta_cow_start(m)); + assert(m->cow_start.nb_bytes == 0); continue; } - /* The end region must be immediately after the data (middle) - * region */ + /* The write request should end immediately before the second + * COW region (see above for why it does not always happen) */ if (m->offset + m->cow_end.offset != offset + bytes) { + assert(offset + bytes > m->offset + m->cow_end.offset); + assert(m->cow_end.nb_bytes == 0); continue; } diff --git a/block/qcow2.h b/block/qcow2.h index 125ea9679b..2e0272a7b8 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -435,17 +435,18 @@ typedef struct Qcow2COWRegion { /** * Describes an in-flight (part of a) write request that writes to clusters - * that are not referenced in their L2 table yet. + * that need to have their L2 table entries updated (because they are + * newly allocated or need changes in their L2 bitmaps) */ typedef struct QCowL2Meta { - /** Guest offset of the first newly allocated cluster */ + /** Guest offset of the first updated cluster */ uint64_t offset; - /** Host offset of the first newly allocated cluster */ + /** Host offset of the first updated cluster */ uint64_t alloc_offset; - /** Number of newly allocated clusters */ + /** Number of updated clusters */ int nb_clusters; /** Do not free the old clusters */ @@ -458,14 +459,16 @@ typedef struct QCowL2Meta CoQueue dependent_requests; /** - * The COW Region between the start of the first allocated cluster and the - * area the guest actually writes to. + * The COW Region immediately before the area the guest actually + * writes to. This (part of the) write request starts at + * cow_start.offset + cow_start.nb_bytes. */ Qcow2COWRegion cow_start; /** - * The COW Region between the area the guest actually writes to and the - * end of the last allocated cluster. + * The COW Region immediately after the area the guest actually + * writes to. This (part of the) write request ends at cow_end.offset + * (which must always be set even when cow_end.nb_bytes is 0). */ Qcow2COWRegion cow_end; From 8ce648056fe2e1b071579b66401c8a58ecbebe05 Mon Sep 17 00:00:00 2001 From: Klaus Jensen Date: Wed, 4 Nov 2020 11:22:46 +0100 Subject: [PATCH 04/15] hw/block/nvme: fix null ns in register namespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix dereference after NULL check. Reported-by: Coverity (CID 1436128) Fixes: b20804946bce ("hw/block/nvme: update nsid when registered") Signed-off-by: Klaus Jensen Message-Id: <20201104102248.32168-2-its@irrelevant.dk> Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Max Reitz --- hw/block/nvme.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index fa2cba744b..080d782f1c 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -2562,8 +2562,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) if (!nsid) { for (int i = 1; i <= n->num_namespaces; i++) { - NvmeNamespace *ns = nvme_ns(n, i); - if (!ns) { + if (!nvme_ns(n, i)) { nsid = ns->params.nsid = i; break; } From bf288953f13b4a3c57e6e59656ac5367491c65eb Mon Sep 17 00:00:00 2001 From: Klaus Jensen Date: Wed, 4 Nov 2020 11:22:47 +0100 Subject: [PATCH 05/15] hw/block/nvme: fix uint16_t use of uint32_t sgls member MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit nvme_map_sgl_data erroneously uses the sgls member of NvmeIdNs as a uint16_t. Reported-by: Coverity (CID 1436129) Fixes: cba0a8a344fe ("hw/block/nvme: add support for scatter gather lists") Signed-off-by: Klaus Jensen Message-Id: <20201104102248.32168-3-its@irrelevant.dk> Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Max Reitz --- hw/block/nvme.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 080d782f1c..2bdc50eb6f 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -452,7 +452,7 @@ static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList *qsg, * segments and/or descriptors. The controller might accept * ignoring the rest of the SGL. */ - uint16_t sgls = le16_to_cpu(n->id_ctrl.sgls); + uint32_t sgls = le32_to_cpu(n->id_ctrl.sgls); if (sgls & NVME_CTRL_SGLS_EXCESS_LENGTH) { break; } From 73ad0ff216d2e1cf08909a0597e7b072babfe9c4 Mon Sep 17 00:00:00 2001 From: Klaus Jensen Date: Wed, 4 Nov 2020 11:22:48 +0100 Subject: [PATCH 06/15] hw/block/nvme: fix free of array-typed value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces"), the namespaces member of NvmeCtrl is no longer a dynamically allocated array. Remove the free. Fixes: 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces") Reported-by: Coverity (CID 1436131) Signed-off-by: Klaus Jensen Message-Id: <20201104102248.32168-4-its@irrelevant.dk> Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Max Reitz --- hw/block/nvme.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 2bdc50eb6f..01b657b1c5 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -2799,7 +2799,6 @@ static void nvme_exit(PCIDevice *pci_dev) NvmeCtrl *n = NVME(pci_dev); nvme_clear_ctrl(n); - g_free(n->namespaces); g_free(n->cq); g_free(n->sq); g_free(n->aer_reqs); From 2daba442059a838f8f3c80d8fa52a85768ba7c4c Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Wed, 4 Nov 2020 20:50:24 +0200 Subject: [PATCH 07/15] iotests: add filter_qmp_virtio_scsi function filter_qmp_virtio_scsi can be used to filter virtio-scsi-pci/ccw differences. Note that this patch was only tested on x86. Suggested-by: Paolo Bonzini Signed-off-by: Maxim Levitsky Tested-by: Christian Borntraeger Reviewed-by: Paolo Bonzini Message-Id: <20201104185025.434703-2-mlevitsk@redhat.com> Signed-off-by: Max Reitz --- 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 814804a4c6..bcd4fe5b6f 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -392,6 +392,16 @@ def filter_qmp_testfiles(qmsg): return value return filter_qmp(qmsg, _filter) +def filter_virtio_scsi(output: str) -> str: + return re.sub(r'(virtio-scsi)-(ccw|pci)', r'\1', output) + +def filter_qmp_virtio_scsi(qmsg): + def _filter(_key, value): + if is_str(value): + return filter_virtio_scsi(value) + return value + return filter_qmp(qmsg, _filter) + def filter_generated_node_ids(msg): return re.sub("#block[0-9]+", "NODE_NAME", msg) From c6ac463631a124eaa47cae8a9a4aaac4d0761d28 Mon Sep 17 00:00:00 2001 From: Maxim Levitsky Date: Wed, 4 Nov 2020 20:50:25 +0200 Subject: [PATCH 08/15] iotests: rewrite iotest 240 in python The recent changes that brought RCU delayed device deletion, broke few tests and this test breakage went unnoticed. Fix this test by rewriting it in python (which allows to wait for DEVICE_DELETED events before continuing). Signed-off-by: Maxim Levitsky Tested-by: Christian Borntraeger Reviewed-by: Paolo Bonzini Message-Id: <20201104185025.434703-3-mlevitsk@redhat.com> Signed-off-by: Max Reitz --- tests/qemu-iotests/240 | 183 ++++++++++++++----------------------- tests/qemu-iotests/240.out | 80 ++++++++-------- 2 files changed, 114 insertions(+), 149 deletions(-) diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240 index 8b4337b58d..c0f71f0461 100755 --- a/tests/qemu-iotests/240 +++ b/tests/qemu-iotests/240 @@ -1,5 +1,5 @@ -#!/usr/bin/env bash -# +#!/usr/bin/env python3 + # Test hot plugging and unplugging with iothreads # # Copyright (C) 2019 Igalia, S.L. @@ -17,133 +17,90 @@ # # You should have received a copy of the GNU General Public License # along with this program. If not, see . -# -# creator -owner=berto@igalia.com +import iotests +import os -seq=`basename $0` -echo "QA output created by $seq" +nbd_sock = iotests.file_path('nbd.sock', base_dir=iotests.sock_dir) -status=1 # failure is the default! +class TestCase(iotests.QMPTestCase): + test_driver = "null-co" -_cleanup() -{ - rm -f "$SOCK_DIR/nbd" -} -trap "_cleanup; exit \$status" 0 1 2 3 15 + def required_drivers(self): + return [self.test_driver] -# get standard environment, filters and checks -. ./common.rc -. ./common.filter + @iotests.skip_if_unsupported(required_drivers) + def setUp(self): + self.vm = iotests.VM() + self.vm.launch() -_supported_fmt generic -_supported_proto generic + def tearDown(self): + self.vm.shutdown() -do_run_qemu() -{ - echo Testing: "$@" - $QEMU -nographic -qmp stdio -serial none "$@" - echo -} + def test1(self): + iotests.log('==Unplug a SCSI disk and then plug it again==') + self.vm.qmp_log('blockdev-add', driver='null-co', read_zeroes=True, node_name='hd0') + self.vm.qmp_log('object-add', qom_type='iothread', id="iothread0") + self.vm.qmp_log('device_add', id='scsi0', driver=iotests.get_virtio_scsi_device(), iothread='iothread0', filters=[iotests.filter_qmp_virtio_scsi]) + self.vm.qmp_log('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0') + self.vm.qmp_log('device_del', id='scsi-hd0') + self.vm.event_wait('DEVICE_DELETED') + self.vm.qmp_log('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0') + self.vm.qmp_log('device_del', id='scsi-hd0') + self.vm.event_wait('DEVICE_DELETED') + self.vm.qmp_log('blockdev-del', node_name='hd0') -# Remove QMP events from (pretty-printed) output. Doesn't handle -# nested dicts correctly, but we don't get any of those in this test. -_filter_qmp_events() -{ - tr '\n' '\t' | sed -e \ - 's/{\s*"timestamp":\s*{[^}]*},\s*"event":[^,}]*\(,\s*"data":\s*{[^}]*}\)\?\s*}\s*//g' \ - | tr '\t' '\n' -} + def test2(self): + iotests.log('==Attach two SCSI disks using the same block device and the same iothread==') + self.vm.qmp_log('blockdev-add', driver='null-co', read_zeroes=True, node_name='hd0', read_only=True) + self.vm.qmp_log('object-add', qom_type='iothread', id="iothread0") + self.vm.qmp_log('device_add', id='scsi0', driver=iotests.get_virtio_scsi_device(), iothread='iothread0', filters=[iotests.filter_qmp_virtio_scsi]) -run_qemu() -{ - do_run_qemu "$@" 2>&1 | _filter_qmp | _filter_qmp_events -} + self.vm.qmp_log('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0') + self.vm.qmp_log('device_add', id='scsi-hd1', driver='scsi-hd', drive='hd0') + self.vm.qmp_log('device_del', id='scsi-hd0') + self.vm.event_wait('DEVICE_DELETED') + self.vm.qmp_log('device_del', id='scsi-hd1') + self.vm.event_wait('DEVICE_DELETED') + self.vm.qmp_log('blockdev-del', node_name='hd0') -case "$QEMU_DEFAULT_MACHINE" in - s390-ccw-virtio) - virtio_scsi=virtio-scsi-ccw - ;; - *) - virtio_scsi=virtio-scsi-pci - ;; -esac + def test3(self): + iotests.log('==Attach two SCSI disks using the same block device but different iothreads==') -echo -echo === Unplug a SCSI disk and then plug it again === -echo + self.vm.qmp_log('blockdev-add', driver='null-co', read_zeroes=True, node_name='hd0', read_only=True) -run_qemu < Date: Thu, 5 Nov 2020 20:31:15 +0800 Subject: [PATCH 09/15] block: Fixes nfs compiling error on msys2/mingw These compiling errors are fixed: ../block/nfs.c:27:10: fatal error: poll.h: No such file or directory 27 | #include | ^~~~~~~~ compilation terminated. ../block/nfs.c:63:5: error: unknown type name 'blkcnt_t' 63 | blkcnt_t st_blocks; | ^~~~~~~~ ../block/nfs.c: In function 'nfs_client_open': ../block/nfs.c:550:27: error: 'struct _stat64' has no member named 'st_blocks' 550 | client->st_blocks = st.st_blocks; | ^ ../block/nfs.c: In function 'nfs_get_allocated_file_size': ../block/nfs.c:751:41: error: 'struct _stat64' has no member named 'st_blocks' 751 | return (task.ret < 0 ? task.ret : st.st_blocks * 512); | ^ ../block/nfs.c: In function 'nfs_reopen_prepare': ../block/nfs.c:805:31: error: 'struct _stat64' has no member named 'st_blocks' 805 | client->st_blocks = st.st_blocks; | ^ ../block/nfs.c: In function 'nfs_get_allocated_file_size': ../block/nfs.c:752:1: error: control reaches end of non-void function [-Werror=return-type] 752 | } | ^ On msys2/mingw, there is no st_blocks in struct _stat64 yet, we disable the usage of it on msys2/mingw, and create a typedef long long blkcnt_t; for further implementation Signed-off-by: Yonggang Luo Message-Id: <20201105123116.674-2-luoyonggang@gmail.com> Signed-off-by: Max Reitz --- block/nfs.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/block/nfs.c b/block/nfs.c index f86e660374..77905f516d 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -24,7 +24,9 @@ #include "qemu/osdep.h" +#if !defined(_WIN32) #include +#endif #include "qemu/config-file.h" #include "qemu/error-report.h" #include "qapi/error.h" @@ -58,7 +60,7 @@ typedef struct NFSClient { bool has_zero_init; AioContext *aio_context; QemuMutex mutex; - blkcnt_t st_blocks; + uint64_t st_blocks; bool cache_used; NFSServer *server; char *path; @@ -545,7 +547,9 @@ static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts, } ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE); +#if !defined(_WIN32) client->st_blocks = st.st_blocks; +#endif client->has_zero_init = S_ISREG(st.st_mode); *strp = '/'; goto out; @@ -706,6 +710,7 @@ static int nfs_has_zero_init(BlockDriverState *bs) return client->has_zero_init; } +#if !defined(_WIN32) /* Called (via nfs_service) with QemuMutex held. */ static void nfs_get_allocated_file_size_cb(int ret, struct nfs_context *nfs, void *data, @@ -748,6 +753,7 @@ static int64_t nfs_get_allocated_file_size(BlockDriverState *bs) return (task.ret < 0 ? task.ret : st.st_blocks * 512); } +#endif static int coroutine_fn nfs_file_co_truncate(BlockDriverState *bs, int64_t offset, bool exact, @@ -800,7 +806,9 @@ static int nfs_reopen_prepare(BDRVReopenState *state, nfs_get_error(client->context)); return ret; } +#if !defined(_WIN32) client->st_blocks = st.st_blocks; +#endif } return 0; @@ -869,7 +877,10 @@ static BlockDriver bdrv_nfs = { .create_opts = &nfs_create_opts, .bdrv_has_zero_init = nfs_has_zero_init, +/* libnfs does not provide the allocated filesize of a file on win32. */ +#if !defined(_WIN32) .bdrv_get_allocated_file_size = nfs_get_allocated_file_size, +#endif .bdrv_co_truncate = nfs_file_co_truncate, .bdrv_file_open = nfs_file_open, From 7c5c53830636a9da263a9be5b510ac528bbd75d5 Mon Sep 17 00:00:00 2001 From: Yonggang Luo Date: Thu, 5 Nov 2020 20:31:16 +0800 Subject: [PATCH 10/15] block: enable libnfs on msys2/mingw in cirrus.yml Initially, libnfs has not been enabled, and now it's fixed, so enable it on cirrus. Signed-off-by: Yonggang Luo Message-Id: <20201105123116.674-3-luoyonggang@gmail.com> Signed-off-by: Max Reitz --- .cirrus.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.cirrus.yml b/.cirrus.yml index 900437dd2a..f0209b7a3e 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -109,6 +109,7 @@ windows_msys2_task: mingw-w64-x86_64-cyrus-sasl \ mingw-w64-x86_64-curl \ mingw-w64-x86_64-gnutls \ + mingw-w64-x86_64-libnfs \ " bitsadmin /transfer msys_download /dynamic /download /priority FOREGROUND ` https://repo.msys2.org/mingw/x86_64/mingw-w64-x86_64-python-sphinx-2.3.1-1-any.pkg.tar.xz ` From 122860bae7c3a3cf45f9f2dedddb0e2492f09888 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 5 Nov 2020 09:51:22 -0600 Subject: [PATCH 11/15] block: Fix integer promotion error in bdrv_getlength() Back in 2015, we attempted to fix error reporting for images that claimed to have more than INT64_MAX/512 sectors, but due to the type promotions caused by BDRV_SECTOR_SIZE being unsigned, this inadvertently forces all negative ret values to be slammed into -EFBIG rather than the original error. While we're at it, we can avoid the confusing ?: by spelling the logic more directly. Fixes: 4a9c9ea0d3 Reported-by: Guoyi Tu Signed-off-by: Eric Blake Message-Id: <20201105155122.60943-1-eblake@redhat.com> Reviewed-by: Alberto Garcia Signed-off-by: Max Reitz --- block.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/block.c b/block.c index 56bacc9e9f..2fd932154e 100644 --- a/block.c +++ b/block.c @@ -5091,8 +5091,13 @@ int64_t bdrv_getlength(BlockDriverState *bs) { int64_t ret = bdrv_nb_sectors(bs); - ret = ret > INT64_MAX / BDRV_SECTOR_SIZE ? -EFBIG : ret; - return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE; + if (ret < 0) { + return ret; + } + if (ret > INT64_MAX / BDRV_SECTOR_SIZE) { + return -EFBIG; + } + return ret * BDRV_SECTOR_SIZE; } /* return 0 as number of sectors if no device present or error */ From 5f14f31d2bbb2c00b59c7e9cbbd584d0cee80273 Mon Sep 17 00:00:00 2001 From: shiliyang Date: Fri, 30 Oct 2020 11:35:12 +0800 Subject: [PATCH 12/15] block: Fix some code style problems, "foo* bar" should be "foo *bar" There have some code style problems be found when read the block driver code. So I fixes some problems of this error, ERROR: "foo* bar" should be "foo *bar". Signed-off-by: Liyang Shi Reported-by: Euler Robot Message-Id: <3211f389-6d22-46c1-4a16-e6a2ba66f070@huawei.com> Signed-off-by: Max Reitz --- block/blkdebug.c | 2 +- block/dmg.c | 2 +- block/qcow2.c | 4 ++-- block/qcow2.h | 6 +++--- block/vpc.c | 10 +++++----- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/block/blkdebug.c b/block/blkdebug.c index 54da719dd1..5fe6172da9 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -173,7 +173,7 @@ static int add_rule(void *opaque, QemuOpts *opts, Error **errp) { struct add_rule_data *d = opaque; BDRVBlkdebugState *s = d->s; - const char* event_name; + const char *event_name; int event; struct BlkdebugRule *rule; int64_t sector; diff --git a/block/dmg.c b/block/dmg.c index 0d6c317296..ef35a505f2 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -559,7 +559,7 @@ static void dmg_refresh_limits(BlockDriverState *bs, Error **errp) bs->bl.request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O */ } -static inline int is_sector_in_chunk(BDRVDMGState* s, +static inline int is_sector_in_chunk(BDRVDMGState *s, uint32_t chunk_num, uint64_t sector_num) { if (chunk_num >= s->n_chunks || s->sectors[chunk_num] > sector_num || diff --git a/block/qcow2.c b/block/qcow2.c index 1b0733238b..3a90ef2786 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -269,7 +269,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset, case QCOW2_EXT_MAGIC_FEATURE_TABLE: if (p_feature_table != NULL) { - void* feature_table = g_malloc0(ext.len + 2 * sizeof(Qcow2Feature)); + void *feature_table = g_malloc0(ext.len + 2 * sizeof(Qcow2Feature)); ret = bdrv_pread(bs->file, offset , feature_table, ext.len); if (ret < 0) { error_setg_errno(errp, -ret, "ERROR: ext_feature_table: " @@ -3388,7 +3388,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) size_t cluster_size; int version; int refcount_order; - uint64_t* refcount_table; + uint64_t *refcount_table; int ret; uint8_t compression_type = QCOW2_COMPRESSION_TYPE_ZLIB; diff --git a/block/qcow2.h b/block/qcow2.h index 2e0272a7b8..0678073b74 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -343,8 +343,8 @@ typedef struct BDRVQcow2State { uint64_t l1_table_offset; uint64_t *l1_table; - Qcow2Cache* l2_table_cache; - Qcow2Cache* refcount_block_cache; + Qcow2Cache *l2_table_cache; + Qcow2Cache *refcount_block_cache; QEMUTimer *cache_clean_timer; unsigned cache_clean_interval; @@ -394,7 +394,7 @@ typedef struct BDRVQcow2State { uint64_t autoclear_features; size_t unknown_header_fields_size; - void* unknown_header_fields; + void *unknown_header_fields; QLIST_HEAD(, Qcow2UnknownHeaderExtension) unknown_header_ext; QTAILQ_HEAD (, Qcow2DiscardRegion) discards; bool cache_discards; diff --git a/block/vpc.c b/block/vpc.c index 890554277e..1ab55f9287 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -172,7 +172,7 @@ static QemuOptsList vpc_runtime_opts = { static QemuOptsList vpc_create_opts; -static uint32_t vpc_checksum(uint8_t* buf, size_t size) +static uint32_t vpc_checksum(uint8_t *buf, size_t size) { uint32_t res = 0; int i; @@ -528,7 +528,7 @@ static inline int64_t get_image_offset(BlockDriverState *bs, uint64_t offset, * * Returns 0 on success and < 0 on error */ -static int rewrite_footer(BlockDriverState* bs) +static int rewrite_footer(BlockDriverState *bs) { int ret; BDRVVPCState *s = bs->opaque; @@ -548,7 +548,7 @@ static int rewrite_footer(BlockDriverState* bs) * * Returns the sectors' offset in the image file on success and < 0 on error */ -static int64_t alloc_block(BlockDriverState* bs, int64_t offset) +static int64_t alloc_block(BlockDriverState *bs, int64_t offset) { BDRVVPCState *s = bs->opaque; int64_t bat_offset; @@ -781,8 +781,8 @@ static int coroutine_fn vpc_co_block_status(BlockDriverState *bs, * the hardware EIDE and ATA-2 limit of 16 heads (max disk size of 127 GB) * and instead allow up to 255 heads. */ -static int calculate_geometry(int64_t total_sectors, uint16_t* cyls, - uint8_t* heads, uint8_t* secs_per_cyl) +static int calculate_geometry(int64_t total_sectors, uint16_t *cyls, + uint8_t *heads, uint8_t *secs_per_cyl) { uint32_t cyls_times_heads; From 6c5f7b3a1002ebe552782de4f3664a486d444323 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 6 Nov 2020 15:42:35 +0300 Subject: [PATCH 13/15] block: add forgotten bdrv_abort_perm_update() to bdrv_co_invalidate_cache() Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201106124241.16950-2-vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia Signed-off-by: Max Reitz --- block.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block.c b/block.c index 2fd932154e..1e68bd2f5f 100644 --- a/block.c +++ b/block.c @@ -5787,6 +5787,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp) bdrv_get_cumulative_perm(bs, &perm, &shared_perm); ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp); if (ret < 0) { + bdrv_abort_perm_update(bs); bs->open_flags |= BDRV_O_INACTIVE; return ret; } From 313274bbd4677f44631921ef4366f4ffc81cc5d5 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 6 Nov 2020 15:42:36 +0300 Subject: [PATCH 14/15] block: add bdrv_replace_node_common() Add new parameter to bdrv_replace_node(): auto_skip. With auto_skip=false we'll have stricter behavior: update _all_ from parents or fail. New behaviour will be used in the following commit in block.c, so keep original function name as public interface. Note: new error message is a bit funny in contrast with further "Cannot" in case of frozen child, but we'd better keep some difference to make it possible to distinguish one from another on failure. Still, actually we'd better refactor should_update_child() call to distinguish also different kinds of "should not". Let's do it later. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201106124241.16950-3-vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia Signed-off-by: Max Reitz --- block.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 1e68bd2f5f..9a945a058d 100644 --- a/block.c +++ b/block.c @@ -4563,8 +4563,16 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to) return ret; } -void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, - Error **errp) +/* + * With auto_skip=true bdrv_replace_node_common skips updating from parents + * if it creates a parent-child relation loop or if parent is block-job. + * + * With auto_skip=false the error is returned if from has a parent which should + * not be updated. + */ +static void bdrv_replace_node_common(BlockDriverState *from, + BlockDriverState *to, + bool auto_skip, Error **errp) { BdrvChild *c, *next; GSList *list = NULL, *p; @@ -4583,7 +4591,12 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { assert(c->bs == from); if (!should_update_child(c, to)) { - continue; + if (auto_skip) { + continue; + } + error_setg(errp, "Should not change '%s' link to '%s'", + c->name, from->node_name); + goto out; } if (c->frozen) { error_setg(errp, "Cannot change '%s' link to '%s'", @@ -4623,6 +4636,12 @@ out: bdrv_unref(from); } +void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, + Error **errp) +{ + return bdrv_replace_node_common(from, to, true, errp); +} + /* * Add new bs contents at the top of an image chain while the chain is * live, while keeping required fields on the top layer. From d669ed6ab028497d634e1f236c74a98725f9e45f Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Fri, 6 Nov 2020 15:42:37 +0300 Subject: [PATCH 15/15] block: make bdrv_drop_intermediate() less wrong First, permission update loop tries to do iterations transactionally, but the whole update is not transactional: nobody roll-back successful loop iterations when some iteration fails. Second, in the iteration we have nested permission update: c->klass->update_filename may point to bdrv_child_cb_update_filename() which calls bdrv_backing_update_filename(), which may do node reopen to RW. Permission update system is not prepared to nested updates, at least it has intermediate permission-update state stored in BdrvChild structures: has_backup_perm, backup_perm and backup_shared_perm. So, let's first do bdrv_replace_node_common() (which is more transactional than open-coded update in bdrv_drop_intermediate()) and then call update_filename() in separate. We still do not rollback changes in case of update_filename() failure but it's not much worse than pre-patch behavior. Note that bdrv_replace_node_common() does check for frozen children, so corresponding check is dropped in bdrv_drop_intermediate(). Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20201106124241.16950-4-vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia Signed-off-by: Max Reitz --- block.c | 54 ++++++++++++++++++++++++------------------------------ 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/block.c b/block.c index 9a945a058d..f1cedac362 100644 --- a/block.c +++ b/block.c @@ -4910,9 +4910,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, { BlockDriverState *explicit_top = top; bool update_inherits_from; - BdrvChild *c, *next; + BdrvChild *c; Error *local_err = NULL; int ret = -EIO; + g_autoptr(GSList) updated_children = NULL; + GSList *p; bdrv_ref(top); bdrv_subtree_drained_begin(top); @@ -4926,14 +4928,6 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, goto exit; } - /* This function changes all links that point to top and makes - * them point to base. Check that none of them is frozen. */ - QLIST_FOREACH(c, &top->parents, next_parent) { - if (c->frozen) { - goto exit; - } - } - /* If 'base' recursively inherits from 'top' then we should set * base->inherits_from to top->inherits_from after 'top' and all * other intermediate nodes have been dropped. @@ -4950,36 +4944,36 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *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 */ - GSList *ignore_children = g_slist_prepend(NULL, c); - ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm, - ignore_children, NULL, &local_err); - g_slist_free(ignore_children); - if (ret < 0) { - error_report_err(local_err); - goto exit; - } + QLIST_FOREACH(c, &top->parents, next_parent) { + updated_children = g_slist_prepend(updated_children, c); + } + + bdrv_replace_node_common(top, base, false, &local_err); + if (local_err) { + error_report_err(local_err); + goto exit; + } + + for (p = updated_children; p; p = p->next) { + c = p->data; - /* If so, update the backing file path in the image file */ if (c->klass->update_filename) { ret = c->klass->update_filename(c, base, backing_file_str, &local_err); if (ret < 0) { - bdrv_abort_perm_update(base); + /* + * TODO: Actually, we want to rollback all previous iterations + * of this loop, and (which is almost impossible) previous + * bdrv_replace_node()... + * + * Note, that c->klass->update_filename may lead to permission + * update, so it's a bad idea to call it inside permission + * update transaction of bdrv_replace_node. + */ error_report_err(local_err); goto exit; } } - - /* - * Do the actual switch in the in-memory graph. - * Completes bdrv_check_update_perm() transaction internally. - * c->frozen is false, we have checked that above. - */ - bdrv_ref(base); - bdrv_replace_child(c, base); - bdrv_unref(top); } if (update_inherits_from) {