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

[Qemu-devel] [PATCH v2 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.

But because errors from this
second call are ignored, the number of sector queried successfully
should be ignored as well, which makes qemu-io -c map actually work for
that bug report.

Patch 3 adds a test for patch 1; I could not conceive a test for patch 2
which patch 1 would not already catch, but patch 2 should be simple
enough not to require a test anyway.


v2:
- In v1, patch 1 completely ignored the number of queried sectors
  returned by the recursive call; but it should only be ignored in case
  of EOF (which is indicated by returning 0 as the queried clusters
  count)
- Thanks to fixing patch 1, the area queried in the test added in patch
  3 is now marked unallocated (which is correct because it's beyond the
  EOF). Therefore, this test does no longer require
  _filter_qemu_img_map() (because there are no mappings shown) and this
  series no longer depends on the qemu-img/QMP commit series.


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                    | 16 ++++++++++--
 qemu-io-cmds.c             |  5 +++-
 tests/qemu-iotests/102     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/102.out | 10 ++++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 93 insertions(+), 3 deletions(-)
 create mode 100755 tests/qemu-iotests/102
 create mode 100644 tests/qemu-iotests/102.out

-- 
1.9.3

[Qemu-devel] [PATCH v2 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 | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 773e87e..68dc11d 100644
--- a/block.c
+++ b/block.c
@@ -3954,13 +3954,25 @@ 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 backing_pnum;
+
         ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
-                                        *pnum, pnum);
+                                        *pnum, &backing_pnum);
         if (ret2 >= 0) {
             /* Ignore errors.  This is just providing extra information, it
              * is useful but not necessary.
              */
-            ret |= (ret2 & BDRV_BLOCK_ZERO);
+            if (!backing_pnum) {
+                /* !backing_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 unallocated and
+                 * empty */
+                ret = BDRV_BLOCK_ZERO;
+            } else {
+                /* Limit request to the range reported by the protocol driver */
+                *pnum = backing_pnum;
+                ret |= (ret2 & BDRV_BLOCK_ZERO);
+            }
         }
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 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>
---
 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 v2 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..f0a91ed
--- /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 not allocated at offset 0 bytes (0)
+Offset          Length          Mapped to       File
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b230996..e4596b1 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 v2 0/3] block: Fix is_allocated() for truncated images

[Max Reitz]

On 2014-10-22 at 15:24, Max Reitz wrote:
> 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.

Sorry, disregard the following paragraph (which I deliberately 
highlighted so it's easier to disregard).

vvv
> But because errors from this
> second call are ignored, the number of sector queried successfully
> should be ignored as well, which makes qemu-io -c map actually work for
> that bug report.
^^^

> Patch 3 adds a test for patch 1; I could not conceive a test for patch 2
> which patch 1 would not already catch, but patch 2 should be simple
> enough not to require a test anyway.
>
>
> v2:
> - In v1, patch 1 completely ignored the number of queried sectors
>    returned by the recursive call; but it should only be ignored in case
>    of EOF (which is indicated by returning 0 as the queried clusters
>    count)
> - Thanks to fixing patch 1, the area queried in the test added in patch
>    3 is now marked unallocated (which is correct because it's beyond the
>    EOF). Therefore, this test does no longer require
>    _filter_qemu_img_map() (because there are no mappings shown) and this
>    series no longer depends on the qemu-img/QMP commit series.
>
>
> 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                    | 16 ++++++++++--
>   qemu-io-cmds.c             |  5 +++-
>   tests/qemu-iotests/102     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/102.out | 10 ++++++++
>   tests/qemu-iotests/group   |  1 +
>   5 files changed, 93 insertions(+), 3 deletions(-)
>   create mode 100755 tests/qemu-iotests/102
>   create mode 100644 tests/qemu-iotests/102.out
>

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

[Kevin Wolf]

Am 22.10.2014 um 15:24 hat Max Reitz geschrieben:
> 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 | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 773e87e..68dc11d 100644
> --- a/block.c
> +++ b/block.c
> @@ -3954,13 +3954,25 @@ 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 backing_pnum;

Sounds as if it were about bs->backing_hd, whereas it really is about
bs->file. Perhaps we can find a better name.

> +
>          ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
> -                                        *pnum, pnum);
> +                                        *pnum, &backing_pnum);
>          if (ret2 >= 0) {
>              /* Ignore errors.  This is just providing extra information, it
>               * is useful but not necessary.
>               */
> -            ret |= (ret2 & BDRV_BLOCK_ZERO);
> +            if (!backing_pnum) {
> +                /* !backing_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 unallocated and
> +                 * empty */
> +                ret = BDRV_BLOCK_ZERO;

I don't think this is correct. Even though the area is unallocated in
bs->file, it's certainly allocated in bs (BDRV_BLOCK_ALLOCATED) and is
read from bs->file (BDRV_BLOCK_DATA). It is arguable whether an offset
beyond EOF is valid (BDRV_BLOCK_OFFSET_VALID), but in terms of the qemu
block layer I'm inclined to say yes.

So I'd suggest ret |= BDRV_BLOCK_ZERO instead.

> +            } else {
> +                /* Limit request to the range reported by the protocol driver */
> +                *pnum = backing_pnum;
> +                ret |= (ret2 & BDRV_BLOCK_ZERO);
> +            }
>          }
>      }

