Re: [Qemu-devel] [PATCH 05/10] block/json: Add bdrv_co_get_block_status()

[Max Reitz <mreitz@redhat.com>]

On 06.03.2014 00:22, Benoît Canet wrote:
> The Wednesday 05 Mar 2014 à 21:44:57 (+0100), Max Reitz wrote :
>> On 05.03.2014 21:41, Benoît Canet wrote:
>>> The Wednesday 05 Mar 2014 à 21:10:03 (+0100), Max Reitz wrote :
>>>> On 05.03.2014 17:11, Benoît Canet wrote:
>>>>> The Monday 03 Mar 2014 à 16:28:48 (+0100), Max Reitz wrote :
>>>>>> Implement this function in the same way as raw_bsd does: Acknowledge
>>>>>> that this is a passthrough driver (always return BDRV_BLOCK_OFFSET_VALID
>>>>>> and BDRV_BLOCK_DATA and derive the offset directly from the sector
>>>>>> index) and add BDRV_BLOCK_RAW to the returned value.
>>>>>>
>>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>>> ---
>>>>>>   block/json.c | 9 +++++++++
>>>>>>   1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/block/json.c b/block/json.c
>>>>>> index a2f4691..7392802 100644
>>>>>> --- a/block/json.c
>>>>>> +++ b/block/json.c
>>>>>> @@ -113,6 +113,14 @@ static coroutine_fn int json_co_write_zeroes(BlockDriverState *bs,
>>>>>>       return bdrv_co_write_zeroes(bs, sector_num, nb_sectors, flags);
>>>>>>   }
>>>>>> +static coroutine_fn int64_t json_co_get_block_status(BlockDriverState *bs,
>>>>>> +                                                     int64_t sector_num,
>>>>>> +                                                     int nb_sectors, int *pnum)
>>>>>> +{
>>>>>> +    return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
>>>>>> +           (sector_num << BDRV_SECTOR_BITS);
>>>>>> +}
>>>>> I don't understand what is the selling point of this method instead of calling
>>>>> bdrv_co_get_block_status on bs->file.
>>>>> Some information risk to be lost and it does look like magic.
>>>> This is the same what "raw" does. It just is more meaningful: This
>>>> way, this function does not pretend to provide the blocks itself but
>>>> instead tells the truth; that is, the blocks are provided by an
>>>> underlying BDS (bs->file).
>>>>
>>>> I wasn't really sure what to do myself. Generally, this driver is
>>>> actually meant to pretend that it provides the blocks itself. On the
>>>> other hand, I tried to imitate the behavior or "raw", since this is
>>>> something I can hope to be approximately correct. Also, as I've said
>>>> before, the value returned here is in fact at least technically
>>>> correct.
>>> The raw_bsd driver have an additional *pnum = nb_sectors; at the begining of the
>>> function. Did you left it on purpose ?
>> Oops, no, I did not. Okay, if I even fail at copying code, the
>> argument about "raw" being at least probably correct isn't worth
>> anything here, I guess. ;-)
> In fact I wonder i posix_raw act this way because one of the BDRV it does wrap
> have simply no clue on how to answer to this request.

If you take a look at bdrv_co_get_block_status() in block.c, you'll see 
that BDRV_BLOCK_RAW actually results in a recursive call to 
bdrv_get_block_status().


Max

> Best regards
>
> Benoît
>
>> Max
>>
>>> Best regards
>>>
>>> Benoît
>>>> Max
>>>>
>>>>> Best regards
>>>>>
>>>>> Benoît
>>>>>
>>>>>> +
>>>>>>   static void json_invalidate_cache(BlockDriverState *bs)
>>>>>>   {
>>>>>>       return bdrv_invalidate_cache(bs->file);
>>>>>> @@ -159,6 +167,7 @@ static BlockDriver bdrv_json = {
>>>>>>       .bdrv_aio_discard           = json_aio_discard,
>>>>>>       .bdrv_co_write_zeroes       = json_co_write_zeroes,
>>>>>> +    .bdrv_co_get_block_status   = json_co_get_block_status,
>>>>>>       .bdrv_invalidate_cache      = json_invalidate_cache,
>>>>>> -- 
>>>>>> 1.9.0
>>>>>>
>>>>>>