Re: [Qemu-devel] [PATCH v2 05/14] block: Make blk_co_preadv/pwritev() public

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

Am 25.05.2016 um 02:24 hat Eric Blake geschrieben:
> On 05/24/2016 07:47 AM, Kevin Wolf wrote:
> > Also add trace points now that the function can be directly called.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/block-backend.c          | 21 ++++++++++++++-------
> >  include/sysemu/block-backend.h |  6 ++++++
> >  trace-events                   |  4 ++++
> >  3 files changed, 24 insertions(+), 7 deletions(-)
> > 
> 
> > @@ -741,11 +742,15 @@ static int blk_check_request(BlockBackend *blk, int64_t sector_num,
> >                                    nb_sectors * BDRV_SECTOR_SIZE);
> >  }
> >  
> > -static int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
> > -                                      unsigned int bytes, QEMUIOVector *qiov,
> > -                                      BdrvRequestFlags flags)
> > +int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
> > +                               unsigned int bytes, QEMUIOVector *qiov,
> 
> Isn't bytes redundant with qiov->size? Or can qiov be NULL?  Should we
> assert(!qiov || qiov->size == bytes)?

It's redundant for reads, as far as I know. I'm not completely sure yet
if the redundancy is completely useless; it might help to catch bugs
with the qiov initialisation in devices.

In any case, if we want to remove bytes from the interface, I think
that's a change that should be done consistently across all functions in
a series of its own and not hidden in something that is only meant to
touch block jobs.

> > +int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
> > +                                unsigned int bytes, QEMUIOVector *qiov,
> > +                                BdrvRequestFlags flags)
> >  {
> 
> Ditto. When doing write_zeroes, do we want qiov == NULL, must we always
> have qiov but just leave qiov->iov[0].base as NULL?  Probably worth
> documenting as part of making it public.

Here it's currently not redundant because we use qiov == NULL in some
callers. Other callers do the .iov_base = NULL, .iov_len = bytes thing.
Perhaps worth cleaning up, but again, not in this series.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]