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 <address@hidden>
> ---
>  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