Re: [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset()

[Michael Tokarev <mjt@tls.msk.ru>]

On 19.03.2012 18:36, Stefan Hajnoczi wrote:
> On Fri, Mar 16, 2012 at 11:19:03AM -0500, Anthony Liguori wrote:
>> On 03/15/2012 04:00 PM, Michael Tokarev wrote:
>>> This patch combines two functions into one, and replaces
>>> the implementation with already existing iov_memset() from
>>> iov.c.
>>>
>>> The new prototype of qemu_iovec_memset():
>>>   size_t qemu_iovec_memset(qiov, size_t offset, int fillc, size_t bytes)
>>> It is different from former qemu_iovec_memset_skip(), and
>>> I want to make other functions to be consistent with it
>>> too: first how much to skip, second what, and 3rd how many
>>> of it.  It also returns actual number of bytes filled in,
>>> which may be less than the requested `bytes' if qiov is
>>> smaller than offset+bytes, in the same way iov_memset()
>>> does.
>>>
>>> While at it, use utility function iov_memset() from
>>> iov.h in posix-aio-compat.c, where qiov was used.
>>>
>>> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru>
>>
>> Please CC Kevin at least when making block changes.
>>
>> It looks fine to me but would appreciate Kevin/Stefan taking a look too.
> 
> I am behind and feel that refactorings like this require careful
> technical review but don't buy us much.

I described what it gives in the cover message.  We've
several interfaces around the same thing, and even several
implementations of iov* functions doing the same but with
different argument order.  When the interface is wrong
(and in this case it was wrong in argument order), it
makes new code using it more error-prone.  Note that
whole thing I'm touching is used in some 10 places only.

Note that I provided a test program for all these functions
too.

Note also that you were CC'ed only for the patches which
touches block layer, for a review for the changes in there,
which is 2 one-liner changes.

>     The best way to get refactoring
> in is by making it part of a larger series that fixes a bug or adds a
> feature.

And for these, you'll tell that the changes should be in
a separate series,

>   I don't have bandwidth for non-trivial cosmetic stuff at the
> moment, sorry.

What's "bandwidth" in this context?

Initially I thought that just making 2 or 3 functions which
were inconsistent with each other to be a very easy task.
But the patchset grew to 11 patches and 5 versions, because
pbonzini said it is insufficient.  Now you're saying it is
too much.

I spent *much* more than any sane amount of time on this,
rediffing and rewriting, on a *trivial* thing, and now what?

If you can't plan even the most simple and low-level interface
to be consistent, please at least let someone who is STILL
willing to make it consistent to do that.  It is not cosmetic.
And if the code will grow this way further (and it does!),
it will be a very big ball of mud, unmaintainable.

Thank you?

/mjt