* [PATCH v2 0/4] mirror: Make sure that source and target size match @ 2020-05-11 13:58 Kevin Wolf 2020-05-11 13:58 ` [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size Kevin Wolf ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Kevin Wolf @ 2020-05-11 13:58 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, jsnow, qemu-devel, mreitz Same thing as the recent fix for backup, except that mirror already forbids resizing during the job. So what remains is checking that the sizes match at the start of the job. v2: - Added patch 1 to fix a test that used different source/target size Kevin Wolf (4): iotests/109: Don't mirror with mismatched size iotests/229: Use blkdebug to inject an error mirror: Make sure that source and target size match iotests: Mirror with different source/target size block/mirror.c | 21 +++++---- tests/qemu-iotests/041 | 45 +++++++++++++++++++ tests/qemu-iotests/041.out | 4 +- tests/qemu-iotests/109 | 10 ++--- tests/qemu-iotests/109.out | 74 +++++++++++++------------------- tests/qemu-iotests/229 | 15 +++++-- tests/qemu-iotests/229.out | 6 +-- tests/qemu-iotests/common.filter | 5 +++ 8 files changed, 114 insertions(+), 66 deletions(-) -- 2.25.3 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size 2020-05-11 13:58 [PATCH v2 0/4] mirror: Make sure that source and target size match Kevin Wolf @ 2020-05-11 13:58 ` Kevin Wolf 2020-05-11 15:08 ` Max Reitz 2020-05-12 15:00 ` Vladimir Sementsov-Ogievskiy 2020-05-11 13:58 ` [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error Kevin Wolf ` (2 subsequent siblings) 3 siblings, 2 replies; 20+ messages in thread From: Kevin Wolf @ 2020-05-11 13:58 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, jsnow, qemu-devel, mreitz This patch makes the raw image the same size as the file in a different format that is mirrored as raw to it to avoid errors when mirror starts to enforce that source and target are the same size. We check only that the first 512 bytes are zeroed (instead of 64k) because some image formats create image files that are smaller than 64k, so trying to read 64k would result in I/O errors. Apart from this, 512 is more appropriate anyway because the raw format driver protects specifically the first 512 bytes. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/109 | 10 ++--- tests/qemu-iotests/109.out | 74 +++++++++++++------------------- tests/qemu-iotests/common.filter | 5 +++ 3 files changed, 41 insertions(+), 48 deletions(-) diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109 index 5bc2e9b001..3ffeaf3c55 100755 --- a/tests/qemu-iotests/109 +++ b/tests/qemu-iotests/109 @@ -77,14 +77,14 @@ for fmt in qcow qcow2 qed vdi vmdk vpc; do echo "=== Writing a $fmt header into raw ===" echo - _make_test_img 64M TEST_IMG="$TEST_IMG.src" IMGFMT=$fmt _make_test_img 64M + _make_test_img $(du -b "$TEST_IMG.src" | cut -f1) | _filter_img_create_size # This first test should fail: The image format was probed, we may not # write an image header at the start of the image run_qemu "$TEST_IMG" "$TEST_IMG.src" "" "BLOCK_JOB_ERROR" | _filter_block_job_len - $QEMU_IO -c 'read -P 0 0 64k' "$TEST_IMG" | _filter_qemu_io + $QEMU_IO -c 'read -P 0 0 512' "$TEST_IMG" | _filter_qemu_io # When raw was explicitly specified, the same must succeed @@ -103,12 +103,12 @@ for sample_img in empty.bochs iotest-dirtylog-10G-4M.vhdx parallels-v1 \ # Can't use _use_sample_img because that isn't designed to be used multiple # times and it overwrites $TEST_IMG (both breaks cleanup) - _make_test_img 64M bzcat "$SAMPLE_IMG_DIR/$sample_img.bz2" > "$TEST_IMG.src" + _make_test_img $(du -b "$TEST_IMG.src" | cut -f1) | _filter_img_create_size run_qemu "$TEST_IMG" "$TEST_IMG.src" "" "BLOCK_JOB_ERROR" | _filter_block_job_offset | _filter_block_job_len - $QEMU_IO -c 'read -P 0 0 64k' "$TEST_IMG" | _filter_qemu_io + $QEMU_IO -c 'read -P 0 0 512' "$TEST_IMG" | _filter_qemu_io run_qemu "$TEST_IMG" "$TEST_IMG.src" "'format': 'raw'," "BLOCK_JOB_READY" $QEMU_IMG compare -f raw -F raw "$TEST_IMG" "$TEST_IMG.src" @@ -119,8 +119,8 @@ echo "=== Write legitimate MBR into raw ===" echo for sample_img in grub_mbr.raw; do - _make_test_img 64M bzcat "$SAMPLE_IMG_DIR/$sample_img.bz2" > "$TEST_IMG.src" + _make_test_img $(du -b "$TEST_IMG.src" | cut -f1) | _filter_img_create_size run_qemu "$TEST_IMG" "$TEST_IMG.src" "" "BLOCK_JOB_READY" $QEMU_IMG compare -f raw -F raw "$TEST_IMG" "$TEST_IMG.src" diff --git a/tests/qemu-iotests/109.out b/tests/qemu-iotests/109.out index 884f65f18d..ad739df46c 100644 --- a/tests/qemu-iotests/109.out +++ b/tests/qemu-iotests/109.out @@ -2,8 +2,8 @@ QA output created by 109 === Writing a qcow header into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -23,8 +23,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -43,13 +43,12 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Writing a qcow2 header into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -69,8 +68,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -89,13 +88,12 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 197120, "offset": 197120, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Writing a qed header into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -115,8 +113,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -135,13 +133,12 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Writing a vdi header into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -161,8 +158,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -181,13 +178,12 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 1024, "offset": 1024, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Writing a vmdk header into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -207,8 +203,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -227,13 +223,12 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 65536, "offset": 65536, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Writing a vpc header into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 Formatting 'TEST_DIR/t.raw.src', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -253,8 +248,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -273,12 +268,11 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Copying sample image empty.bochs into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -298,8 +292,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -318,12 +312,11 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2560, "offset": 2560, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Copying sample image iotest-dirtylog-10G-4M.vhdx into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -343,8 +336,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -363,12 +356,11 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 31457280, "offset": 31457280, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Copying sample image parallels-v1 into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -388,8 +380,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -408,12 +400,11 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 327680, "offset": 327680, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Copying sample image simple-pattern.cloop into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -433,8 +424,8 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"execute":"quit"} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -read 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'format': 'IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -453,12 +444,11 @@ read 65536/65536 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 2048, "offset": 2048, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. === Write legitimate MBR into raw === -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=SIZE { 'execute': 'qmp_capabilities' } {"return": {}} {'execute':'drive-mirror', 'arguments':{ 'device': 'src', 'target': 'TEST_DIR/t.IMGFMT', 'mode': 'existing', 'sync': 'full'}} @@ -480,7 +470,6 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. { 'execute': 'qmp_capabilities' } {"return": {}} @@ -500,6 +489,5 @@ Images are identical. {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "src", "len": 512, "offset": 512, "speed": 0, "type": "mirror"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "src"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "src"}} -Warning: Image size mismatch! Images are identical. *** done diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter index 3f8ee3e5f7..e4dcf5e6e7 100644 --- a/tests/qemu-iotests/common.filter +++ b/tests/qemu-iotests/common.filter @@ -155,6 +155,11 @@ _filter_img_create() -e "s# force_size=\\(on\\|off\\)##g" } +_filter_img_create_size() +{ + $SED -e "s# size=[0-9]\\+# size=SIZE#g" +} + _filter_img_info() { if [[ "$1" == "--format-specific" ]]; then -- 2.25.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size 2020-05-11 13:58 ` [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size Kevin Wolf @ 2020-05-11 15:08 ` Max Reitz 2020-05-11 15:29 ` Kevin Wolf 2020-05-12 15:00 ` Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 20+ messages in thread From: Max Reitz @ 2020-05-11 15:08 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: vsementsov, jsnow, qemu-devel [-- Attachment #1.1: Type: text/plain, Size: 1400 bytes --] On 11.05.20 15:58, Kevin Wolf wrote: > This patch makes the raw image the same size as the file in a different > format that is mirrored as raw to it to avoid errors when mirror starts > to enforce that source and target are the same size. > > We check only that the first 512 bytes are zeroed (instead of 64k) > because some image formats create image files that are smaller than 64k, > so trying to read 64k would result in I/O errors. Apart from this, 512 > is more appropriate anyway because the raw format driver protects > specifically the first 512 bytes. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > tests/qemu-iotests/109 | 10 ++--- > tests/qemu-iotests/109.out | 74 +++++++++++++------------------- > tests/qemu-iotests/common.filter | 5 +++ > 3 files changed, 41 insertions(+), 48 deletions(-) > > diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109 > index 5bc2e9b001..3ffeaf3c55 100755 > --- a/tests/qemu-iotests/109 > +++ b/tests/qemu-iotests/109 > @@ -77,14 +77,14 @@ for fmt in qcow qcow2 qed vdi vmdk vpc; do > echo "=== Writing a $fmt header into raw ===" > echo > > - _make_test_img 64M > TEST_IMG="$TEST_IMG.src" IMGFMT=$fmt _make_test_img 64M > + _make_test_img $(du -b "$TEST_IMG.src" | cut -f1) | _filter_img_create_size Why du and not the file length (stat -c '%s')? Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size 2020-05-11 15:08 ` Max Reitz @ 2020-05-11 15:29 ` Kevin Wolf 2020-05-11 15:37 ` Max Reitz 0 siblings, 1 reply; 20+ messages in thread From: Kevin Wolf @ 2020-05-11 15:29 UTC (permalink / raw) To: Max Reitz; +Cc: vsementsov, jsnow, qemu-devel, qemu-block [-- Attachment #1: Type: text/plain, Size: 1762 bytes --] Am 11.05.2020 um 17:08 hat Max Reitz geschrieben: > On 11.05.20 15:58, Kevin Wolf wrote: > > This patch makes the raw image the same size as the file in a different > > format that is mirrored as raw to it to avoid errors when mirror starts > > to enforce that source and target are the same size. > > > > We check only that the first 512 bytes are zeroed (instead of 64k) > > because some image formats create image files that are smaller than 64k, > > so trying to read 64k would result in I/O errors. Apart from this, 512 > > is more appropriate anyway because the raw format driver protects > > specifically the first 512 bytes. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > tests/qemu-iotests/109 | 10 ++--- > > tests/qemu-iotests/109.out | 74 +++++++++++++------------------- > > tests/qemu-iotests/common.filter | 5 +++ > > 3 files changed, 41 insertions(+), 48 deletions(-) > > > > diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109 > > index 5bc2e9b001..3ffeaf3c55 100755 > > --- a/tests/qemu-iotests/109 > > +++ b/tests/qemu-iotests/109 > > @@ -77,14 +77,14 @@ for fmt in qcow qcow2 qed vdi vmdk vpc; do > > echo "=== Writing a $fmt header into raw ===" > > echo > > > > - _make_test_img 64M > > TEST_IMG="$TEST_IMG.src" IMGFMT=$fmt _make_test_img 64M > > + _make_test_img $(du -b "$TEST_IMG.src" | cut -f1) | _filter_img_create_size > > Why du and not the file length (stat -c '%s')? Because the test from which I copied had 'du' and the internet claimed that 'stat -c' isn't portable. Now I see that we do use it in other test cases, so I guess it would have been fine, too. Is there a good reason why 'stat' would be better? Kevin [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size 2020-05-11 15:29 ` Kevin Wolf @ 2020-05-11 15:37 ` Max Reitz 0 siblings, 0 replies; 20+ messages in thread From: Max Reitz @ 2020-05-11 15:37 UTC (permalink / raw) To: Kevin Wolf; +Cc: vsementsov, jsnow, qemu-devel, qemu-block [-- Attachment #1.1: Type: text/plain, Size: 2077 bytes --] On 11.05.20 17:29, Kevin Wolf wrote: > Am 11.05.2020 um 17:08 hat Max Reitz geschrieben: >> On 11.05.20 15:58, Kevin Wolf wrote: >>> This patch makes the raw image the same size as the file in a different >>> format that is mirrored as raw to it to avoid errors when mirror starts >>> to enforce that source and target are the same size. >>> >>> We check only that the first 512 bytes are zeroed (instead of 64k) >>> because some image formats create image files that are smaller than 64k, >>> so trying to read 64k would result in I/O errors. Apart from this, 512 >>> is more appropriate anyway because the raw format driver protects >>> specifically the first 512 bytes. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> tests/qemu-iotests/109 | 10 ++--- >>> tests/qemu-iotests/109.out | 74 +++++++++++++------------------- >>> tests/qemu-iotests/common.filter | 5 +++ >>> 3 files changed, 41 insertions(+), 48 deletions(-) >>> >>> diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109 >>> index 5bc2e9b001..3ffeaf3c55 100755 >>> --- a/tests/qemu-iotests/109 >>> +++ b/tests/qemu-iotests/109 >>> @@ -77,14 +77,14 @@ for fmt in qcow qcow2 qed vdi vmdk vpc; do >>> echo "=== Writing a $fmt header into raw ===" >>> echo >>> >>> - _make_test_img 64M >>> TEST_IMG="$TEST_IMG.src" IMGFMT=$fmt _make_test_img 64M >>> + _make_test_img $(du -b "$TEST_IMG.src" | cut -f1) | _filter_img_create_size >> >> Why du and not the file length (stat -c '%s')? > > Because the test from which I copied had 'du' and the internet claimed > that 'stat -c' isn't portable. Now I see that we do use it in other test > cases, so I guess it would have been fine, too. Is there a good reason > why 'stat' would be better? Oh, I didn’t know that du -b reports the file length. Well, then that works, too. (I’ve never seen du used for anything but getting the, well, disk usage.) (I figured -b would just report the size in bytes.) Then: Reviewed-by: Max Reitz <mreitz@redhat.com> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size 2020-05-11 13:58 ` [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size Kevin Wolf 2020-05-11 15:08 ` Max Reitz @ 2020-05-12 15:00 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 20+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-05-12 15:00 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: jsnow, qemu-devel, mreitz 11.05.2020 16:58, Kevin Wolf wrote: > This patch makes the raw image the same size as the file in a different > format that is mirrored as raw to it to avoid errors when mirror starts > to enforce that source and target are the same size. > > We check only that the first 512 bytes are zeroed (instead of 64k) > because some image formats create image files that are smaller than 64k, > so trying to read 64k would result in I/O errors. Apart from this, 512 > is more appropriate anyway because the raw format driver protects > specifically the first 512 bytes. > > Signed-off-by: Kevin Wolf<kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error 2020-05-11 13:58 [PATCH v2 0/4] mirror: Make sure that source and target size match Kevin Wolf 2020-05-11 13:58 ` [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size Kevin Wolf @ 2020-05-11 13:58 ` Kevin Wolf 2020-05-11 15:18 ` Max Reitz 2020-05-12 15:54 ` Vladimir Sementsov-Ogievskiy 2020-05-11 13:58 ` [PATCH v2 3/4] mirror: Make sure that source and target size match Kevin Wolf 2020-05-11 13:58 ` [PATCH v2 4/4] iotests: Mirror with different source/target size Kevin Wolf 3 siblings, 2 replies; 20+ messages in thread From: Kevin Wolf @ 2020-05-11 13:58 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, jsnow, qemu-devel, mreitz 229 relies on the mirror running into an I/O error when the target is smaller than the source. After changing mirror to catch this condition while starting the job, this test case won't get a job that is paused for an I/O error any more. Use blkdebug instead to inject an error. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200507145228.323412-2-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/229 | 15 +++++++++++---- tests/qemu-iotests/229.out | 6 +++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/tests/qemu-iotests/229 b/tests/qemu-iotests/229 index 866168b236..a5d4e5d4f2 100755 --- a/tests/qemu-iotests/229 +++ b/tests/qemu-iotests/229 @@ -33,6 +33,7 @@ _cleanup() _cleanup_test_img _rm_test_img "$TEST_IMG" _rm_test_img "$DEST_IMG" + rm -f "$TEST_DIR/blkdebug.conf" } trap "_cleanup; exit \$status" 0 1 2 3 15 @@ -49,11 +50,10 @@ _supported_os Linux DEST_IMG="$TEST_DIR/d.$IMGFMT" TEST_IMG="$TEST_DIR/b.$IMGFMT" +BLKDEBUG_CONF="$TEST_DIR/blkdebug.conf" _make_test_img 2M - -# destination for mirror will be too small, causing error -TEST_IMG=$DEST_IMG _make_test_img 1M +TEST_IMG=$DEST_IMG _make_test_img 2M $QEMU_IO -c 'write 0 2M' "$TEST_IMG" | _filter_qemu_io @@ -67,11 +67,18 @@ echo echo '=== Starting drive-mirror, causing error & stop ===' echo +cat > "$BLKDEBUG_CONF" <<EOF +[inject-error] +event = "write_aio" +errno = "5" +once = "on" +EOF + _send_qemu_cmd $QEMU_HANDLE \ "{'execute': 'drive-mirror', 'arguments': {'device': 'testdisk', 'format': '$IMGFMT', - 'target': '$DEST_IMG', + 'target': 'blkdebug:$BLKDEBUG_CONF:$DEST_IMG', 'sync': 'full', 'mode': 'existing', 'on-source-error': 'stop', diff --git a/tests/qemu-iotests/229.out b/tests/qemu-iotests/229.out index 22350d75d7..fdcd249642 100644 --- a/tests/qemu-iotests/229.out +++ b/tests/qemu-iotests/229.out @@ -1,6 +1,6 @@ QA output created by 229 Formatting 'TEST_DIR/b.IMGFMT', fmt=IMGFMT size=2097152 -Formatting 'TEST_DIR/d.IMGFMT', fmt=IMGFMT size=1048576 +Formatting 'TEST_DIR/d.IMGFMT', fmt=IMGFMT size=2097152 wrote 2097152/2097152 bytes at offset 0 2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) {'execute': 'qmp_capabilities'} @@ -8,7 +8,7 @@ wrote 2097152/2097152 bytes at offset 0 === Starting drive-mirror, causing error & stop === -{'execute': 'drive-mirror', 'arguments': {'device': 'testdisk', 'format': 'IMGFMT', 'target': 'TEST_DIR/d.IMGFMT', 'sync': 'full', 'mode': 'existing', 'on-source-error': 'stop', 'on-target-error': 'stop' }} +{'execute': 'drive-mirror', 'arguments': {'device': 'testdisk', 'format': 'IMGFMT', 'target': 'blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/d.IMGFMT', 'sync': 'full', 'mode': 'existing', 'on-source-error': 'stop', 'on-target-error': 'stop' }} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "testdisk"}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "testdisk"}} {"return": {}} @@ -21,5 +21,5 @@ wrote 2097152/2097152 bytes at offset 0 {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "testdisk"}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "testdisk"}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "testdisk", "len": 2097152, "offset": 1048576, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "testdisk", "len": 2097152, "offset": 2097152, "speed": 0, "type": "mirror"}} *** done -- 2.25.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error 2020-05-11 13:58 ` [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error Kevin Wolf @ 2020-05-11 15:18 ` Max Reitz 2020-05-11 15:33 ` Kevin Wolf 2020-05-12 15:54 ` Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 20+ messages in thread From: Max Reitz @ 2020-05-11 15:18 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: vsementsov, jsnow, qemu-devel [-- Attachment #1.1: Type: text/plain, Size: 914 bytes --] On 11.05.20 15:58, Kevin Wolf wrote: > 229 relies on the mirror running into an I/O error when the target is > smaller than the source. After changing mirror to catch this condition > while starting the job, this test case won't get a job that is paused > for an I/O error any more. Use blkdebug instead to inject an error. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Message-Id: <20200507145228.323412-2-kwolf@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Did you do this on purpose, or did you send the patch mail this way accidentally (with Message-ID tag and double S-o-b)? (Because I’ve never seen anyone else do it) > --- > tests/qemu-iotests/229 | 15 +++++++++++---- > tests/qemu-iotests/229.out | 6 +++--- > 2 files changed, 14 insertions(+), 7 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error 2020-05-11 15:18 ` Max Reitz @ 2020-05-11 15:33 ` Kevin Wolf 0 siblings, 0 replies; 20+ messages in thread From: Kevin Wolf @ 2020-05-11 15:33 UTC (permalink / raw) To: Max Reitz; +Cc: vsementsov, jsnow, qemu-devel, qemu-block [-- Attachment #1: Type: text/plain, Size: 1012 bytes --] Am 11.05.2020 um 17:18 hat Max Reitz geschrieben: > On 11.05.20 15:58, Kevin Wolf wrote: > > 229 relies on the mirror running into an I/O error when the target is > > smaller than the source. After changing mirror to catch this condition > > while starting the job, this test case won't get a job that is paused > > for an I/O error any more. Use blkdebug instead to inject an error. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Message-Id: <20200507145228.323412-2-kwolf@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Did you do this on purpose, or did you send the patch mail this way > accidentally (with Message-ID tag and double S-o-b)? It's an artifact on applying my own patch from the list with Message-Id tags (as we're asked to do), rebasing my development branch (which got rid of the original version of the commit message) and then noticing in the last minute that it doesn't pass iotests. Kevin [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error 2020-05-11 13:58 ` [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error Kevin Wolf 2020-05-11 15:18 ` Max Reitz @ 2020-05-12 15:54 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 20+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-05-12 15:54 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: jsnow, qemu-devel, mreitz 11.05.2020 16:58, Kevin Wolf wrote: > 229 relies on the mirror running into an I/O error when the target is > smaller than the source. After changing mirror to catch this condition > while starting the job, this test case won't get a job that is paused > for an I/O error any more. Use blkdebug instead to inject an error. > > Signed-off-by: Kevin Wolf<kwolf@redhat.com> > Message-Id:<20200507145228.323412-2-kwolf@redhat.com> > Reviewed-by: Eric Blake<eblake@redhat.com> > Signed-off-by: Kevin Wolf<kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 3/4] mirror: Make sure that source and target size match 2020-05-11 13:58 [PATCH v2 0/4] mirror: Make sure that source and target size match Kevin Wolf 2020-05-11 13:58 ` [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size Kevin Wolf 2020-05-11 13:58 ` [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error Kevin Wolf @ 2020-05-11 13:58 ` Kevin Wolf 2020-05-11 15:32 ` Max Reitz 2020-05-12 17:15 ` Vladimir Sementsov-Ogievskiy 2020-05-11 13:58 ` [PATCH v2 4/4] iotests: Mirror with different source/target size Kevin Wolf 3 siblings, 2 replies; 20+ messages in thread From: Kevin Wolf @ 2020-05-11 13:58 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, jsnow, qemu-devel, mreitz If the target is shorter than the source, mirror would copy data until it reaches the end of the target and then fail with an I/O error when trying to write past the end. If the target is longer than the source, the mirror job would complete successfully, but the target wouldn't actually be an accurate copy of the source image (it would contain some additional garbage at the end). Fix this by checking that both images have the same size when the job starts. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200507145228.323412-3-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/mirror.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index aca95c9bc9..201ffa26f9 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -872,6 +872,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) BlockDriverState *target_bs = blk_bs(s->target); bool need_drain = true; int64_t length; + int64_t target_length; BlockDriverInfo bdi; char backing_filename[2]; /* we only need 2 characters because we are only checking for a NULL string */ @@ -887,24 +888,26 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) goto immediate_exit; } + target_length = blk_getlength(s->target); + if (target_length < 0) { + ret = target_length; + goto immediate_exit; + } + /* Active commit must resize the base image if its size differs from the * active layer. */ if (s->base == blk_bs(s->target)) { - int64_t base_length; - - base_length = blk_getlength(s->target); - if (base_length < 0) { - ret = base_length; - goto immediate_exit; - } - - if (s->bdev_length > base_length) { + if (s->bdev_length > target_length) { ret = blk_truncate(s->target, s->bdev_length, false, PREALLOC_MODE_OFF, 0, NULL); if (ret < 0) { goto immediate_exit; } } + } else if (s->bdev_length != target_length) { + error_setg(errp, "Source and target image have different sizes"); + ret = -EINVAL; + goto immediate_exit; } if (s->bdev_length == 0) { -- 2.25.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/4] mirror: Make sure that source and target size match 2020-05-11 13:58 ` [PATCH v2 3/4] mirror: Make sure that source and target size match Kevin Wolf @ 2020-05-11 15:32 ` Max Reitz 2020-05-12 17:15 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 20+ messages in thread From: Max Reitz @ 2020-05-11 15:32 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: vsementsov, jsnow, qemu-devel [-- Attachment #1.1: Type: text/plain, Size: 891 bytes --] On 11.05.20 15:58, Kevin Wolf wrote: > If the target is shorter than the source, mirror would copy data until > it reaches the end of the target and then fail with an I/O error when > trying to write past the end. > > If the target is longer than the source, the mirror job would complete > successfully, but the target wouldn't actually be an accurate copy of > the source image (it would contain some additional garbage at the end). > > Fix this by checking that both images have the same size when the job > starts. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Message-Id: <20200507145228.323412-3-kwolf@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/mirror.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/4] mirror: Make sure that source and target size match 2020-05-11 13:58 ` [PATCH v2 3/4] mirror: Make sure that source and target size match Kevin Wolf 2020-05-11 15:32 ` Max Reitz @ 2020-05-12 17:15 ` Vladimir Sementsov-Ogievskiy 2020-05-12 17:16 ` Vladimir Sementsov-Ogievskiy 2020-05-12 18:48 ` Kevin Wolf 1 sibling, 2 replies; 20+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-05-12 17:15 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: jsnow, qemu-devel, mreitz 11.05.2020 16:58, Kevin Wolf wrote: > If the target is shorter than the source, mirror would copy data until > it reaches the end of the target and then fail with an I/O error when > trying to write past the end. > > If the target is longer than the source, the mirror job would complete > successfully, but the target wouldn't actually be an accurate copy of > the source image (it would contain some additional garbage at the end). > > Fix this by checking that both images have the same size when the job > starts. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Message-Id: <20200507145228.323412-3-kwolf@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/mirror.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index aca95c9bc9..201ffa26f9 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -872,6 +872,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) > BlockDriverState *target_bs = blk_bs(s->target); > bool need_drain = true; > int64_t length; > + int64_t target_length; > BlockDriverInfo bdi; > char backing_filename[2]; /* we only need 2 characters because we are only > checking for a NULL string */ > @@ -887,24 +888,26 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) > goto immediate_exit; > } > > + target_length = blk_getlength(s->target); > + if (target_length < 0) { > + ret = target_length; > + goto immediate_exit; > + } > + > /* Active commit must resize the base image if its size differs from the > * active layer. */ > if (s->base == blk_bs(s->target)) { > - int64_t base_length; > - > - base_length = blk_getlength(s->target); > - if (base_length < 0) { > - ret = base_length; > - goto immediate_exit; > - } > - > - if (s->bdev_length > base_length) { > + if (s->bdev_length > target_length) { > ret = blk_truncate(s->target, s->bdev_length, false, > PREALLOC_MODE_OFF, 0, NULL); > if (ret < 0) { > goto immediate_exit; > } > } Hmm, interesting, if base is larger, is our behavior correct? Blockdev becomes larger after commit and old data becomes available? I think we should zero the tail after old EOF or shrink the target.. > + } else if (s->bdev_length != target_length) { > + error_setg(errp, "Source and target image have different sizes"); > + ret = -EINVAL; Seems, the only case, when mirror_run() sets errp. And, therefore, the only correct one.. > + goto immediate_exit; > } > > if (s->bdev_length == 0) { > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/4] mirror: Make sure that source and target size match 2020-05-12 17:15 ` Vladimir Sementsov-Ogievskiy @ 2020-05-12 17:16 ` Vladimir Sementsov-Ogievskiy 2020-05-12 18:48 ` Kevin Wolf 1 sibling, 0 replies; 20+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-05-12 17:16 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: jsnow, qemu-devel, mreitz 12.05.2020 20:15, Vladimir Sementsov-Ogievskiy wrote: >> + } else if (s->bdev_length != target_length) { >> + error_setg(errp, "Source and target image have different sizes"); >> + ret = -EINVAL; > > Seems, the only case, when mirror_run() sets errp. And, therefore, the only correct one.. the only one failure case I mean, of course. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/4] mirror: Make sure that source and target size match 2020-05-12 17:15 ` Vladimir Sementsov-Ogievskiy 2020-05-12 17:16 ` Vladimir Sementsov-Ogievskiy @ 2020-05-12 18:48 ` Kevin Wolf 2020-05-13 10:44 ` Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 20+ messages in thread From: Kevin Wolf @ 2020-05-12 18:48 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy; +Cc: jsnow, qemu-devel, qemu-block, mreitz Am 12.05.2020 um 19:15 hat Vladimir Sementsov-Ogievskiy geschrieben: > 11.05.2020 16:58, Kevin Wolf wrote: > > If the target is shorter than the source, mirror would copy data until > > it reaches the end of the target and then fail with an I/O error when > > trying to write past the end. > > > > If the target is longer than the source, the mirror job would complete > > successfully, but the target wouldn't actually be an accurate copy of > > the source image (it would contain some additional garbage at the end). > > > > Fix this by checking that both images have the same size when the job > > starts. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Message-Id: <20200507145228.323412-3-kwolf@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block/mirror.c | 21 ++++++++++++--------- > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/block/mirror.c b/block/mirror.c > > index aca95c9bc9..201ffa26f9 100644 > > --- a/block/mirror.c > > +++ b/block/mirror.c > > @@ -872,6 +872,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) > > BlockDriverState *target_bs = blk_bs(s->target); > > bool need_drain = true; > > int64_t length; > > + int64_t target_length; > > BlockDriverInfo bdi; > > char backing_filename[2]; /* we only need 2 characters because we are only > > checking for a NULL string */ > > @@ -887,24 +888,26 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) > > goto immediate_exit; > > } > > + target_length = blk_getlength(s->target); > > + if (target_length < 0) { > > + ret = target_length; > > + goto immediate_exit; > > + } > > + > > /* Active commit must resize the base image if its size differs from the > > * active layer. */ > > if (s->base == blk_bs(s->target)) { > > - int64_t base_length; > > - > > - base_length = blk_getlength(s->target); > > - if (base_length < 0) { > > - ret = base_length; > > - goto immediate_exit; > > - } > > - > > - if (s->bdev_length > base_length) { > > + if (s->bdev_length > target_length) { > > ret = blk_truncate(s->target, s->bdev_length, false, > > PREALLOC_MODE_OFF, 0, NULL); > > if (ret < 0) { > > goto immediate_exit; > > } > > } > > Hmm, interesting, if base is larger, is our behavior correct? Blockdev > becomes larger after commit and old data becomes available? I think we > should zero the tail after old EOF or shrink the target.. Yes, I agree, we should shrink it. But active commit is a different case than what I'm fixing in this patch, so I left it unmodified. We'll probably need a third series for commit after backup and mirror. > > + } else if (s->bdev_length != target_length) { > > + error_setg(errp, "Source and target image have different sizes"); > > + ret = -EINVAL; > > Seems, the only case, when mirror_run() sets errp. And, therefore, the > only correct one.. job_update_rc() takes care to fill job->err (with strerror()) if it hasn't been set yet, so the other places aren't strictly wrong, but probably provide suboptimal error messages. Kevin ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/4] mirror: Make sure that source and target size match 2020-05-12 18:48 ` Kevin Wolf @ 2020-05-13 10:44 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 20+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-05-13 10:44 UTC (permalink / raw) To: Kevin Wolf; +Cc: jsnow, qemu-devel, qemu-block, mreitz 12.05.2020 21:48, Kevin Wolf wrote: > Am 12.05.2020 um 19:15 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 11.05.2020 16:58, Kevin Wolf wrote: >>> If the target is shorter than the source, mirror would copy data until >>> it reaches the end of the target and then fail with an I/O error when >>> trying to write past the end. >>> >>> If the target is longer than the source, the mirror job would complete >>> successfully, but the target wouldn't actually be an accurate copy of >>> the source image (it would contain some additional garbage at the end). >>> >>> Fix this by checking that both images have the same size when the job >>> starts. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> Message-Id: <20200507145228.323412-3-kwolf@redhat.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> block/mirror.c | 21 ++++++++++++--------- >>> 1 file changed, 12 insertions(+), 9 deletions(-) >>> >>> diff --git a/block/mirror.c b/block/mirror.c >>> index aca95c9bc9..201ffa26f9 100644 >>> --- a/block/mirror.c >>> +++ b/block/mirror.c >>> @@ -872,6 +872,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) >>> BlockDriverState *target_bs = blk_bs(s->target); >>> bool need_drain = true; >>> int64_t length; >>> + int64_t target_length; >>> BlockDriverInfo bdi; >>> char backing_filename[2]; /* we only need 2 characters because we are only >>> checking for a NULL string */ >>> @@ -887,24 +888,26 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) >>> goto immediate_exit; >>> } >>> + target_length = blk_getlength(s->target); >>> + if (target_length < 0) { >>> + ret = target_length; >>> + goto immediate_exit; >>> + } >>> + >>> /* Active commit must resize the base image if its size differs from the >>> * active layer. */ >>> if (s->base == blk_bs(s->target)) { >>> - int64_t base_length; >>> - >>> - base_length = blk_getlength(s->target); >>> - if (base_length < 0) { >>> - ret = base_length; >>> - goto immediate_exit; >>> - } >>> - >>> - if (s->bdev_length > base_length) { >>> + if (s->bdev_length > target_length) { >>> ret = blk_truncate(s->target, s->bdev_length, false, >>> PREALLOC_MODE_OFF, 0, NULL); >>> if (ret < 0) { >>> goto immediate_exit; >>> } >>> } >> >> Hmm, interesting, if base is larger, is our behavior correct? Blockdev >> becomes larger after commit and old data becomes available? I think we >> should zero the tail after old EOF or shrink the target.. > > Yes, I agree, we should shrink it. But active commit is a different case > than what I'm fixing in this patch, so I left it unmodified. We'll > probably need a third series for commit after backup and mirror. > >>> + } else if (s->bdev_length != target_length) { >>> + error_setg(errp, "Source and target image have different sizes"); >>> + ret = -EINVAL; >> >> Seems, the only case, when mirror_run() sets errp. And, therefore, the >> only correct one.. > > job_update_rc() takes care to fill job->err (with strerror()) if it > hasn't been set yet, so the other places aren't strictly wrong, but > probably provide suboptimal error messages. > Hmm. but I failed to find, where job->err is reported except for job_query_single(), which doesn't call job_update_rc(). block_job_event_completed() doesn't use job->err, but instead create a message using strerror(-job->job.ret). Interesting also, that job_finish_sync may return error, not setting errp.. Still except for tests, it should influence only run_block_job() from qemu-img, which itself doesn't care too much about setting errp on failure, so it's broken anyway. OK, seems this all is not very related to the series: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 4/4] iotests: Mirror with different source/target size 2020-05-11 13:58 [PATCH v2 0/4] mirror: Make sure that source and target size match Kevin Wolf ` (2 preceding siblings ...) 2020-05-11 13:58 ` [PATCH v2 3/4] mirror: Make sure that source and target size match Kevin Wolf @ 2020-05-11 13:58 ` Kevin Wolf 2020-05-11 15:42 ` Max Reitz 2020-05-13 11:17 ` Vladimir Sementsov-Ogievskiy 3 siblings, 2 replies; 20+ messages in thread From: Kevin Wolf @ 2020-05-11 13:58 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, jsnow, qemu-devel, mreitz This tests that the mirror job catches situations where the target node has a different size than the source node. It must also forbid resize operations when the job is already running. Signed-off-by: Kevin Wolf <kwolf@redhat.com> Message-Id: <20200507145228.323412-4-kwolf@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/041 | 45 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/041.out | 4 ++-- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 index 1812dd8479..601c756117 100755 --- a/tests/qemu-iotests/041 +++ b/tests/qemu-iotests/041 @@ -240,6 +240,49 @@ class TestSingleBlockdev(TestSingleDrive): target=self.qmp_target) self.assert_qmp(result, 'error/class', 'GenericError') + def do_test_resize(self, device, node): + def pre_finalize(): + if device: + result = self.vm.qmp('block_resize', device=device, size=65536) + self.assert_qmp(result, 'error/class', 'GenericError') + + result = self.vm.qmp('block_resize', node_name=node, size=65536) + self.assert_qmp(result, 'error/class', 'GenericError') + + result = self.vm.qmp(self.qmp_cmd, job_id='job0', device='drive0', + sync='full', target=self.qmp_target, + auto_finalize=False, auto_dismiss=False) + self.assert_qmp(result, 'return', {}) + + result = self.vm.run_job('job0', auto_finalize=False, + pre_finalize=pre_finalize) + self.assertEqual(result, None) + + def test_source_resize(self): + self.do_test_resize('drive0', 'top') + + def test_target_resize(self): + self.do_test_resize(None, self.qmp_target) + + def do_test_target_size(self, size): + result = self.vm.qmp('block_resize', node_name=self.qmp_target, + size=size) + self.assert_qmp(result, 'return', {}) + + result = self.vm.qmp(self.qmp_cmd, job_id='job0', + device='drive0', sync='full', auto_dismiss=False, + target=self.qmp_target) + self.assert_qmp(result, 'return', {}) + + result = self.vm.run_job('job0') + self.assertEqual(result, 'Source and target image have different sizes') + + def test_small_target(self): + self.do_test_target_size(self.image_len // 2) + + def test_large_target(self): + self.do_test_target_size(self.image_len * 2) + test_large_cluster = None test_image_not_found = None test_small_buffer2 = None @@ -251,6 +294,8 @@ class TestSingleDriveZeroLength(TestSingleDrive): class TestSingleBlockdevZeroLength(TestSingleBlockdev): image_len = 0 + test_small_target = None + test_large_target = None class TestSingleDriveUnalignedLength(TestSingleDrive): image_len = 1025 * 1024 diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out index 877b76fd31..53abe11d73 100644 --- a/tests/qemu-iotests/041.out +++ b/tests/qemu-iotests/041.out @@ -1,5 +1,5 @@ -.............................................................................................. +........................................................................................................ ---------------------------------------------------------------------- -Ran 94 tests +Ran 104 tests OK -- 2.25.3 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/4] iotests: Mirror with different source/target size 2020-05-11 13:58 ` [PATCH v2 4/4] iotests: Mirror with different source/target size Kevin Wolf @ 2020-05-11 15:42 ` Max Reitz 2020-05-13 11:17 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 20+ messages in thread From: Max Reitz @ 2020-05-11 15:42 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: vsementsov, jsnow, qemu-devel [-- Attachment #1.1: Type: text/plain, Size: 663 bytes --] On 11.05.20 15:58, Kevin Wolf wrote: > This tests that the mirror job catches situations where the target node > has a different size than the source node. It must also forbid resize > operations when the job is already running. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Message-Id: <20200507145228.323412-4-kwolf@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > tests/qemu-iotests/041 | 45 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/041.out | 4 ++-- > 2 files changed, 47 insertions(+), 2 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/4] iotests: Mirror with different source/target size 2020-05-11 13:58 ` [PATCH v2 4/4] iotests: Mirror with different source/target size Kevin Wolf 2020-05-11 15:42 ` Max Reitz @ 2020-05-13 11:17 ` Vladimir Sementsov-Ogievskiy 2020-05-13 14:21 ` Kevin Wolf 1 sibling, 1 reply; 20+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2020-05-13 11:17 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: jsnow, qemu-devel, mreitz 11.05.2020 16:58, Kevin Wolf wrote: > This tests that the mirror job catches situations where the target node > has a different size than the source node. It must also forbid resize > operations when the job is already running. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Message-Id: <20200507145228.323412-4-kwolf@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > tests/qemu-iotests/041 | 45 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/041.out | 4 ++-- > 2 files changed, 47 insertions(+), 2 deletions(-) > > diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 > index 1812dd8479..601c756117 100755 > --- a/tests/qemu-iotests/041 > +++ b/tests/qemu-iotests/041 > @@ -240,6 +240,49 @@ class TestSingleBlockdev(TestSingleDrive): Hmm, probably resize tests would be good in the TestSingleDrive, to cover drive-mirror too. Still, there shouldn't be any difference.. So, I don't think it worth doing. > target=self.qmp_target) > self.assert_qmp(result, 'error/class', 'GenericError') > > + def do_test_resize(self, device, node): > + def pre_finalize(): [..] > + def do_test_target_size(self, size): > + result = self.vm.qmp('block_resize', node_name=self.qmp_target, > + size=size) > + self.assert_qmp(result, 'return', {}) > + > + result = self.vm.qmp(self.qmp_cmd, job_id='job0', > + device='drive0', sync='full', auto_dismiss=False, > + target=self.qmp_target) > + self.assert_qmp(result, 'return', {}) > + > + result = self.vm.run_job('job0') > + self.assertEqual(result, 'Source and target image have different sizes') Hmm, and this proves, that we are not very good with handling early errors. A lot better would be to fail earlier, on blockdev-mirror command. And, as shown by your previous series, backup works better in this case. But again, it's for another series. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/4] iotests: Mirror with different source/target size 2020-05-13 11:17 ` Vladimir Sementsov-Ogievskiy @ 2020-05-13 14:21 ` Kevin Wolf 0 siblings, 0 replies; 20+ messages in thread From: Kevin Wolf @ 2020-05-13 14:21 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy; +Cc: jsnow, qemu-devel, qemu-block, mreitz Am 13.05.2020 um 13:17 hat Vladimir Sementsov-Ogievskiy geschrieben: > 11.05.2020 16:58, Kevin Wolf wrote: > > This tests that the mirror job catches situations where the target node > > has a different size than the source node. It must also forbid resize > > operations when the job is already running. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Message-Id: <20200507145228.323412-4-kwolf@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > > --- > > tests/qemu-iotests/041 | 45 ++++++++++++++++++++++++++++++++++++++ > > tests/qemu-iotests/041.out | 4 ++-- > > 2 files changed, 47 insertions(+), 2 deletions(-) > > > > diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041 > > index 1812dd8479..601c756117 100755 > > --- a/tests/qemu-iotests/041 > > +++ b/tests/qemu-iotests/041 > > @@ -240,6 +240,49 @@ class TestSingleBlockdev(TestSingleDrive): > > > Hmm, probably resize tests would be good in the TestSingleDrive, to > cover drive-mirror too. Still, there shouldn't be any difference.. So, > I don't think it worth doing. At first, I tried to do a single implementation that works for both, but it wasn't easily possible. I'm not sure any more if it was the only problem, but at least drive-mirror targets don't have a known node-name, so we would have to guess it from query-named-block-nodes. Not impossible, but maybe not worth it... > > target=self.qmp_target) > > self.assert_qmp(result, 'error/class', 'GenericError') > > + def do_test_resize(self, device, node): > > + def pre_finalize(): > > [..] > > > + def do_test_target_size(self, size): > > + result = self.vm.qmp('block_resize', node_name=self.qmp_target, > > + size=size) > > + self.assert_qmp(result, 'return', {}) > > + > > + result = self.vm.qmp(self.qmp_cmd, job_id='job0', > > + device='drive0', sync='full', auto_dismiss=False, > > + target=self.qmp_target) > > + self.assert_qmp(result, 'return', {}) > > + > > + result = self.vm.run_job('job0') > > + self.assertEqual(result, 'Source and target image have different sizes') > > Hmm, and this proves, that we are not very good with handling early > errors. A lot better would be to fail earlier, on blockdev-mirror > command. And, as shown by your previous series, backup works better in > this case. But again, it's for another series. At least it shows that errp does make it to the client. :-) I'm not sure which way is currently better. QMP handlers are still synchronous code that blocks the rest of QEMU, so doing things only inside the coroutine does have some benefits. Maybe with coroutine QMP handlers this will become less of a concern, though. Kevin ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-05-13 14:22 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-11 13:58 [PATCH v2 0/4] mirror: Make sure that source and target size match Kevin Wolf 2020-05-11 13:58 ` [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size Kevin Wolf 2020-05-11 15:08 ` Max Reitz 2020-05-11 15:29 ` Kevin Wolf 2020-05-11 15:37 ` Max Reitz 2020-05-12 15:00 ` Vladimir Sementsov-Ogievskiy 2020-05-11 13:58 ` [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error Kevin Wolf 2020-05-11 15:18 ` Max Reitz 2020-05-11 15:33 ` Kevin Wolf 2020-05-12 15:54 ` Vladimir Sementsov-Ogievskiy 2020-05-11 13:58 ` [PATCH v2 3/4] mirror: Make sure that source and target size match Kevin Wolf 2020-05-11 15:32 ` Max Reitz 2020-05-12 17:15 ` Vladimir Sementsov-Ogievskiy 2020-05-12 17:16 ` Vladimir Sementsov-Ogievskiy 2020-05-12 18:48 ` Kevin Wolf 2020-05-13 10:44 ` Vladimir Sementsov-Ogievskiy 2020-05-11 13:58 ` [PATCH v2 4/4] iotests: Mirror with different source/target size Kevin Wolf 2020-05-11 15:42 ` Max Reitz 2020-05-13 11:17 ` Vladimir Sementsov-Ogievskiy 2020-05-13 14:21 ` Kevin Wolf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.