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

[Vladimir Sementsov-Ogievskiy]

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


--
Best regards,
Vladimir