[PATCH v2 0/4] mirror: Make sure that source and target size match

[PATCH v2 0/4] mirror: Make sure that source and target size match

[Kevin Wolf]

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

[PATCH v2 1/4] iotests/109: Don't mirror with mismatched size

[Kevin Wolf]

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

Re: [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size

[Max Reitz]

[-- 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 --]

Re: [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size

[Kevin Wolf]

[-- 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 --]

Re: [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size

[Max Reitz]

[-- 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 --]

Re: [PATCH v2 1/4] iotests/109: Don't mirror with mismatched size

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v2 2/4] iotests/229: Use blkdebug to inject an error

[Kevin Wolf]

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

Re: [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error

[Max Reitz]

[-- 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 --]

Re: [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error

[Kevin Wolf]

[-- 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 --]

Re: [PATCH v2 2/4] iotests/229: Use blkdebug to inject an error

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v2 3/4] mirror: Make sure that source and target size match

[Kevin Wolf]

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

Re: [PATCH v2 3/4] mirror: Make sure that source and target size match

[Max Reitz]

[-- 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 --]

Re: [PATCH v2 3/4] mirror: Make sure that source and target size match

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v2 3/4] mirror: Make sure that source and target size match

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v2 3/4] mirror: Make sure that source and target size match

[Kevin Wolf]

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

Re: [PATCH v2 3/4] mirror: Make sure that source and target size match

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v2 4/4] iotests: Mirror with different source/target size

[Kevin Wolf]

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

Re: [PATCH v2 4/4] iotests: Mirror with different source/target size

[Max Reitz]

[-- 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 --]

Re: [PATCH v2 4/4] iotests: Mirror with different source/target size

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v2 4/4] iotests: Mirror with different source/target size

[Kevin Wolf]

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