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<m...@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