Kevin

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

[Max Reitz]

On 2014-10-22 at 15:40, Kevin Wolf wrote:
> Am 22.10.2014 um 15:24 hat Max Reitz geschrieben:
>> 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 | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 773e87e..68dc11d 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3954,13 +3954,25 @@ 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 backing_pnum;
> Sounds as if it were about bs->backing_hd, whereas it really is about
> bs->file. Perhaps we can find a better name.

Good question why I called it that way. I don't know and just haven't 
thought about it afterwards.

>> +
>>           ret2 = bdrv_co_get_block_status(bs->file, ret >> BDRV_SECTOR_BITS,
>> -                                        *pnum, pnum);
>> +                                        *pnum, &backing_pnum);
>>           if (ret2 >= 0) {
>>               /* Ignore errors.  This is just providing extra information, it
>>                * is useful but not necessary.
>>                */
>> -            ret |= (ret2 & BDRV_BLOCK_ZERO);
>> +            if (!backing_pnum) {
>> +                /* !backing_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 unallocated and
>> +                 * empty */
>> +                ret = BDRV_BLOCK_ZERO;
> I don't think this is correct. Even though the area is unallocated in
> bs->file, it's certainly allocated in bs (BDRV_BLOCK_ALLOCATED) and is
> read from bs->file (BDRV_BLOCK_DATA). It is arguable whether an offset
> beyond EOF is valid (BDRV_BLOCK_OFFSET_VALID), but in terms of the qemu
> block layer I'm inclined to say yes.
>
> So I'd suggest ret |= BDRV_BLOCK_ZERO instead.

You're right, this fits in better with the intention of this block of 
code (the whole recursion to the protocol layer), too.

Thanks,

Max

>> +            } else {
>> +                /* Limit request to the range reported by the protocol driver */
>> +                *pnum = backing_pnum;
>> +                ret |= (ret2 & BDRV_BLOCK_ZERO);
>> +            }
>>           }
>>       }
> Kevin

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

[Kevin Wolf]

Am 22.10.2014 um 15:24 hat Max Reitz geschrieben:
> 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>

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

[Kevin Wolf]

Am 22.10.2014 um 15:24 hat Max Reitz geschrieben:
> Add a test for qemu-img map and qemu-io -c map on truncated files.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

But how about adding a case for patch 2, too? Or is that one mostly
theoretical (like the image file being modified in the background) and
not reproducible reliably?

Kevin

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

[Max Reitz]

On 2014-10-22 at 15:51, Kevin Wolf wrote:
> Am 22.10.2014 um 15:24 hat Max Reitz geschrieben:
>> Add a test for qemu-img map and qemu-io -c map on truncated files.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
> But how about adding a case for patch 2, too? Or is that one mostly
> theoretical (like the image file being modified in the background) and
> not reproducible reliably?

See the cover letter, I could not find a way to test patch 2 without 
triggering the changes introduced by patch 1. Yes, modifying the image 
in the background could be a way to do this. I could try, but I don't 
know if we really need a test for it.

I'll give myself a couple of minutes and if it doesn't work, well, then 
this test stays the same in v3.

Max

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

[Max Reitz]

On 2014-10-22 at 15:54, Max Reitz wrote:
> On 2014-10-22 at 15:51, Kevin Wolf wrote:
>> Am 22.10.2014 um 15:24 hat Max Reitz geschrieben:
>>> Add a test for qemu-img map and qemu-io -c map on truncated files.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>
>> But how about adding a case for patch 2, too? Or is that one mostly
>> theoretical (like the image file being modified in the background) and
>> not reproducible reliably?
>
> See the cover letter, I could not find a way to test patch 2 without 
> triggering the changes introduced by patch 1. Yes, modifying the image 
> in the background could be a way to do this. I could try, but I don't 
> know if we really need a test for it.
>
> I'll give myself a couple of minutes and if it doesn't work, well, 
> then this test stays the same in v3.

And I just realized that my series "raw-posix: Fix 
raw_co_get_block_status()" contains exactly such a test, which is also 
the reason why it doesn't work without this series.

Max

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

[Max Reitz]

On 2014-10-22 at 16:48, Max Reitz wrote:
> On 2014-10-22 at 15:54, Max Reitz wrote:
>> On 2014-10-22 at 15:51, Kevin Wolf wrote:
>>> Am 22.10.2014 um 15:24 hat Max Reitz geschrieben:
>>>> Add a test for qemu-img map and qemu-io -c map on truncated files.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>>
>>> But how about adding a case for patch 2, too? Or is that one mostly
>>> theoretical (like the image file being modified in the background) and
>>> not reproducible reliably?
>>
>> See the cover letter, I could not find a way to test patch 2 without 
>> triggering the changes introduced by patch 1. Yes, modifying the 
>> image in the background could be a way to do this. I could try, but I 
>> don't know if we really need a test for it.
>>
>> I'll give myself a couple of minutes and if it doesn't work, well, 
>> then this test stays the same in v3.
>
> And I just realized that my series "raw-posix: Fix 
> raw_co_get_block_status()" contains exactly such a test, which is also 
> the reason why it doesn't work without this series.

And now I realized I hadn't sent out the latest version of that series, 
which includes this test, yet. Will do.

Max