From 53dd4015acb48bad5aee5ba707848370dc442077 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Tue, 1 Aug 2017 17:31:27 -0400 Subject: [PATCH 01/18] qemu-iotests/109: Fix lock race condition A race condition is currently present between the clean up attempt of the QEMU process and the execution of qemu-img. The actual (bad) output is: -Warning: Image size mismatch! -Images are identical. +qemu-img: Could not open '/tests/qemu-iotests/scratch/t.raw': Failed to get "consistent read" lock +Is another process using the image? A KILL signal is sent to the QEMU process, but qemu-img may begin to run before the QEMU process is really gone. qemu-img will then attempt to open the TEST_IMG file before it can secure a lock on it. This attempts a more graceful shutdown, and waits for the QEMU process to exit. Signed-off-by: Cleber Rosa Reviewed-by: Eric Blake Reviewed-by: Jeff Cody Reviewed-by: John Snow Signed-off-by: Kevin Wolf --- tests/qemu-iotests/109 | 3 +- tests/qemu-iotests/109.out | 56 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109 index 3b496a3918..d70b574d88 100755 --- a/tests/qemu-iotests/109 +++ b/tests/qemu-iotests/109 @@ -67,7 +67,8 @@ function run_qemu() _send_qemu_cmd $QEMU_HANDLE '' "BLOCK_JOB_COMPLETED" fi _send_qemu_cmd $QEMU_HANDLE '{"execute":"query-block-jobs"}' "return" - _cleanup_qemu + _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return" + wait=1 _cleanup_qemu } for fmt in qcow qcow2 qed vdi vmdk vpc; do diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out index dc02f9eefa..c189e2833d 100644 --- a/tests/qemu-iotests/109.out +++ b/tests/qemu-iotests/109.out @@ -12,12 +12,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -33,12 +38,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 512, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 197120, "offset": 197120, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -54,12 +64,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 262144, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 327680, "offset": 327680, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -75,12 +90,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 1024, "offset": 1024, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -96,12 +116,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 65536, "offset": 65536, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -117,12 +142,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": 0, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -137,12 +167,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2560, "offset": 2560, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -157,12 +192,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 31457280, "offset": 31457280, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 31457280, "offset": 31457280, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 31457280, "offset": 31457280, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -177,12 +217,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 327680, "offset": 327680, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -197,12 +242,17 @@ Specify the 'raw' format explicitly to remove the restrictions. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_ERROR", "data": {"device": "src", "operation": "write", "action": "report"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": LEN, "offset": OFFSET, "speed": 0, "type": "mirror", "error": "Operation not permitted"}} {"return": []} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} read 65536/65536 bytes at offset 0 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 2048, "offset": 2048, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. @@ -216,12 +266,18 @@ Specify the 'raw' format explicitly to remove the restrictions. {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} {"return": [{"io-status": "ok", "device": "src", "busy": false, "len": 512, "offset": 512, "paused": false, "speed": 0, "ready": true, "type": "mirror"}]} +{"return": {}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} Warning: Image size mismatch! Images are identical. *** done From 795be0621a643f3d103d112dfcbddee2992f5035 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Mon, 7 Aug 2017 15:36:58 +0300 Subject: [PATCH 02/18] quorum: Set sectors-count to 0 when reporting a flush error The QUORUM_REPORT_BAD event has fields to report the sector in which the error was detected and the number of affected sectors starting from that one. This is important for read and write errors, but not for flush errors. For flush errors the current code reports the total size of the disk image. That is however not useful information in this case. Moreover, the bdrv_getlength() call can fail, and there's no good way of handling that failure. Since we're reporting useless information and we cannot even guarantee to do it in a consistent way, this patch changes the code to report 0 instead in all cases. Reported-by: Markus Armbruster Signed-off-by: Alberto Garcia Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/quorum.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/block/quorum.c b/block/quorum.c index 55ba916655..d04da4f430 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -785,8 +785,7 @@ static coroutine_fn int quorum_co_flush(BlockDriverState *bs) for (i = 0; i < s->num_children; i++) { result = bdrv_co_flush(s->children[i]->bs); if (result) { - quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0, - bdrv_getlength(s->children[i]->bs), + quorum_report_bad(QUORUM_OP_TYPE_FLUSH, 0, 0, s->children[i]->bs->node_name, result); result_value.l = result; quorum_count_vote(&error_votes, &result_value, i); From 3f910692c287e1c611c00e763ebeb95ed0e017f8 Mon Sep 17 00:00:00 2001 From: Jeff Cody Date: Mon, 7 Aug 2017 08:38:19 -0400 Subject: [PATCH 03/18] block/vhdx: check error return of bdrv_getlength() Calls to bdrv_getlength() were not checking for error. In vhdx.c, this can lead to truncating an image file, so it is a definite bug. In vhdx-log.c, the path for improper behavior is less clear, but it is best to check in any case. Some minor code movement of the log_guid intialization, as well. Reported-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Jeff Cody Signed-off-by: Kevin Wolf --- block/vhdx-log.c | 23 ++++++++++++++++++----- block/vhdx.c | 9 ++++++++- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/block/vhdx-log.c b/block/vhdx-log.c index 01278f3fc9..2e26fd46a5 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -491,6 +491,7 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, uint32_t cnt, sectors_read; uint64_t new_file_size; void *data = NULL; + int64_t file_length; VHDXLogDescEntries *desc_entries = NULL; VHDXLogEntryHeader hdr_tmp = { 0 }; @@ -510,10 +511,15 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, if (ret < 0) { goto exit; } + file_length = bdrv_getlength(bs->file->bs); + if (file_length < 0) { + ret = file_length; + goto exit; + } /* if the log shows a FlushedFileOffset larger than our current file * size, then that means the file has been truncated / corrupted, and * we must refused to open it / use it */ - if (hdr_tmp.flushed_file_offset > bdrv_getlength(bs->file->bs)) { + if (hdr_tmp.flushed_file_offset > file_length) { ret = -EINVAL; goto exit; } @@ -543,7 +549,7 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, goto exit; } } - if (bdrv_getlength(bs->file->bs) < desc_entries->hdr.last_file_offset) { + if (file_length < desc_entries->hdr.last_file_offset) { new_file_size = desc_entries->hdr.last_file_offset; if (new_file_size % (1024*1024)) { /* round up to nearest 1MB boundary */ @@ -851,6 +857,7 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s, uint32_t partial_sectors = 0; uint32_t bytes_written = 0; uint64_t file_offset; + int64_t file_length; VHDXHeader *header; VHDXLogEntryHeader new_hdr; VHDXLogDescriptor *new_desc = NULL; @@ -904,6 +911,12 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s, sectors += partial_sectors; + file_length = bdrv_getlength(bs->file->bs); + if (file_length < 0) { + ret = file_length; + goto exit; + } + /* sectors is now how many sectors the data itself takes, not * including the header and descriptor metadata */ @@ -913,11 +926,11 @@ static int vhdx_log_write(BlockDriverState *bs, BDRVVHDXState *s, .sequence_number = s->log.sequence, .descriptor_count = sectors, .reserved = 0, - .flushed_file_offset = bdrv_getlength(bs->file->bs), - .last_file_offset = bdrv_getlength(bs->file->bs), + .flushed_file_offset = file_length, + .last_file_offset = file_length, + .log_guid = header->log_guid, }; - new_hdr.log_guid = header->log_guid; desc_sectors = vhdx_compute_desc_sectors(new_hdr.descriptor_count); diff --git a/block/vhdx.c b/block/vhdx.c index a9cecd2773..37224b8858 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -1166,7 +1166,14 @@ exit: static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s, uint64_t *new_offset) { - *new_offset = bdrv_getlength(bs->file->bs); + int64_t current_len; + + current_len = bdrv_getlength(bs->file->bs); + if (current_len < 0) { + return current_len; + } + + *new_offset = current_len; /* per the spec, the address for a block is in units of 1MB */ *new_offset = ROUND_UP(*new_offset, 1024 * 1024); From 27539ac53154cf9da6990ce9bec1064d37cf2b1d Mon Sep 17 00:00:00 2001 From: Jeff Cody Date: Mon, 7 Aug 2017 08:38:20 -0400 Subject: [PATCH 04/18] block/vhdx: check for offset overflow to bdrv_truncate() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VHDX uses uint64_t types for most offsets, following the VHDX spec. However, bdrv_truncate() takes an int64_t value for the truncating offset. Check for overflow before calling bdrv_truncate(). While we are here, replace the bit shifting with QEMU_ALIGN_UP as well. N.B.: For a compliant image this is not an issue, as the maximum VHDX image size is defined per the spec to be 64TB. Reviewed-by: Eric Blake Reviewed-by: Kevin Wolf Signed-off-by: Jeff Cody Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Kevin Wolf --- block/vhdx-log.c | 6 +++++- block/vhdx.c | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/block/vhdx-log.c b/block/vhdx-log.c index 2e26fd46a5..95972230f0 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -553,7 +553,11 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, new_file_size = desc_entries->hdr.last_file_offset; if (new_file_size % (1024*1024)) { /* round up to nearest 1MB boundary */ - new_file_size = ((new_file_size >> 20) + 1) << 20; + new_file_size = QEMU_ALIGN_UP(new_file_size, MiB); + if (new_file_size > INT64_MAX) { + ret = -EINVAL; + goto exit; + } bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, NULL); } } diff --git a/block/vhdx.c b/block/vhdx.c index 37224b8858..7ae4589879 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -1177,6 +1177,9 @@ static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s, /* per the spec, the address for a block is in units of 1MB */ *new_offset = ROUND_UP(*new_offset, 1024 * 1024); + if (*new_offset > INT64_MAX) { + return -EINVAL; + } return bdrv_truncate(bs->file, *new_offset + s->block_size, PREALLOC_MODE_OFF, NULL); From c6572fa0d2b81bc3a9ca5716f975f2bf59c62e6c Mon Sep 17 00:00:00 2001 From: Jeff Cody Date: Mon, 7 Aug 2017 08:38:21 -0400 Subject: [PATCH 05/18] block/vhdx: check error return of bdrv_flush() Reported-by: Kevin Wolf Signed-off-by: Jeff Cody Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/vhdx-log.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/block/vhdx-log.c b/block/vhdx-log.c index 95972230f0..a27dc059cd 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -565,7 +565,10 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, desc_entries = NULL; } - bdrv_flush(bs); + ret = bdrv_flush(bs); + if (ret < 0) { + goto exit; + } /* once the log is fully flushed, indicate that we have an empty log * now. This also sets the log guid to 0, to indicate an empty log */ vhdx_log_reset(bs, s); @@ -1039,7 +1042,11 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, BDRVVHDXState *s, /* Make sure data written (new and/or changed blocks) is stable * on disk, before creating log entry */ - bdrv_flush(bs); + ret = bdrv_flush(bs); + if (ret < 0) { + goto exit; + } + ret = vhdx_log_write(bs, s, data, length, offset); if (ret < 0) { goto exit; @@ -1047,7 +1054,11 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, BDRVVHDXState *s, logs.log = s->log; /* Make sure log is stable on disk */ - bdrv_flush(bs); + ret = bdrv_flush(bs); + if (ret < 0) { + goto exit; + } + ret = vhdx_log_flush(bs, s, &logs); if (ret < 0) { goto exit; From 95d729835f3ceeed977eaf326a7ebb92788dee6d Mon Sep 17 00:00:00 2001 From: Jeff Cody Date: Mon, 7 Aug 2017 08:38:22 -0400 Subject: [PATCH 06/18] block/vhdx: check error return of bdrv_truncate() Signed-off-by: Jeff Cody Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/vhdx-log.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/block/vhdx-log.c b/block/vhdx-log.c index a27dc059cd..14b724ef7b 100644 --- a/block/vhdx-log.c +++ b/block/vhdx-log.c @@ -558,7 +558,11 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, ret = -EINVAL; goto exit; } - bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, NULL); + ret = bdrv_truncate(bs->file, new_file_size, PREALLOC_MODE_OFF, + NULL); + if (ret < 0) { + goto exit; + } } } qemu_vfree(desc_entries); From 82346685b8cc0537819a3cf7fe2303b55b5f825f Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Fri, 4 Aug 2017 17:26:55 +0200 Subject: [PATCH 07/18] block: drop bdrv_set_key from BlockDriver This is not used anymore since c01c214b69 ("block: remove all encryption handling APIs", 2017-07-11). Signed-off-by: Paolo Bonzini Reviewed-by: Stefan Hajnoczi Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- include/block/block_int.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/block/block_int.h b/include/block/block_int.h index d4f4ea7584..7571c0aaaf 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -127,7 +127,6 @@ struct BlockDriver { Error **errp); void (*bdrv_close)(BlockDriverState *bs); int (*bdrv_create)(const char *filename, QemuOpts *opts, Error **errp); - int (*bdrv_set_key)(BlockDriverState *bs, const char *key); int (*bdrv_make_empty)(BlockDriverState *bs); void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options); From 809eb70ed6cfbfb6c198a25aed036849cc11944d Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 4 Aug 2017 12:44:22 +0200 Subject: [PATCH 08/18] block/null: Remove 'filename' option This option was only added to allow 'null-co://' and 'null-aio://' as filenames, its value never served any actual purpose and was ignored. Nevertheless it was accepted as '-drive driver=null,filename=foo'. The correct way to enable the protocol prefixes (and that without adding a useless -drive option) is implementing .bdrv_parse_filename. This is what this patch does. Technically, this is an incompatible change, but the null block driver is only used for benchmarking, testing and debugging, and an option without effect isn't likely to be used by anyone anyway, so no bad effects are to be expected. Reported-by: Markus Armbruster Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Jeff Cody Reviewed-by: Stefan Hajnoczi --- block/null.c | 31 ++++++++++++++++++++++++++----- tests/qemu-iotests/136 | 2 +- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/block/null.c b/block/null.c index 876f90965b..dd9c13f9ba 100644 --- a/block/null.c +++ b/block/null.c @@ -29,11 +29,6 @@ static QemuOptsList runtime_opts = { .name = "null", .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), .desc = { - { - .name = "filename", - .type = QEMU_OPT_STRING, - .help = "", - }, { .name = BLOCK_OPT_SIZE, .type = QEMU_OPT_SIZE, @@ -54,6 +49,30 @@ static QemuOptsList runtime_opts = { }, }; +static void null_co_parse_filename(const char *filename, QDict *options, + Error **errp) +{ + /* This functions only exists so that a null-co:// filename is accepted + * with the null-co driver. */ + if (strcmp(filename, "null-co://")) { + error_setg(errp, "The only allowed filename for this driver is " + "'null-co://'"); + return; + } +} + +static void null_aio_parse_filename(const char *filename, QDict *options, + Error **errp) +{ + /* This functions only exists so that a null-aio:// filename is accepted + * with the null-aio driver. */ + if (strcmp(filename, "null-aio://")) { + error_setg(errp, "The only allowed filename for this driver is " + "'null-aio://'"); + return; + } +} + static int null_file_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { @@ -242,6 +261,7 @@ static BlockDriver bdrv_null_co = { .instance_size = sizeof(BDRVNullState), .bdrv_file_open = null_file_open, + .bdrv_parse_filename = null_co_parse_filename, .bdrv_close = null_close, .bdrv_getlength = null_getlength, @@ -261,6 +281,7 @@ static BlockDriver bdrv_null_aio = { .instance_size = sizeof(BDRVNullState), .bdrv_file_open = null_file_open, + .bdrv_parse_filename = null_aio_parse_filename, .bdrv_close = null_close, .bdrv_getlength = null_getlength, diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136 index 635b977552..4b994897af 100644 --- a/tests/qemu-iotests/136 +++ b/tests/qemu-iotests/136 @@ -75,7 +75,7 @@ sector = "%d" drive_args.append("stats-account-failed=%s" % (self.account_failed and "on" or "off")) self.create_blkdebug_file() - self.vm = iotests.VM().add_drive('blkdebug:%s:%s ' % + self.vm = iotests.VM().add_drive('blkdebug:%s:%s' % (blkdebug_file, self.test_img), ','.join(drive_args)) self.vm.launch() From 0e51b9b7c7b24127e9996259b611db3177ef6444 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 4 Aug 2017 22:09:42 +0800 Subject: [PATCH 09/18] vmdk: Fix error handling/reporting of vmdk_check Errors from the callees must be captured and propagated to our caller, ensure this for both find_extent() and bdrv_getlength(). Reported-by: Markus Armbruster Signed-off-by: Fam Zheng Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/vmdk.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index 0fc97391a6..c665bcc977 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -2236,6 +2236,7 @@ static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result, fprintf(stderr, "ERROR: could not find extent for sector %" PRId64 "\n", sector_num); + ret = -EINVAL; break; } ret = get_cluster_offset(bs, extent, NULL, @@ -2247,19 +2248,28 @@ static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result, PRId64 "\n", sector_num); break; } - if (ret == VMDK_OK && - cluster_offset >= bdrv_getlength(extent->file->bs)) - { - fprintf(stderr, - "ERROR: cluster offset for sector %" - PRId64 " points after EOF\n", sector_num); - break; + if (ret == VMDK_OK) { + int64_t extent_len = bdrv_getlength(extent->file->bs); + if (extent_len < 0) { + fprintf(stderr, + "ERROR: could not get extent file length for sector %" + PRId64 "\n", sector_num); + ret = extent_len; + break; + } + if (cluster_offset >= extent_len) { + fprintf(stderr, + "ERROR: cluster offset for sector %" + PRId64 " points after EOF\n", sector_num); + ret = -EINVAL; + break; + } } sector_num += extent->cluster_sectors; } result->corruptions++; - return 0; + return ret; } static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs) From 70d9110b440e5252f637d54b5efd33bbc6237c1f Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Fri, 4 Aug 2017 18:10:11 +0300 Subject: [PATCH 10/18] block: respect error code from bdrv_getlength in handle_aiocb_write_zeroes Original idea beyond the code in question was the following: we have failed to write zeroes with fallocate(FALLOC_FL_ZERO_RANGE) as the simplest approach and via fallocate(FALLOC_FL_PUNCH_HOLE)/fallocate(0). We have the only chance now: if the request comes beyond end of the file. Thus we should calculate file length and respect the error code from that op. Signed-off-by: Denis V. Lunev CC: Markus Armbruster CC: Kevin Wolf CC: Max Reitz CC: Stefan Hajnoczi Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- block/file-posix.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/block/file-posix.c b/block/file-posix.c index cfbb236f6f..f4de022ae0 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1339,6 +1339,9 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) #if defined(CONFIG_FALLOCATE) || defined(CONFIG_XFS) BDRVRawState *s = aiocb->bs->opaque; #endif +#ifdef CONFIG_FALLOCATE + int64_t len; +#endif if (aiocb->aio_type & QEMU_AIO_BLKDEV) { return handle_aiocb_write_zeroes_block(aiocb); @@ -1381,7 +1384,10 @@ static ssize_t handle_aiocb_write_zeroes(RawPosixAIOData *aiocb) #endif #ifdef CONFIG_FALLOCATE - if (s->has_fallocate && aiocb->aio_offset >= bdrv_getlength(aiocb->bs)) { + /* Last resort: we are trying to extend the file with zeroed data. This + * can be done via fallocate(fd, 0) */ + len = bdrv_getlength(aiocb->bs); + if (s->has_fallocate && len >= 0 && aiocb->aio_offset >= len) { int ret = do_fallocate(s->fd, 0, aiocb->aio_offset, aiocb->aio_nbytes); if (ret == 0 || ret != -ENOTSUP) { return ret; From d8b83e37c381cf86ecd907301b7dcc65baaa0aea Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Fri, 4 Aug 2017 18:10:12 +0300 Subject: [PATCH 11/18] parallels: respect error code of bdrv_getlength() in allocate_clusters() If we can not get the file length, the state of BDS is broken completely. Return error to the caller. Signed-off-by: Denis V. Lunev CC: Markus Armbruster CC: Kevin Wolf CC: Max Reitz CC: Stefan Hajnoczi Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- block/parallels.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index 5bbdfabb7a..6794e53c0b 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -192,7 +192,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum) { BDRVParallelsState *s = bs->opaque; - int64_t pos, space, idx, to_allocate, i; + int64_t pos, space, idx, to_allocate, i, len; pos = block_status(s, sector_num, nb_sectors, pnum); if (pos > 0) { @@ -214,7 +214,11 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num, assert(idx < s->bat_size && idx + to_allocate <= s->bat_size); space = to_allocate * s->tracks; - if (s->data_end + space > bdrv_getlength(bs->file->bs) >> BDRV_SECTOR_BITS) { + len = bdrv_getlength(bs->file->bs); + if (len < 0) { + return len; + } + if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) { int ret; space += s->prealloc_size; if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) { From e5e6268348972aaf415d7931bbd808b3fdba6cb1 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Fri, 4 Aug 2017 18:10:13 +0300 Subject: [PATCH 12/18] parallels: drop check that bdrv_truncate() is working This would be actually strange and error prone. If truncate() nowadays will fail, there is something fatally wrong. Let's check for that during the actual work. The only fallback case is when the file is not zero initialized. In this case we should switch to preallocation via fallocate(). Signed-off-by: Denis V. Lunev CC: Markus Armbruster CC: Kevin Wolf CC: Max Reitz CC: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- block/parallels.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/block/parallels.c b/block/parallels.c index 6794e53c0b..e1e06d23cc 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -703,9 +703,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, goto fail_options; } - if (!(flags & BDRV_O_RESIZE) || !bdrv_has_zero_init(bs->file->bs) || - bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs), - PREALLOC_MODE_OFF, NULL) != 0) { + if (!bdrv_has_zero_init(bs->file->bs)) { s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE; } From 8aecf1d1bd250a7346165de154f5ccc150ad1aa7 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 3 Aug 2017 17:02:57 +0200 Subject: [PATCH 13/18] block: Fix order in bdrv_replace_child() Commit 8ee03995 refactored the code incorrectly and broke the release of permissions on the old BDS. Instead of changing the permissions to the new required values after removing the old BDS from the list of children, it only re-obtains the permissions it already had. Change the order of operations so that the old BDS is removed again before calculating the new required permissions. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Jeff Cody Reviewed-by: John Snow --- block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block.c b/block.c index ce9cce7b3c..ab908cdc50 100644 --- a/block.c +++ b/block.c @@ -1933,6 +1933,8 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs) BlockDriverState *old_bs = child->bs; uint64_t perm, shared_perm; + bdrv_replace_child_noperm(child, new_bs); + if (old_bs) { /* Update permissions for old node. This is guaranteed to succeed * because we're just taking a parent away, so we're loosening @@ -1942,8 +1944,6 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs) bdrv_set_perm(old_bs, perm, shared_perm); } - bdrv_replace_child_noperm(child, new_bs); - if (new_bs) { bdrv_get_cumulative_perm(new_bs, &perm, &shared_perm); bdrv_set_perm(new_bs, perm, shared_perm); From 54a32bfec140e03ebb19863db944b1c81e4df25e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 3 Aug 2017 17:02:58 +0200 Subject: [PATCH 14/18] block: Allow reopen rw without BDRV_O_ALLOW_RDWR BDRV_O_ALLOW_RDWR is a flag that tells whether qemu can internally reopen a node read-write temporarily because the user requested read-write for the top-level image, but qemu decided that read-only is enough for this node (a backing file). bdrv_reopen() is different, it is also used for cases where the user changed their mind and wants to update the options. There is no reason to forbid making a node read-write in that case. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Jeff Cody Reviewed-by: John Snow --- block.c | 11 +++++++---- include/block/block.h | 3 ++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/block.c b/block.c index ab908cdc50..2711c3dd3b 100644 --- a/block.c +++ b/block.c @@ -246,7 +246,8 @@ bool bdrv_is_writable(BlockDriverState *bs) return !bdrv_is_read_only(bs) && !(bs->open_flags & BDRV_O_INACTIVE); } -int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp) +int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, + bool ignore_allow_rdw, Error **errp) { /* Do not set read_only if copy_on_read is enabled */ if (bs->copy_on_read && read_only) { @@ -256,7 +257,9 @@ int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp) } /* Do not clear read_only if it is prohibited */ - if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) { + if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR) && + !ignore_allow_rdw) + { error_setg(errp, "Node '%s' is read only", bdrv_get_device_or_node_name(bs)); return -EPERM; @@ -269,7 +272,7 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp) { int ret = 0; - ret = bdrv_can_set_read_only(bs, read_only, errp); + ret = bdrv_can_set_read_only(bs, read_only, false, errp); if (ret < 0) { return ret; } @@ -2907,7 +2910,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, * to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is * not set, or if the BDS still has copy_on_read enabled */ read_only = !(reopen_state->flags & BDRV_O_RDWR); - ret = bdrv_can_set_read_only(reopen_state->bs, read_only, &local_err); + ret = bdrv_can_set_read_only(reopen_state->bs, read_only, true, &local_err); if (local_err) { error_propagate(errp, local_err); goto error; diff --git a/include/block/block.h b/include/block/block.h index 34770bb33a..ab80195378 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -436,7 +436,8 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, bool bdrv_is_read_only(BlockDriverState *bs); bool bdrv_is_writable(BlockDriverState *bs); -int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, Error **errp); +int bdrv_can_set_read_only(BlockDriverState *bs, bool read_only, + bool ignore_allow_rdw, Error **errp); int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp); bool bdrv_is_sg(BlockDriverState *bs); bool bdrv_is_inserted(BlockDriverState *bs); From fd4520212bd4fc9c86db4ccd4d9c3d612e8e58f9 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 3 Aug 2017 17:02:59 +0200 Subject: [PATCH 15/18] block: Set BDRV_O_ALLOW_RDWR during rw reopen Reopening an image should be consistent with opening it, so we should set BDRV_O_ALLOW_RDWR for any image that is reopened read-write like in bdrv_open_inherit(). Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Jeff Cody Reviewed-by: John Snow --- block.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index 2711c3dd3b..3615a6809e 100644 --- a/block.c +++ b/block.c @@ -2729,8 +2729,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, bdrv_join_options(bs, options, old_options); QDECREF(old_options); - /* bdrv_open() masks this flag out */ + /* bdrv_open_inherit() sets and clears some additional flags internally */ flags &= ~BDRV_O_PROTOCOL; + if (flags & BDRV_O_RDWR) { + flags |= BDRV_O_ALLOW_RDWR; + } QLIST_FOREACH(child, &bs->children, next) { QDict *new_child_options; From ea92203c661b59a63430b8b1cf2ce43a8c67c65e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 3 Aug 2017 17:03:00 +0200 Subject: [PATCH 16/18] qemu-io: Allow reopen read-write This allows qemu-iotests to test the switch between read-only and read-write mode for block devices. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Jeff Cody Reviewed-by: John Snow --- qemu-io-cmds.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 3eb42c6728..2811a89099 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -1920,6 +1920,7 @@ static void reopen_help(void) " 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a qcow2 image\n" "\n" " -r, -- Reopen the image read-only\n" +" -w, -- Reopen the image read-write\n" " -c, -- Change the cache mode to the given value\n" " -o, -- Changes block driver options (cf. 'open' command)\n" "\n"); @@ -1942,7 +1943,7 @@ static const cmdinfo_t reopen_cmd = { .argmin = 0, .argmax = -1, .cfunc = reopen_f, - .args = "[-r] [-c cache] [-o options]", + .args = "[(-r|-w)] [-c cache] [-o options]", .oneline = "reopens an image with new options", .help = reopen_help, }; @@ -1955,11 +1956,12 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) int c; int flags = bs->open_flags; bool writethrough = !blk_enable_write_cache(blk); + bool has_rw_option = false; BlockReopenQueue *brq; Error *local_err = NULL; - while ((c = getopt(argc, argv, "c:o:r")) != -1) { + while ((c = getopt(argc, argv, "c:o:rw")) != -1) { switch (c) { case 'c': if (bdrv_parse_cache_mode(optarg, &flags, &writethrough) < 0) { @@ -1974,7 +1976,20 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) } break; case 'r': + if (has_rw_option) { + error_report("Only one -r/-w option may be given"); + return 0; + } flags &= ~BDRV_O_RDWR; + has_rw_option = true; + break; + case 'w': + if (has_rw_option) { + error_report("Only one -r/-w option may be given"); + return 0; + } + flags |= BDRV_O_RDWR; + has_rw_option = true; break; default: qemu_opts_reset(&reopen_opts); From ea22b7a2207721695342063175212c5be76d5c69 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 3 Aug 2017 17:03:01 +0200 Subject: [PATCH 17/18] qemu-iotests: Test reopen between read-only and read-write This serves as a regression test for the bugs that were just fixed for bdrv_reopen() between read-only and read-write mode. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Jeff Cody Reviewed-by: John Snow --- tests/qemu-iotests/187 | 69 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/187.out | 18 ++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 88 insertions(+) create mode 100755 tests/qemu-iotests/187 create mode 100644 tests/qemu-iotests/187.out diff --git a/tests/qemu-iotests/187 b/tests/qemu-iotests/187 new file mode 100755 index 0000000000..7bb783363c --- /dev/null +++ b/tests/qemu-iotests/187 @@ -0,0 +1,69 @@ +#!/bin/bash +# +# Test switching between read-only and read-write +# +# Copyright (C) 2017 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +# creator +owner=kwolf@redhat.com + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img + rm -f "$TEST_IMG.2" + rm -f "$TEST_IMG.3" +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt qcow2 +_supported_proto file +_supported_os Linux + +size=64M +_make_test_img $size + +echo +echo "Start from read-only" +echo + +$QEMU_IO -r -c 'write 0 64k' $TEST_IMG | _filter_qemu_io +$QEMU_IO -r -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io +$QEMU_IO -r -c 'reopen -w' -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io + +echo +echo "Start from read-write" +echo + +$QEMU_IO -c 'write 0 64k' $TEST_IMG | _filter_qemu_io +$QEMU_IO -c 'reopen -r' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io +$QEMU_IO -c 'reopen -r' -c 'reopen -w' -c 'write 0 64k' $TEST_IMG | _filter_qemu_io + + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/187.out b/tests/qemu-iotests/187.out new file mode 100644 index 0000000000..68fb944cd5 --- /dev/null +++ b/tests/qemu-iotests/187.out @@ -0,0 +1,18 @@ +QA output created by 187 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 + +Start from read-only + +Block node is read-only +wrote 65536/65536 bytes at offset 0 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Block node is read-only + +Start from read-write + +wrote 65536/65536 bytes at offset 0 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +write failed: Operation not permitted +wrote 65536/65536 bytes at offset 0 +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 823811076d..1848077932 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -182,6 +182,7 @@ 183 rw auto migration 185 rw auto 186 rw auto +187 rw auto 188 rw auto quick 189 rw auto 190 rw auto quick From 113fe792fd4931dd0538f03859278b8719ee4fa2 Mon Sep 17 00:00:00 2001 From: Jeff Cody Date: Mon, 7 Aug 2017 18:29:09 -0400 Subject: [PATCH 18/18] block/nfs: fix mutex assertion in nfs_file_close() Commit c096358e747e88fc7364e40e3c354ee0bb683960 introduced assertion checks for when qemu_mutex() functions are called without the corresponding qemu_mutex_init() having initialized the mutex. This uncovered a latent bug in qemu's nfs driver - in nfs_client_close(), the NFSClient structure is overwritten with zeros, prior to the mutex being destroyed. Go ahead and destroy the mutex in nfs_client_close(), and change where we call qemu_mutex_init() so that it is correctly balanced. There are also a couple of memory leaks obscured by the memset, so this fixes those as well. Finally, we should be able to get rid of the memset(), as it isn't necessary. Cc: qemu-stable@nongnu.org Signed-off-by: Jeff Cody Reviewed-by: Peter Lieven Reviewed-by: Stefan Hajnoczi Reviewed-by: John Snow Signed-off-by: Kevin Wolf --- block/nfs.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/block/nfs.c b/block/nfs.c index d8db419957..bec16b72a6 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -433,19 +433,23 @@ static void nfs_client_close(NFSClient *client) if (client->context) { if (client->fh) { nfs_close(client->context, client->fh); + client->fh = NULL; } aio_set_fd_handler(client->aio_context, nfs_get_fd(client->context), false, NULL, NULL, NULL, NULL); nfs_destroy_context(client->context); + client->context = NULL; } - memset(client, 0, sizeof(NFSClient)); + g_free(client->path); + qemu_mutex_destroy(&client->mutex); + qapi_free_NFSServer(client->server); + client->server = NULL; } static void nfs_file_close(BlockDriverState *bs) { NFSClient *client = bs->opaque; nfs_client_close(client); - qemu_mutex_destroy(&client->mutex); } static NFSServer *nfs_config(QDict *options, Error **errp) @@ -498,6 +502,7 @@ static int64_t nfs_client_open(NFSClient *client, QDict *options, struct stat st; char *file = NULL, *strp = NULL; + qemu_mutex_init(&client->mutex); opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); qemu_opts_absorb_qdict(opts, options, &local_err); if (local_err) { @@ -660,7 +665,7 @@ static int nfs_file_open(BlockDriverState *bs, QDict *options, int flags, if (ret < 0) { return ret; } - qemu_mutex_init(&client->mutex); + bs->total_sectors = ret; ret = 0; return ret;