[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}(
From: |
Michael Tokarev |
Subject: |
Re: [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset() |
Date: |
Tue, 20 Mar 2012 00:04:36 +0400 |
User-agent: |
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:8.0) Gecko/20120216 Icedove/8.0 |
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.
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
- [Qemu-devel] [PATCHv4 09/11] export iov_send_recv() and use it in iov_send() and iov_recv(), (continued)
- [Qemu-devel] [PATCHv4 09/11] export iov_send_recv() and use it in iov_send() and iov_recv(), Michael Tokarev, 2012/03/15
- [Qemu-devel] [PATCHv4 08/11] rename qemu_sendv to iov_send, change proto and move declarations to iov.h, Michael Tokarev, 2012/03/15
- [Qemu-devel] [PATCHv4 01/11] virtio-serial-bus: use correct lengths in control_out() message, Michael Tokarev, 2012/03/15
- [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset(), Michael Tokarev, 2012/03/15
- Re: [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset(), Anthony Liguori, 2012/03/16
- Re: [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset(), Stefan Hajnoczi, 2012/03/19
- Re: [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset(),
Michael Tokarev <=
- Re: [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset(), Anthony Liguori, 2012/03/19
- Re: [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset(), Stefan Hajnoczi, 2012/03/20
[Qemu-devel] [PATCHv4 10/11] cleanup qemu_co_sendv(), qemu_co_recvv() and friends, Michael Tokarev, 2012/03/15
[Qemu-devel] [PATCHv4 06/11] consolidate qemu_iovec_copy() and qemu_iovec_concat() and make them consistent, Michael Tokarev, 2012/03/15
[Qemu-devel] [PATCHv4 11/11] rewrite iov_send_recv() and move it to iov.c, Michael Tokarev, 2012/03/15
[Qemu-devel] [PATCHv4 07/11] change qemu_iovec_to_buf() to match other to, from_buf functions, Michael Tokarev, 2012/03/15
[Qemu-devel] [PATCHv4 05/11] allow qemu_iovec_from_buffer() to specify offset from which to start copying, Michael Tokarev, 2012/03/15