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

[Anthony Liguori]

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<address@hidden>
> > >
> > > 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