From 50a3efb0f05bcfbe04201d4ebac0b96551a1b551 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Mon, 6 Nov 2017 16:53:45 +0200 Subject: [PATCH 1/2] block: Close a BlockDriverState completely even when bs->drv is NULL bdrv_close() skips much of its logic when bs->drv is NULL. This is fine when we're closing a BlockDriverState that has just been created (because e.g the initialization process failed), but it's not enough in other cases. For example, when a valid qcow2 image is found to be corrupted then QEMU marks it as such in the file header and then sets bs->drv to NULL in order to make the BlockDriverState unusable. When that BDS is later closed then many of its data structures are not freed (leaking their memory) and none of its children are detached. This results in bdrv_close_all() failing to close all BDSs and making this assertion fail when QEMU is being shut down: bdrv_close_all: Assertion `QTAILQ_EMPTY(&all_bdrv_states)' failed. This patch makes bdrv_close() do the full uninitialization process in all cases. This fixes the problem with corrupted images and still works fine with freshly created BDSs. Signed-off-by: Alberto Garcia Message-id: 20171106145345.12038-1-berto@igalia.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- block.c | 65 +++++++++++++++++++------------------- tests/qemu-iotests/060 | 13 ++++++++ tests/qemu-iotests/060.out | 12 +++++++ 3 files changed, 57 insertions(+), 33 deletions(-) diff --git a/block.c b/block.c index 68b724206d..9a1a0d1e73 100644 --- a/block.c +++ b/block.c @@ -3198,6 +3198,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state) static void bdrv_close(BlockDriverState *bs) { BdrvAioNotifier *ban, *ban_next; + BdrvChild *child, *next; assert(!bs->job); assert(!bs->refcnt); @@ -3207,43 +3208,41 @@ static void bdrv_close(BlockDriverState *bs) bdrv_drain(bs); /* in case flush left pending I/O */ if (bs->drv) { - BdrvChild *child, *next; - bs->drv->bdrv_close(bs); bs->drv = NULL; - - bdrv_set_backing_hd(bs, NULL, &error_abort); - - if (bs->file != NULL) { - bdrv_unref_child(bs, bs->file); - bs->file = NULL; - } - - QLIST_FOREACH_SAFE(child, &bs->children, next, next) { - /* TODO Remove bdrv_unref() from drivers' close function and use - * bdrv_unref_child() here */ - if (child->bs->inherits_from == bs) { - child->bs->inherits_from = NULL; - } - bdrv_detach_child(child); - } - - g_free(bs->opaque); - bs->opaque = NULL; - atomic_set(&bs->copy_on_read, 0); - bs->backing_file[0] = '\0'; - bs->backing_format[0] = '\0'; - bs->total_sectors = 0; - bs->encrypted = false; - bs->sg = false; - QDECREF(bs->options); - QDECREF(bs->explicit_options); - bs->options = NULL; - bs->explicit_options = NULL; - QDECREF(bs->full_open_options); - bs->full_open_options = NULL; } + bdrv_set_backing_hd(bs, NULL, &error_abort); + + if (bs->file != NULL) { + bdrv_unref_child(bs, bs->file); + bs->file = NULL; + } + + QLIST_FOREACH_SAFE(child, &bs->children, next, next) { + /* TODO Remove bdrv_unref() from drivers' close function and use + * bdrv_unref_child() here */ + if (child->bs->inherits_from == bs) { + child->bs->inherits_from = NULL; + } + bdrv_detach_child(child); + } + + g_free(bs->opaque); + bs->opaque = NULL; + atomic_set(&bs->copy_on_read, 0); + bs->backing_file[0] = '\0'; + bs->backing_format[0] = '\0'; + bs->total_sectors = 0; + bs->encrypted = false; + bs->sg = false; + QDECREF(bs->options); + QDECREF(bs->explicit_options); + bs->options = NULL; + bs->explicit_options = NULL; + QDECREF(bs->full_open_options); + bs->full_open_options = NULL; + bdrv_release_named_dirty_bitmaps(bs); assert(QLIST_EMPTY(&bs->dirty_bitmaps)); diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060 index 1eca09417b..14797dd3b0 100755 --- a/tests/qemu-iotests/060 +++ b/tests/qemu-iotests/060 @@ -426,6 +426,19 @@ echo '--- Repairing ---' _check_test_img -q -r all _check_test_img -r all +echo +echo "=== Testing the QEMU shutdown with a corrupted image ===" +echo +_make_test_img 64M +poke_file "$TEST_IMG" "$rt_offset" "\x00\x00\x00\x00\x00\x00\x00\x00" +echo "{'execute': 'qmp_capabilities'} + {'execute': 'human-monitor-command', + 'arguments': {'command-line': 'qemu-io drive \"write 0 512\"'}} + {'execute': 'quit'}" \ + | $QEMU -qmp stdio -nographic -nodefaults \ + -drive if=none,node-name=drive,file="$TEST_IMG",driver=qcow2 \ + | _filter_qmp | _filter_qemu_io + # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out index 56f5eb15d8..c4cb7c665e 100644 --- a/tests/qemu-iotests/060.out +++ b/tests/qemu-iotests/060.out @@ -399,4 +399,16 @@ The following inconsistencies were found and repaired: Double checking the fixed image now... No errors were found on the image. + +=== Testing the QEMU shutdown with a corrupted image === + +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 +qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with refcount table); further corruption events will be suppressed +QMP_VERSION +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "none0", "msg": "Preventing invalid write on metadata (overlaps with refcount table)", "offset": 65536, "node-name": "drive", "fatal": true, "size": 65536}} +write failed: Input/output error +{"return": ""} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} *** done From 2807746ff178fe2e62638755693ece57aeeacc05 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 17 Nov 2017 13:04:22 -0600 Subject: [PATCH 2/2] iotests: Fix 176 on 32-bit host The contents of a qcow2 bitmap are rounded up to a size that matches the number of bits available for the granularity, but that granularity differs for 32-bit hosts (our default 64k cluster allows for 2M bitmap coverage per 'long') and 64-bit hosts (4M bitmap per 'long'). If the image is a multiple of 2M but not 4M, then the number of bytes occupied by the array of longs in memory differs between architecture, thus resulting in different SHA256 hashes. Furthermore (but untested by me), if our computation of the SHA256 hash is at all endian-dependent because of how we store data in memory, that's another variable we'd have to account for (ideally, we specified the bitmap stored in qcow2 as fixed-endian on disk, because the same qcow2 file must be usable across any architecture; but that says nothing about how we represent things in memory). But we already have test 165 to validate that bitmaps are stored correctly on disk, while this test is merely testing that the bitmap exists. So for this test, the easiest solution is to filter out the actual hash value. Broken in commit 4096974e. Reported-by: Max Reitz Signed-off-by: Eric Blake Message-id: 20171117190422.23626-1-eblake@redhat.com Reviewed-by: John Snow Reviewed-by: Max Reitz Signed-off-by: Max Reitz --- tests/qemu-iotests/176 | 3 ++- tests/qemu-iotests/176.out | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/qemu-iotests/176 b/tests/qemu-iotests/176 index 0f31a20294..b8dc17c592 100755 --- a/tests/qemu-iotests/176 +++ b/tests/qemu-iotests/176 @@ -52,7 +52,8 @@ _supported_os Linux function run_qemu() { $QEMU -nographic -qmp stdio -serial none "$@" 2>&1 \ - | _filter_testdir | _filter_qmp | _filter_qemu + | _filter_testdir | _filter_qmp | _filter_qemu \ + | sed 's/"sha256": ".\{64\}"/"sha256": HASH/' } for reason in snapshot bitmap; do diff --git a/tests/qemu-iotests/176.out b/tests/qemu-iotests/176.out index e62085cd0a..f03a2e776c 100644 --- a/tests/qemu-iotests/176.out +++ b/tests/qemu-iotests/176.out @@ -205,7 +205,7 @@ Offset Length File QMP_VERSION {"return": {}} {"return": {}} -{"return": {"sha256": "e12600978d86b5a453861ae5c17d275204673fef3874b7c3c5433c6153d84706"}} +{"return": {"sha256": HASH}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} @@ -255,7 +255,7 @@ Offset Length File QMP_VERSION {"return": {}} {"return": {}} -{"return": {"sha256": "e12600978d86b5a453861ae5c17d275204673fef3874b7c3c5433c6153d84706"}} +{"return": {"sha256": HASH}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} @@ -305,7 +305,7 @@ Offset Length File QMP_VERSION {"return": {}} {"return": {}} -{"return": {"sha256": "e12600978d86b5a453861ae5c17d275204673fef3874b7c3c5433c6153d84706"}} +{"return": {"sha256": HASH}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} @@ -352,7 +352,7 @@ Offset Length File QMP_VERSION {"return": {}} {"return": {}} -{"return": {"sha256": "e12600978d86b5a453861ae5c17d275204673fef3874b7c3c5433c6153d84706"}} +{"return": {"sha256": HASH}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} *** done