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 <mre...@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
