[Qemu-devel] [PATCH 0/2] Fix qemu-img map on unaligned image

[Qemu-devel] [PATCH 0/2] Fix qemu-img map on unaligned image

[Eric Blake]

See https://bugzilla.redhat.com/1589738; thanks to Nir, Maor, and Dan
for figuring out that it was a qemu-img regression and coming up with
a test case; and for Kevin for then bisecting it to point to my
byte-based conversion code being at fault.

Eric Blake (2):
  qemu-img: Fix assert when mapping unaligned raw file
  iotests: Add test 221 to catch qemu-img map regression

 qemu-img.c                 |  2 +-
 tests/qemu-iotests/221     | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/221.out | 16 +++++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/221
 create mode 100644 tests/qemu-iotests/221.out

-- 
2.14.4

[Qemu-devel] [PATCH 1/2] qemu-img: Fix assert when mapping unaligned raw file

[Eric Blake]

Commit a290f085 exposed a latent bug in qemu-img map introduced
during the conversion of block status to be byte-based.  Earlier in
commit 5e344dd8, the internal interface get_block_status() switched
to take byte-based parameters, but still called a sector-based
block layer function; as such, rounding was added in the lone
caller to obey the contract.  However, commit 237d78f8 changed
get_block_status() to truly be byte-based, at which point rounding
to sector boundaries can result in calling bdrv_block_status() with
'bytes == 0' (a coding error) when the boundary between data and a
hole falls mid-sector (true for the past-EOF implicit hole present
in POSIX files).  Fix things by removing the rounding that is now
no longer necessary.

See also https://bugzilla.redhat.com/1589738

Fixes: 237d78f8
Reported-by: Dan Kenigsberg <danken@redhat.com>
Reported-by: Nir Soffer <nsoffer@redhat.com>
Reported-by: Maor Lipchuk <mlipchuk@redhat.com>
CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 1dcdd47254a..e1a506f7f67 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2906,7 +2906,7 @@ static int img_map(int argc, char **argv)
         int64_t n;

         /* Probe up to 1 GiB at a time.  */
