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

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/19/2012 03:04 PM, Michael Tokarev wrote:
> 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.

Yeah, and I looked at it, and I believe Paolo gave some feedback which I fully 
agreed with (although I can't find the mail now).

I was expecting you to send another series with the test case included (and 
integrated into make check).

>
> 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?

I think Stefan is just pointing out that given his limited amount of time, he 
doesn't think the change is worth reviewing in detail.

I don't think he means to be offensive, just honest and open which is a good thing.

But since I have a vested interest in getting more test cases written, as I 
mentioned in a previous note, I'll review this series thoroughly and merge it 
when it comes with a test case, so please resubmit.

Regards,

Anthony Liguori