From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35623) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2mKa-00068Z-3d for qemu-devel@nongnu.org; Wed, 29 Feb 2012 11:24:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S2mKT-0001Jt-NJ for qemu-devel@nongnu.org; Wed, 29 Feb 2012 11:24:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48244) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2mKT-0001Jh-Du for qemu-devel@nongnu.org; Wed, 29 Feb 2012 11:24:37 -0500 Message-ID: <4F4E5140.5030600@redhat.com> Date: Wed, 29 Feb 2012 17:24:32 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1330473276-8975-1-git-send-email-mjt@tls.msk.ru> <1330473276-8975-4-git-send-email-mjt@msgid.tls.msk.ru> <4F4E4BDA.9000304@redhat.com> <4F4E4E7F.8050202@msgid.tls.msk.ru> In-Reply-To: <4F4E4E7F.8050202@msgid.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Tokarev Cc: kwolf@redhat.com, qemu-devel@nongnu.org Il 29/02/2012 17:12, Michael Tokarev ha scritto: > On 29.02.2012 20:01, Paolo Bonzini wrote: >> Il 29/02/2012 00:54, Michael Tokarev ha scritto: >>> BlockDriver *drv = bs->drv; >>> BdrvTrackedRequest req; >>> + bool is_write = flags & (BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE); >>> int ret; >> >> You can do BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE, but not >> BDRV_REQ_READ|BDRV_REQ_COPY_ON_READ. That's ugly. > > BDRV_REQ_READ is zero. This is just mnemonic to avoid "magic > numbers" elsewhere in the code. This is an internal function > and the comment above it says just that, and it is always > called with just ONE value. It is not a bitmask, it is used > as such inside this very routine ONLY. The argument is declared > as enum too, -- this should tell something. In the function > prototype it should have been named "opcode" or "request", > not "flags". It is used as flags only inside this function. > > This code isn't written by me, it was this way before. > I just added 2 more possible values for this parameter. If you have 4 values, make them 1/2/4/8 or 0/1/2/3. Not 0/1/2/4. > No block driver -- at least currently -- needs any other value > here except of read-or-write (or is_write). COPY_ON_READ is > not a business of the individual block drivers currently. Sure, but ZERO_WRITES is (we have a separate callback). > These defines are _only_ to make some code a bit more readable, > in a very few places where it necessary to call individual > read or write block driver method. So that the construct: > > ret - s->bdrv_co_rw_vector(bs, ..., true) > > becomes > > ret - s->bdrv_co_rw_vector(bs, ..., BDRV_WRITE) > > and it is immediately obvious that it is write. The prototype > of the method has "bool is_write" here. If you use an enum, the prototype shouldn't be bool. >> But I'm skeptical, the >> actual amount of unification is not that large. > > This is not about unification. This is, as described in the introduction > email, about removing complexity of a very twisted nature of read and > write code paths, for a start. The patches are a balance of removing duplication and adding conditionals, no? Removing duplication is unification. Paolo