Re: [Qemu-devel] [PATCH 16/17] block: add default get_block_status implementation for protocols

[Peter Lieven]

Am 16.07.2013 um 09:19 schrieb Paolo Bonzini <address@hidden>:

> Il 16/07/2013 08:47, Peter Lieven ha scritto:
>>> 
>>> @@ -2977,7 +2977,11 @@ static int64_t coroutine_fn
>>> bdrv_co_get_block_status(BlockDriverState *bs,
>>>        if (!bs->drv->bdrv_co_get_block_status) {
>>>          *pnum = nb_sectors;
>>> -        return BDRV_BLOCK_DATA;
>>> +        ret = BDRV_BLOCK_DATA;
>>> +        if (bs->drv->protocol_name) {
>>> +            ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num *
>>> BDRV_SECTOR_SIZE);
>>> +        }
>>> +        return ret;
>>>      }
>>>        ret = bs->drv->bdrv_co_get_block_status(bs, sector_num,
>>> nb_sectors, pnum);
>> I am curious if this is right. Doesn't this mean we say that at offset
>> sector_num * BDRV_SECTOR_SIZE are nb_sectors of linear data? This is
>> something we do not know for sure.
> 
> Only for protocols.  In this case, we do know, or format=raw wouldn't
> work.  This is not propagated up to the actual BDS for the image, except
> for format=raw.

Wouldn't it be better to add this general tweak in to raw_co_is_allocated
rather than at the block level?

What you write above is true, but you will never know what will happen in the
future. For raw this tweak is right, but for anything developed in the future
it might not be and in the end nobody remembers to fix it at this position.

I was just thinking of the has_zero_init = 1 thing. At the time it was written
the code was correct and then came more and more extensions like
extends on a block device or iscsi and nobody remembered.

Peter