[v2,1/3] block: Respect underlying file's EOF

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(-)

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

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