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

[Michael Tokarev]

On 11.03.2012 05:49, Michael Tokarev wrote:
> 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)

And ofcourse I forgot to mention: the difference from
initial patch:

- picked 2 more qemu_iovec_* functions (minor), and
- changed order of arguments of sendv() and recvv().

The second point is relatively major, since it changes quite
some innocent places, and has a potential of breaking things
due to someone not noticing the args has been reordered.

But this is something I was wanted to do initially, because
this is how the args should has been initially (IMHO anyway),
but wasn't brave enough to actually change it.  But it is
better to do it now while we don't have lots of callers,
instead of not changing them at all because it's "too late"
and everyone got used to it.

Thanks,

/mjt