From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48863) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9iuN-0003Pz-H0 for qemu-devel@nongnu.org; Mon, 19 Mar 2012 16:10:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S9iuH-0002FD-OG for qemu-devel@nongnu.org; Mon, 19 Mar 2012 16:10:23 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:36429) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9iuH-0002F3-JW for qemu-devel@nongnu.org; Mon, 19 Mar 2012 16:10:17 -0400 Received: by yhoo21 with SMTP id o21so6804041yho.4 for ; Mon, 19 Mar 2012 13:10:15 -0700 (PDT) Message-ID: <4F6792A5.3050409@codemonkey.ws> Date: Mon, 19 Mar 2012 15:10:13 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1331845217-21705-1-git-send-email-mjt@msgid.tls.msk.ru> <1331845217-21705-5-git-send-email-mjt@msgid.tls.msk.ru> <4F6367F7.7040506@redhat.com> <20120319143613.GB19040@stefanha-thinkpad.localdomain> <4F679154.9080903@msgid.tls.msk.ru> In-Reply-To: <4F679154.9080903@msgid.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv4 04/11] consolidate qemu_iovec_memset{, _skip}() into single function and use existing iov_memset() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: Kevin Wolf , Paolo Bonzini , Stefan Hajnoczi , qemu-devel@nongnu.org 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 >>> >>> 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