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

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

Thanks to patch 3, this series depends on my previous series
"[PATCH v10 00/14] qemu-img: Implement commit like QMP"
(strictly speaking only on patch 11 of that series which adds
_filter_qemu_img_map() to tests/qemu-iotests/common.filter).


Max Reitz (3):
  block: Ignore allocation size in underlying file
  qemu-io: Respect early image end for map
  iotests: Add test for map commands

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

-- 
2.0.4

[Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file

[Max Reitz]

When falling through to the underlying file in
bdrv_co_get_block_status(), 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 3e252a2..c922664 100644
--- a/block.c
+++ b/block.c
@@ -3991,9 +3991,11 @@ 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);
-        if (ret2 >= 0) {
+                                        *pnum, &backing_pnum);
+        if (ret2 >= 0 && backing_pnum >= *pnum) {
             /* Ignore errors.  This is just providing extra information, it
              * is useful but not necessary.
              */
-- 
2.0.4

[Qemu-devel] [PATCH 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>
---
 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 c503fc6..860b834 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1909,7 +1909,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;
@@ -1936,6 +1936,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";
-- 
2.0.4

[Qemu-devel] [PATCH 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 | 11 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 76 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..7ac46ef
--- /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" | _filter_qemu_img_map
+
+# 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..af3a2a6
--- /dev/null
+++ b/tests/qemu-iotests/102.out
@@ -0,0 +1,11 @@
+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          File
+0               0x10000         TEST_DIR/t.IMGFMT
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index dfbd0ed..b8f7c39 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -102,3 +102,4 @@
 095 rw auto quick
 097 rw auto backing
 098 rw auto backing quick
+102 rw auto quick
-- 
2.0.4

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

[Max Reitz]

On 16.08.2014 20:54, 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. 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.
>
> Thanks to patch 3, this series depends on my previous series
> "[PATCH v10 00/14] qemu-img: Implement commit like QMP"
> (strictly speaking only on patch 11 of that series which adds
> _filter_qemu_img_map() to tests/qemu-iotests/common.filter).
>
>
> Max Reitz (3):
>    block: Ignore allocation size in underlying file
>    qemu-io: Respect early image end for map
>    iotests: Add test for map commands
>
>   block.c                    |  6 +++--
>   qemu-io-cmds.c             |  5 +++-
>   tests/qemu-iotests/102     | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/102.out | 11 ++++++++
>   tests/qemu-iotests/group   |  1 +
>   5 files changed, 84 insertions(+), 3 deletions(-)
>   create mode 100755 tests/qemu-iotests/102
>   create mode 100644 tests/qemu-iotests/102.out
>

Ping.

Re: [Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On 08/16/2014 12:54 PM, Max Reitz wrote:
> When falling through to the underlying file in
> bdrv_co_get_block_status(), 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 | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On 08/16/2014 12:54 PM, Max Reitz wrote:
> 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>
> ---
>  qemu-io-cmds.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

On 08/16/2014 12:54 PM, Max Reitz wrote:
> 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 | 11 ++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 76 insertions(+)
>  create mode 100755 tests/qemu-iotests/102
>  create mode 100644 tests/qemu-iotests/102.out
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file

[Benoît Canet]

The Saturday 16 Aug 2014 à 20:54:16 (+0200), Max Reitz wrote :
> When falling through to the underlying file in
> bdrv_co_get_block_status(), 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 | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3e252a2..c922664 100644
> --- a/block.c
> +++ b/block.c
> @@ -3991,9 +3991,11 @@ 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);
> -        if (ret2 >= 0) {
> +                                        *pnum, &backing_pnum);
> +        if (ret2 >= 0 && backing_pnum >= *pnum) {

About backing_pnum >= *pnum.

The documentation of bdrv_co_get_block_status says:

 * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes   
 * beyond the end of the disk image it will be clamped.                         
 */                                                                             
static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,      
                                                     int64_t sector_num,        
                                                     int nb_sectors, int *pnum) 

So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum.

This means that  backing_pnum > *pnum will never happen.

I think either this test is wrong or the doc is wrong.

Best regards

Benoît


>              /* Ignore errors.  This is just providing extra information, it
>               * is useful but not necessary.
>               */
> -- 
> 2.0.4
> 
> 

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

[Benoît Canet]

> +        } else if (!num) {
> +            error_report("Unexpected end of image");
> +            return 0;

I think this test can miss some case of Unexpected end of image.

For example supose that in map_is_allocated the first bdrv_is_allocated
actually succeed then *pnum = num. Then the bottom loop has exit on failure
and the function return.

in map_f &num is map_is_allocated *pnum so map_f's num != 0 and this very test
fails to see the unexpected end of image error.

Best regards

Benoît

>          }
>  
>          retstr = ret ? "    allocated" : "not allocated";
> -- 
> 2.0.4
> 
> 

Re: [Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file

[Max Reitz]

Am 10.10.2014 um 13:50 schrieb Benoît Canet:
> The Saturday 16 Aug 2014 à 20:54:16 (+0200), Max Reitz wrote :
>> When falling through to the underlying file in
>> bdrv_co_get_block_status(), 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 | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 3e252a2..c922664 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3991,9 +3991,11 @@ 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);
>> -        if (ret2 >= 0) {
>> +                                        *pnum, &backing_pnum);
>> +        if (ret2 >= 0 && backing_pnum >= *pnum) {
> About backing_pnum >= *pnum.
>
> The documentation of bdrv_co_get_block_status says:
>
>   * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
>   * beyond the end of the disk image it will be clamped.
>   */
> static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>                                                       int64_t sector_num,
>                                                       int nb_sectors, int *pnum)
>
> So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum.
>
> This means that  backing_pnum > *pnum will never happen.
>
> I think either this test is wrong or the doc is wrong.

Thank you for confusing me, I had to think quite a while about this. *g*

The condition is not for error checking. If it was, it would be the 
wrong order (the condition should be true on success, that's why it's 
"ret2 >= 0" and not "ret2 < 0", so it should then be "backing_pnum <= 
*pnum"). So what this is testing is whether all sectors in the 
underlying file in the queried range are read as zero. But if 
"backing_pnum < *pnum" that is not the case, some clusters are not zero. 
So we may not set the zero flag if backing_pnum < *pnum; or as it reads 
in the code, we may only set it if backing_pnum >= *pnum. This is not 
about whether *pnum > backing_pnum, but more about whether backing_pnum 
== *pnum (but >= would be fine, too, if bdrv_co_get_block_status() 
supported it, so that's why I wrote it that way).

However, I'm starting to think about whether it would be better, for the 
backing_pnum < *pnum case, not to not set the zero flag, but rather 
simply set *pnum = backing_pnum. And this in turn would be pretty 
equivalent to just omitting this patch, because:

If we get to this point where we query the underlying file and it 
returns a certain number of sectors is zero; then we therefore want to 
set *pnum = backing_pnum (both if backing_pnum < *pnum and if 
backing_pnum == *pnum; backing_pnum > *pnum cannot happen, as you 
pointed out). On the other hand, if the sectors are not reported to be 
zero, but backing_pnum < *pnum, we want to shorten *pnum accordingly as 
well because this may indicate that after another backing_pnum sectors, 
we arrive at a hole in the file.

There is only one point I can imagine where it makes sense not to let 
backing_pnum overwrite *pnum: And that's if bdrv_co_get_block_status() 
reported BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID with an offset beyond 
the EOF. I think this might actually happen with qcow2, if one cluster 
simply lies beyond the EOF (which is perfectly valid). So I conclude 
that this patch has its use after all but needs to be modified so that 
backing_pnum always overwrites *pnum; except for when backing_pnum is 
zero (which should only happen at or after the EOF) in which case the 
zero flag should be set and *pnum should be left as it was.

And now in all honesty: Thanks for confusing me, I guess I can think 
better when I'm confused. :-)

Max

> Best regards
>
> Benoît
>
>
>>               /* Ignore errors.  This is just providing extra information, it
>>                * is useful but not necessary.
>>                */
>> -- 
>> 2.0.4
>>
>>

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

[Max Reitz]

Am 10.10.2014 um 14:03 schrieb Benoît Canet:
>> +        } else if (!num) {
>> +            error_report("Unexpected end of image");
>> +            return 0;
> I think this test can miss some case of Unexpected end of image.
>
> For example supose that in map_is_allocated the first bdrv_is_allocated
> actually succeed then *pnum = num. Then the bottom loop has exit on failure
> and the function return.
>
> in map_f &num is map_is_allocated *pnum so map_f's num != 0 and this very test
> fails to see the unexpected end of image error.

num != 0 because some sectors where successfully queried. In my opinion, 
we should print the information about them we have. Then, the do-while 
loop is repeated; and this time, map_is_allocated() either again returns 
num > 0 (for whatever reason, but I'd be fine with it) or, more 
probably, it'll be num == 0 this time. So the error is not missed, it's 
just printed one iteration later.

Max

> Best regards
>
> Benoît
>
>>           }
>>   
>>           retstr = ret ? "    allocated" : "not allocated";
>> -- 
>> 2.0.4
>>
>>

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

[Benoît Canet]

The Saturday 11 Oct 2014 à 11:53:40 (+0200), Max Reitz wrote :
> Am 10.10.2014 um 14:03 schrieb Benoît Canet:
> >>+        } else if (!num) {
> >>+            error_report("Unexpected end of image");
> >>+            return 0;
> >I think this test can miss some case of Unexpected end of image.
> >
> >For example supose that in map_is_allocated the first bdrv_is_allocated
> >actually succeed then *pnum = num. Then the bottom loop has exit on failure
> >and the function return.
> >
> >in map_f &num is map_is_allocated *pnum so map_f's num != 0 and this very test
> >fails to see the unexpected end of image error.
> 
> num != 0 because some sectors where successfully queried. In my opinion, we
> should print the information about them we have. Then, the do-while loop is
> repeated; and this time, map_is_allocated() either again returns num > 0
> (for whatever reason, but I'd be fine with it) or, more probably, it'll be
> num == 0 this time. So the error is not missed, it's just printed one
> iteration later.

Ok that make sense.

Best regards

Benoît

> 
> Max
> 
> >Best regards
> >
> >Benoît
> >
> >>          }
> >>          retstr = ret ? "    allocated" : "not allocated";
> >>-- 
> >>2.0.4
> >>
> >>
> 
> 

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

[Benoît Canet]

The Saturday 16 Aug 2014 à 20:54:17 (+0200), Max Reitz wrote :
> 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>
> ---
>  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 c503fc6..860b834 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1909,7 +1909,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;
> @@ -1936,6 +1936,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";
> -- 
> 2.0.4
> 
> 

Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Re: [Qemu-devel] [PATCH 1/3] block: Ignore allocation size in underlying file

[Benoît Canet]

The Saturday 11 Oct 2014 à 11:44:20 (+0200), Max Reitz wrote :
> Am 10.10.2014 um 13:50 schrieb Benoît Canet:
> >The Saturday 16 Aug 2014 à 20:54:16 (+0200), Max Reitz wrote :
> >>When falling through to the underlying file in
> >>bdrv_co_get_block_status(), 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 | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/block.c b/block.c
> >>index 3e252a2..c922664 100644
> >>--- a/block.c
> >>+++ b/block.c
> >>@@ -3991,9 +3991,11 @@ 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);
> >>-        if (ret2 >= 0) {
> >>+                                        *pnum, &backing_pnum);
> >>+        if (ret2 >= 0 && backing_pnum >= *pnum) {
> >About backing_pnum >= *pnum.
> >
> >The documentation of bdrv_co_get_block_status says:
> >
> >  * 'nb_sectors' is the max value 'pnum' should be set to.  If nb_sectors goes
> >  * beyond the end of the disk image it will be clamped.
> >  */
> >static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
> >                                                      int64_t sector_num,
> >                                                      int nb_sectors, int *pnum)
> >
> >So clearly after the bdrv_co_get_block_status *pnum >= backing_pnum.
> >
> >This means that  backing_pnum > *pnum will never happen.
> >
> >I think either this test is wrong or the doc is wrong.
> 
> Thank you for confusing me, I had to think quite a while about this. *g*
> 
> The condition is not for error checking. If it was, it would be the wrong
> order (the condition should be true on success, that's why it's "ret2 >= 0"
> and not "ret2 < 0", so it should then be "backing_pnum <= *pnum"). So what
> this is testing is whether all sectors in the underlying file in the queried
> range are read as zero. But if "backing_pnum < *pnum" that is not the case,
> some clusters are not zero. So we may not set the zero flag if backing_pnum
> < *pnum; or as it reads in the code, we may only set it if backing_pnum >=
> *pnum. This is not about whether *pnum > backing_pnum, but more about
> whether backing_pnum == *pnum (but >= would be fine, too, if
> bdrv_co_get_block_status() supported it, so that's why I wrote it that way).
> 
> However, I'm starting to think about whether it would be better, for the
> backing_pnum < *pnum case, not to not set the zero flag, but rather simply
> set *pnum = backing_pnum. And this in turn would be pretty equivalent to
> just omitting this patch, because:
> 
> If we get to this point where we query the underlying file and it returns a
> certain number of sectors is zero; then we therefore want to set *pnum =
> backing_pnum (both if backing_pnum < *pnum and if backing_pnum == *pnum;
> backing_pnum > *pnum cannot happen, as you pointed out). On the other hand,
> if the sectors are not reported to be zero, but backing_pnum < *pnum, we
> want to shorten *pnum accordingly as well because this may indicate that
> after another backing_pnum sectors, we arrive at a hole in the file.
> 
> There is only one point I can imagine where it makes sense not to let
> backing_pnum overwrite *pnum: And that's if bdrv_co_get_block_status()
> reported BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID with an offset beyond the
> EOF. I think this might actually happen with qcow2, if one cluster simply
> lies beyond the EOF (which is perfectly valid). So I conclude that this
> patch has its use after all but needs to be modified so that backing_pnum
> always overwrites *pnum; except for when backing_pnum is zero (which should
> only happen at or after the EOF) in which case the zero flag should be set
> and *pnum should be left as it was.
> 
> And now in all honesty: Thanks for confusing me, I guess I can think better
> when I'm confused. :-)
> 

You better have killer english skills to sumarize this in a nice commit message :)
I'll read the next version.

Best regards

Benoît

> Max
> 
> >Best regards
> >
> >Benoît
> >
> >
> >>              /* Ignore errors.  This is just providing extra information, it
> >>               * is useful but not necessary.
> >>               */
> >>-- 
> >>2.0.4
> >>
> >>
> 
>