-        n = QEMU_ALIGN_DOWN(MIN(1 << 30, length - offset), BDRV_SECTOR_SIZE);
+        n = MIN(1 << 30, length - offset);
         ret = get_block_status(bs, offset, n, &next);

         if (ret < 0) {
-- 
2.14.4

[Qemu-devel] [PATCH 2/2] iotests: Add test 221 to catch qemu-img map regression

[Eric Blake]

Although qemu-img creates aligned files (by rounding up), it
must also gracefully handle files that are not sector-aligned.
Test that the bug fixed in the previous patch does not recur.

It's a bit annoying that we can see the (implicit) hole past
the end of the file on to the next sector boundary, so if we
ever reach the point where we report a byte-accurate size rather
than our current behavior of always rounding up, this test will
probably need a slight modification.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/221     | 60 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/221.out | 16 +++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 77 insertions(+)
 create mode 100755 tests/qemu-iotests/221
 create mode 100644 tests/qemu-iotests/221.out

diff --git a/tests/qemu-iotests/221 b/tests/qemu-iotests/221
new file mode 100755
index 00000000000..f2cd3c2210e
--- /dev/null
+++ b/tests/qemu-iotests/221
@@ -0,0 +1,60 @@
+#!/bin/bash
+#
+# Test qemu-img vs. unaligned images
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+status=1 # failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt raw
+_supported_proto file
+_supported_os Linux
+
+echo
+echo "=== Check mapping of unaligned raw image ==="
+echo
+
+_make_test_img 43009 # qemu-img create rounds size up
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+truncate --size=43009 "$TEST_IMG" # so we resize it and check again
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+$QEMU_IO -c 'w 43008 1' "$TEST_IMG" # writing also rounds up
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+truncate --size=43009 "$TEST_IMG" # so we resize it and check again
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/221.out b/tests/qemu-iotests/221.out
new file mode 100644
index 00000000000..fcf61352cc3
--- /dev/null
+++ b/tests/qemu-iotests/221.out
@@ -0,0 +1,16 @@
+QA output created by 221
+
+=== Check mapping of unaligned raw image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=43009
+[{ "start": 0, "length": 43520, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
+[{ "start": 0, "length": 43520, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
+wrote 1/1 bytes at offset 43008
+1 bytes, 1 ops; 0.0001 sec (7.512 KiB/sec and 7692.3077 ops/sec)
+[{ "start": 0, "length": 40960, "depth": 0, "zero": true, "data": false, "offset": OFFSET},
+{ "start": 40960, "length": 2049, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 43009, "length": 511, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
+[{ "start": 0, "length": 40960, "depth": 0, "zero": true, "data": false, "offset": OFFSET},
+{ "start": 40960, "length": 2049, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 43009, "length": 511, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0914c922d7f..937a3d0a4d8 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -218,3 +218,4 @@
 217 rw auto quick
 218 rw auto quick
 219 rw auto
+221 rw auto quick
-- 
2.14.4

Re: [Qemu-devel] [PATCH 2/2] iotests: Add test 221 to catch qemu-img map regression

[Eric Blake]

On 06/11/2018 04:39 PM, Eric Blake wrote:
> Although qemu-img creates aligned files (by rounding up), it
> must also gracefully handle files that are not sector-aligned.
> Test that the bug fixed in the previous patch does not recur.
> 
> It's a bit annoying that we can see the (implicit) hole past
> the end of the file on to the next sector boundary, so if we
> ever reach the point where we report a byte-accurate size rather
> than our current behavior of always rounding up, this test will
> probably need a slight modification.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---

> +
> +$QEMU_IO -c 'w 43008 1' "$TEST_IMG" # writing also rounds up

Shoot - missing a filter...

> +++ b/tests/qemu-iotests/221.out
> @@ -0,0 +1,16 @@
> +QA output created by 221
> +
> +=== Check mapping of unaligned raw image ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=43009
> +[{ "start": 0, "length": 43520, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
> +[{ "start": 0, "length": 43520, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
> +wrote 1/1 bytes at offset 43008
> +1 bytes, 1 ops; 0.0001 sec (7.512 KiB/sec and 7692.3077 ops/sec)

...which leaks volatile output.  Squash this in:

diff --git i/tests/qemu-iotests/221 w/tests/qemu-iotests/221
index f2cd3c2210e..41c4e4bdf88 100755
--- i/tests/qemu-iotests/221
+++ w/tests/qemu-iotests/221
@@ -48,7 +48,7 @@ $QEMU_IMG map --output=json "$TEST_IMG" | 
_filter_qemu_img_map
  truncate --size=43009 "$TEST_IMG" # so we resize it and check again
  $QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map

-$QEMU_IO -c 'w 43008 1' "$TEST_IMG" # writing also rounds up
+$QEMU_IO -c 'w 43008 1' "$TEST_IMG" | _filter_qemu_io # writing also 
rounds up
  $QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map

  truncate --size=43009 "$TEST_IMG" # so we resize it and check again
diff --git i/tests/qemu-iotests/221.out w/tests/qemu-iotests/221.out
index fcf61352cc3..a9c0190aadc 100644
--- i/tests/qemu-iotests/221.out
+++ w/tests/qemu-iotests/221.out
@@ -6,7 +6,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=43009
  [{ "start": 0, "length": 43520, "depth": 0, "zero": true, "data": 
false, "offset": OFFSET}]
  [{ "start": 0, "length": 43520, "depth": 0, "zero": true, "data": 
false, "offset": OFFSET}]
  wrote 1/1 bytes at offset 43008
-1 bytes, 1 ops; 0.0001 sec (7.512 KiB/sec and 7692.3077 ops/sec)
+1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
  [{ "start": 0, "length": 40960, "depth": 0, "zero": true, "data": 
false, "offset": OFFSET},
  { "start": 40960, "length": 2049, "depth": 0, "zero": false, "data": 
true, "offset": OFFSET},
  { "start": 43009, "length": 511, "depth": 0, "zero": true, "data": 
false, "offset": OFFSET}]


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/2] iotests: Add test 221 to catch qemu-img map regression

[Kevin Wolf]

Am 12.06.2018 um 00:03 hat Eric Blake geschrieben:
> On 06/11/2018 04:39 PM, Eric Blake wrote:
> > Although qemu-img creates aligned files (by rounding up), it
> > must also gracefully handle files that are not sector-aligned.
> > Test that the bug fixed in the previous patch does not recur.
> > 
> > It's a bit annoying that we can see the (implicit) hole past
> > the end of the file on to the next sector boundary, so if we
> > ever reach the point where we report a byte-accurate size rather
> > than our current behavior of always rounding up, this test will
> > probably need a slight modification.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> 
> > +
> > +$QEMU_IO -c 'w 43008 1' "$TEST_IMG" # writing also rounds up
> 
> Shoot - missing a filter...
> 
> > +++ b/tests/qemu-iotests/221.out
> > @@ -0,0 +1,16 @@
> > +QA output created by 221
> > +
> > +=== Check mapping of unaligned raw image ===
> > +
> > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=43009
> > +[{ "start": 0, "length": 43520, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
> > +[{ "start": 0, "length": 43520, "depth": 0, "zero": true, "data": false, "offset": OFFSET}]
> > +wrote 1/1 bytes at offset 43008
> > +1 bytes, 1 ops; 0.0001 sec (7.512 KiB/sec and 7692.3077 ops/sec)
> 
> ...which leaks volatile output.  Squash this in:

Thanks, applied to the block branch. (After fixing the patch corruption
caused by your mailer helpfully adding line breaks.)

Kevin