Re: [Qemu-devel] [PATCHv2 0/7] cleanup/consolidate some iovec functions

[Paolo Bonzini]

Il 11/03/2012 02:49, Michael Tokarev ha scritto:
> This is a little cleanup/consolidation for some iovec-related
> low-level routines in qemu.
> 
> The plan is to make library functions more understandable,
> consistent and useful.
> 
> The patch changes prototypes of several iov and qiov functions
> to match each other, changes types of arguments for some
> functions, _swaps_ some function arguments with each other,
> and makes use of common code in r/w path.
> 
> The result of all these changes.
> 
> 1. Most qiov-related (qemu_iovec_*) functions now accepts
>  'offset' parameter to specify from which (byte) position to
>  start operation.  This is added for _memset (removing
>  _memset_skip), _from_buffer (allowing to copy a bounce-
>  buffer to a middle of qiov).  Typical:
> 
>   void qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, int c, size_t 
> bytes);
> 
> 2. All functions that accepts this `offset' argument does
>  it in a similar manner, following the
>    iov,fromwhere,bytes
>  pattern.  This is consistent with (updated) qemu_sendv()
>  and qemu_recvv() and friends, where `offset' and `bytes'
>  arguments were _renamed_, with the following prototypes:
> 
>    int qemu_sendv(sockfd, iov, size_t offset, size_t bytes)
> 
>  instead of
> 
>    int qemu_sendv(sockfd, iov, int len, int iov_offset)
> 
>  See how offset & bytes are used in the same way as for qemu_iovec_*
>  A few callers of these are verified and converted.
> 
> 3. Used size_t instead of various variations for byte counts.
>  Including qemu_iovec_copy which used uint64_t(!) type.
> 
> 4. Function arguments are renamed to better match with their
>  actual meaning.  Compare new and original prototype of
>  qemu_sendv() above: old prototype with `len' does not
>  tell if `len' refers to number of iov elements (as
>  regular writev() call) or to number of data bytes.
>  Ditto for several usages of `count' for some qemu_iovec_*,
>  which is also replaced to `bytes'.
> 
> The resulting function usage is much more consistent, the
> functions themselves are nice and understandable, which
> means they're easier to use and less error-prone.
> 
> This patchset also consolidates a few low-level send&recv
> functions into one, since both versions were exactly
> the same (and were finally calling common function anyway).
> This is done by exporting a common send_recv function with
> one extra bool argument, and making current send&recv to
> be just #defines.
> 
> And while at it all, also made some implementations shorter,
> cleaner and much easier to read/understand, and add some
> code comments.
> 
> The read&write consolidation has great potential for the
> block layer, as has been demonstrated before.
> Unification and generalization of qemu_iovec_* functions
> will let to optimize/simplify some more code in block/*,
> especially qemu_iovec_memset() and _from_buffer() (this
> optimization/simplification isn't done in this series)
> 
> Michael Tokarev (7):
>   Consolidate qemu_iovec_memset{,_skip}() into single, simplified function
>   allow qemu_iovec_from_buffer() to specify offset from which to start copying
>   consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them 
> consistent
>   change prototypes of qemu_sendv() and qemu_recvv()
>   Export qemu_sendv_recvv() and use it in qemu_sendv() and qemu_recvv()
>   cleanup qemu_co_sendv(), qemu_co_recvv() and friends
>   rewrite and comment qemu_sendv_recvv()
> 
>  block.c             |    8 +-
>  block/curl.c        |    4 +-
>  block/nbd.c         |    4 +-
>  block/qcow.c        |    2 +-
>  block/qcow2.c       |   14 ++--
>  block/qed.c         |   10 +-
>  block/sheepdog.c    |    6 +-
>  block/vdi.c         |    2 +-
>  cutils.c            |  275 
> +++++++++++++++++++++------------------------------
>  hw/9pfs/virtio-9p.c |    8 +-
>  linux-aio.c         |    4 +-
>  posix-aio-compat.c  |    2 +-
>  qemu-common.h       |   64 +++++++-----
>  qemu-coroutine-io.c |   83 ++++-----------
>  14 files changed, 205 insertions(+), 281 deletions(-)
> 

Looks good, except that I don't like #defines.  (I also don't like
exporting qemu_sendv_recvv, but I can live with it. :))  I commented on
the single patches.

I'm happy to take 4-7 via the NBD tree.

Paolo