Re: [PATCH v4 4/5] block: drop bdrv_prwv

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

27.05.2020 00:34, Eric Blake wrote:
> On 5/25/20 5:08 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Now, when we are not more paying extra code for coroutine wrappers,
>> there no more sence in extra indirection layer: bdrv_prwv(). Let's drop
>> it and instead genereate pure bdrv_preadv() and bdrv_pwritev().
> 
> Typos and grammar; I suggest:
> 
> Now that we are not maintaining boilerplate code for coroutine wrappers, there is no more sense in keeping the extra indirection layer of bdrv_prwv().  Let's drop it and instead generate pure bdrv_preadv() and bdrv_pwritev().
> 
>>
>> Currently, bdrv_pwritev() and bdrv_preadv() are returning bytes on
>> success, auto generated functions will instead return zero, as their
>> _co_ prototype. Still, it's simple to make the conversion safe: the
>> only external user of bdrv_pwritev() is test-bdrv-drain, and it is
>> comfortable enough with bdrv_co_pwritev() instead. So prototypes are
>> moved to local block/coroutines.h. Next, the only internal use is
>> bdrv_pread() and bdrv_pwrite(), which are modified to return bytes on
>> success.
> 
> Does returning bytes on success buy us anything useful?  We don't allow partial success, so blindly returning 0 on success is no less useful. True, we'd have to audit callers to make sure we aren't doing an inadvertent semantic change.

Not so simple.. Seems we have 151 calls in 23 files:

# git grep -l '\(bdrv_pread\|bdrv_pwrite\)\>' '*.[hc]' | wc -l
23
# git grep '\(bdrv_pread\|bdrv_pwrite\)\>' '*.[hc]' | wc -l
151

Amyway, let it be another series. And such series should probably change most of calls to _co_ variants.

> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/coroutines.h      | 10 ++++-----
>>   include/block/block.h   |  2 --
>>   block/io.c              | 49 ++++++++---------------------------------
>>   tests/test-bdrv-drain.c |  2 +-
>>   4 files changed, 15 insertions(+), 48 deletions(-)
>>
> 
> At any rate, I think this patch is reasonable.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


-- 
Best regards,
Vladimir