Re: [Qemu-devel] [PATCHv5 00/11] cleanup/consolidate iovec functions

[Michael Tokarev]

Hello again..  Can we do something with this stuff,
or should it finally die in peace? :)

Thanks!

/mjt

On 20.03.2012 01:22, Michael Tokarev wrote:
> This is my last attempt.  It is pointless to bother so much
> people with so much trivial stuff again and again and again.
> 
> This is cleanup/consolidation of iovec-related low-level
> routines in qemu.
> 
> The plan is to make library functions more understandable,
> consistent and useful, and to drop numerous implementations
> of the same thing.
> 
> 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:
> 
>   size_t 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, what, bytes
>  pattern.  This is consistent with (updated) iov_send()
>  and iov_recv() and friends, where `offset' and `bytes'
>  arguments were _renamed_, with the following prototypes:
> 
>    ssize_t iov_send(sockfd, iov, iov_cnt 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 iov_*
>  and qemu_iovec_*.   A few callers of these are verified and
>  converted.
> 
> 3. Always use iov_cnt (number of iov entries) together with
>   iov, to be able to verify that we don't go past the array.
>   iov_send (former qemu_sendv) and iov_recv accepts one extra
>   argument now, as are all other derivates (qemu_co_sendv etc).
> 
> 4. Used size_t instead of various variations for byte counts.
>  Including qemu_iovec_copy which used uint64_t(!) type.
> 
> 5. 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'.
> 
> 5. One implementation of the code remain. For example,
>  qemu_iovec_from_buffer() uses iov_from_buf() directly,
>  instead of repeating the same code.
> 
> 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 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 is already used in qcow2.c
> a bit).
> 
> The resulting thing should behave like current/existing
> code, there should be no behavor changes, just some
> shuffling and a bit of sanity checks.  It has been tested
> slightly, by booting different guests out of different
> image formats and doing some i/o, after each of the 11
> patches, and it all works correctly so far.
> 
> Changes since v4:
> 
> - added unit tests for iov_from_buf(), iov_to_buf(), iov_memset()
>   (in 3/11 "rewrite iov_* functions") and for iov_send_recv()
>   (in 11/11 "rewrite iov_send_recv() and move it to iov.c")
>   Note: the send_recv() test is unix-specific since it uses
>   socketpair(PF_UNIX) and fork() to test actual send/receive.
> - after running unit tests, fixed to bugs in iov_from_buf*()
>   and iov_send_recv() introduced during countless rewrites
>   of the code and rearranges of the patches to be even more
>   review-friendly.
> 
> Changes since v3:
> 
> - rename qemu_sendv(), qemu_sendv_recvv() to iov_send(),
>   iov_send_recv() and move them from qemu-common.h and
>   cutils.c to iov.h and iov.c.
> - add new argument, iov_cnt, to all send/recv-related
>   functions (iov_send_recv(), qemu_co_sendv_recv() etc).
>   This resulted in a bit more changes in other places,
>   some of which, while simple, may still be non-obvious
>   (block/nbd.c and block/sheepdog.c).
>   Due to the rename above and to introduction of the
>   new iov_cnt arg, the function signatures are changed
>   so no old code using new function wrongly is possible.
> - patch that changes iov_from_buf() & iov_to_buf() is
>   split into two halves: prototype changes and rewrite.
> - rewrite iov_send_recv() (former qemu_sendv_recvv())
>   again, slightly, to use newly introduced iov_cnt arg.
> 
> Changes since v2:
> 
> - covered iov.[ch] with iov_*() functions which contained
>   similar functionality
> - changed tabs to spaces as per suggestion by Kevin
> - explicitly allow to use large sizes for frombuf/tobuf
>   functions and friends, stopping at the end of iovec
>   in case more bytes requested when available, and
>   _returning_ number of actual bytes processed, but
>   made it extra clear what a return value will be.
> - used ssize_t for sendv/recvv instead of int, to
>   be consistent with all system headers and other
>   places in qemu
> - fix/clarify a case in my qemu_co_sendv_recvv()
>   of handling zero reads(recvs) and writes(sends).
> - covered qemu_iovec_to_buf() to have the same
>   pattern as other too (was missing in v2), and
>   use it in qcow2.c right away to simplify things
> - spotted and removed wrong usage of qiov instead of
>   plain iov in posix-aio-compat.c
> 
> What is _not_ covered:
> 
> - suggestion by pbonzini to not use macros for
>   access methods to call sendv_recvv with an
>   extra true/false argument: I still think the
>   usage of macros here is better than anything
>   else.
> - maybe re-shuffling of all this stuff into
>   different headers -- we already have iov.h,
>   maybe rename it to qemu-iov.h for consistency
>   and move all iov-related functions into there
>   (and ditto for cutils.c => (qemu-)iov.c).
> 
> Michael Tokarev (11):
>   virtio-serial-bus: use correct lengths in control_out() message
>   change iov_* function prototypes to be more appropriate
>   rewrite iov_* functions
>   consolidate qemu_iovec_memset{,_skip}() into single function and use 
> existing iov_memset()
>   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 qemu_iovec_to_buf() to match other to,from_buf functions
>   rename qemu_sendv to iov_send, change proto and move declarations to iov.h
>   export iov_send_recv() and use it in iov_send() and iov_recv()
>   cleanup qemu_co_sendv(), qemu_co_recvv() and friends
>   rewrite iov_send_recv() and move it to iov.c
> 
>  block.c                |   12 +-
>  block/curl.c           |    6 +-
>  block/iscsi.c          |    2 +-
>  block/nbd.c            |   18 ++--
>  block/qcow.c           |    4 +-
>  block/qcow2.c          |   19 ++--
>  block/qed.c            |   10 +-
>  block/rbd.c            |    2 +-
>  block/sheepdog.c       |    6 +-
>  block/vdi.c            |    4 +-
>  cutils.c               |  234 ++++++--------------------------------------
>  hw/9pfs/virtio-9p.c    |    8 +-
>  hw/rtl8139.c           |    2 +-
>  hw/usb/core.c          |    6 +-
>  hw/virtio-balloon.c    |    4 +-
>  hw/virtio-net.c        |    4 +-
>  hw/virtio-serial-bus.c |   10 +-
>  iov.c                  |  192 ++++++++++++++++++++++++++----------
>  iov.h                  |   77 +++++++++++++-
>  linux-aio.c            |    4 +-
>  net.c                  |    2 +-
>  posix-aio-compat.c     |    8 +-
>  qemu-common.h          |   56 +++++------
>  qemu-coroutine-io.c    |   83 +++++-----------
>  test-iov.c             |  256 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/Makefile         |    6 +-
>  26 files changed, 617 insertions(+), 418 deletions(-)
>  create mode 100644 test-iov.c
>