[Qemu-devel] [PATCH v3 0/3] block: Fix is_allocated() for truncated images

[Qemu-devel] [PATCH v3 0/3] block: Fix is_allocated() for truncated images

[Max Reitz]

Patch 2:
The bdrv_is_allocated() functions may return a number of zero sectors
e.g. if a sector beyond the image end has been queried. Respect this
case in qemu-io's map implementation so it doesn't run into an infinite
loop (https://bugs.launchpad.net/qemu/+bug/1356969).

Patch 1:
In that bug report, bdrv_co_get_block_status() fell through to the
underlying file to get additional information (whether the sectors are
zeroed). However, the offset reported by the image format (qcow2) was
beyond the size of the underlying file and so this second query returned
only zero clusters which replaced the actual number of sectors queried
in the "formatted" (on qcow2 level) image. This replacement is generally
fine because it allows bdrv_co_get_block_status() to follow the
granularity (holes and non-holes) of the underlying file, but not if
only zero clusters could be queried (which indicates a request beyond
the image end).
This patch makes bdrv_co_get_block_status() respect the underlying
file's EOF and act accordingly.

Patch 3 adds a test for patch 1; a test which depends on patch 2 is
included in v3 of my series "raw-posix: Fix raw_co_get_block_status()".


v3:
- Patch 1:
  - %s/backing_pnum/file_pnum/ [Kevin]
  - ret |= BDRV_BLOCK_ZERO instead of ret = BDRV_BLOCK_ZERO (and modify
    the comment accordingly) [Kevin]
- Patch 3: With the latter change in patch 1, the sectors are now marked
  as allocated instead of not allocated; the qemu-img map output however
  stays the same (and therefore doesn't require the filter from the
  qemu-img/QMP commit series), because the sectors are marked as zero
  and qemu-img only outputs non-zero data clusters.


git-backport-diff against v2:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/3:[0015] [FC] 'block: Respect underlying file's EOF'
002/3:[----] [--] 'qemu-io: Respect early image end for map'
003/3:[0002] [FC] 'iotests: Add test for map commands'


Max Reitz (3):
  block: Respect underlying file's EOF
  qemu-io: Respect early image end for map
  iotests: Add test for map commands

 block.c                    | 15 +++++++++--
 qemu-io-cmds.c             |  5 +++-
 tests/qemu-iotests/102     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/102.out | 10 ++++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 92 insertions(+), 3 deletions(-)
 create mode 100755 tests/qemu-iotests/102
 create mode 100644 tests/qemu-iotests/102.out

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/3] block: Respect underlying file's EOF

[Max Reitz]

When falling through to the underlying file in
bdrv_co_get_block_status(), if it returns that the query offset is
beyond the file end (by setting *pnum to 0), return the range to be
zero and do not let the number of sectors for which information could be
obtained be overwritten.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index bbb04e7..88f6d9b 100644
--- a/block.c
+++ b/block.c
@@ -3954,13 +3954,24 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     if (bs->file &&
         (ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&
         (ret & BDRV_BLOCK_OFFSET_VALID)) {
+        int file_pnum;
+
         ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
-                                        *pnum, pnum);
+                                        *pnum, &file_pnum);
         if (ret2 >= 0) {
             /* Ignore errors.  This is just providing extra information, it
              * is useful but not necessary.
              */
-            ret |= (ret2 & BDRV_BLOCK_ZERO);
+            if (!file_pnum) {
+                /* !file_pnum indicates an offset at or beyond the EOF; it is
+                 * perfectly valid for the format block driver to point to such
+                 * offsets, so catch it and mark everything as zero */
+                ret |= BDRV_BLOCK_ZERO;
+            } else {
+                /* Limit request to the range reported by the protocol driver */
+                *pnum = file_pnum;
+                ret |= (ret2 & BDRV_BLOCK_ZERO);
+            }
         }
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH v3 2/3] qemu-io: Respect early image end for map

[Max Reitz]

bdrv_is_allocated() may report zero clusters which most probably means
the image (file) is shorter than expected. Respect this case in order to
avoid an infinite loop.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index b224ede..d94fb1e 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1900,7 +1900,7 @@ static int map_is_allocated(BlockDriverState *bs, int64_t sector_num,
 
         num_checked = MIN(nb_sectors, INT_MAX);
         ret = bdrv_is_allocated(bs, sector_num, num_checked, &num);
-        if (ret == firstret) {
+        if (ret == firstret && num) {
             *pnum += num;
         } else {
             break;
@@ -1927,6 +1927,9 @@ static int map_f(BlockDriverState *bs, int argc, char **argv)
         if (ret < 0) {
             error_report("Failed to get allocation status: %s", strerror(-ret));
             return 0;
+        } else if (!num) {
+            error_report("Unexpected end of image");
+            return 0;
         }
 
         retstr = ret ? "    allocated" : "not allocated";
-- 
1.9.3

[Qemu-devel] [PATCH v3 3/3] iotests: Add test for map commands

[Max Reitz]

Add a test for qemu-img map and qemu-io -c map on truncated files.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/102     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/102.out | 10 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 75 insertions(+)
 create mode 100755 tests/qemu-iotests/102
 create mode 100644 tests/qemu-iotests/102.out

diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102
new file mode 100755
index 0000000..34b363f
--- /dev/null
+++ b/tests/qemu-iotests/102
@@ -0,0 +1,64 @@
+#!/bin/bash
+#
+# Test case for qemu-io -c map and qemu-img map
+#
+# Copyright (C) 2014 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/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+here=$PWD
+tmp=/tmp/$$
+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 qcow2
+_supported_proto file
+_supported_os Linux
+
+IMG_SIZE=64K
+
+echo
+echo '=== Testing map command on truncated image ==='
+echo
+
+_make_test_img $IMG_SIZE
+# Create cluster
+$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
+# Remove data cluster from image (first cluster: image header, second: reftable,
+# third: refblock, fourth: L1 table, fifth: L2 table)
+truncate -s $((5 * 64 * 1024)) "$TEST_IMG"
+
+$QEMU_IO -c map "$TEST_IMG"
+$QEMU_IMG map "$TEST_IMG"
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/102.out b/tests/qemu-iotests/102.out
new file mode 100644
index 0000000..e0e9cdc
--- /dev/null
+++ b/tests/qemu-iotests/102.out
@@ -0,0 +1,10 @@
+QA output created by 102
+
+=== Testing map command on truncated image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+[                       0]      128/     128 sectors     allocated at offset 0 bytes (1)
+Offset          Length          Mapped to       File
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index be2054f..c9752e9 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -103,6 +103,7 @@
 099 rw auto quick
 100 rw auto quick
 101 rw auto quick
+102 rw auto quick
 103 rw auto quick
 104 rw auto
 105 rw auto quick
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 0/3] block: Fix is_allocated() for truncated images

[Kevin Wolf]

Am 22.10.2014 um 17:00 hat Max Reitz geschrieben:
> Patch 2:
> The bdrv_is_allocated() functions may return a number of zero sectors
> e.g. if a sector beyond the image end has been queried. Respect this
> case in qemu-io's map implementation so it doesn't run into an infinite
> loop (https://bugs.launchpad.net/qemu/+bug/1356969).
> 
> Patch 1:
> In that bug report, bdrv_co_get_block_status() fell through to the
> underlying file to get additional information (whether the sectors are
> zeroed). However, the offset reported by the image format (qcow2) was
> beyond the size of the underlying file and so this second query returned
> only zero clusters which replaced the actual number of sectors queried
> in the "formatted" (on qcow2 level) image. This replacement is generally
> fine because it allows bdrv_co_get_block_status() to follow the
> granularity (holes and non-holes) of the underlying file, but not if
> only zero clusters could be queried (which indicates a request beyond
> the image end).
> This patch makes bdrv_co_get_block_status() respect the underlying
> file's EOF and act accordingly.
> 
> Patch 3 adds a test for patch 1; a test which depends on patch 2 is
> included in v3 of my series "raw-posix: Fix raw_co_get_block_status()".

Thanks, applied to the block branch.

Kevin