From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36188) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2MvK-0003rr-Nu for qemu-devel@nongnu.org; Tue, 28 Feb 2012 08:17:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S2MvB-0006Uc-KM for qemu-devel@nongnu.org; Tue, 28 Feb 2012 08:16:58 -0500 Received: from isrv.corpit.ru ([86.62.121.231]:60152) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2MvB-0006UO-CO for qemu-devel@nongnu.org; Tue, 28 Feb 2012 08:16:49 -0500 Message-ID: <4F4CD3BE.2010309@msgid.tls.msk.ru> Date: Tue, 28 Feb 2012 17:16:46 +0400 From: Michael Tokarev MIME-Version: 1.0 References: <20120228111424.2DC63195@gandalf.tls.msk.ru> <4F4CBBE5.8000707@redhat.com> <4F4CCA17.7080802@msgid.tls.msk.ru> <4F4CD096.5000309@redhat.com> In-Reply-To: <4F4CD096.5000309@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] Consolidate reads and writes in nbd block device into one common routine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org On 28.02.2012 17:03, Paolo Bonzini wrote: > Il 28/02/2012 13:35, Michael Tokarev ha scritto: >> On 28.02.2012 15:35, Paolo Bonzini wrote: >>> Il 28/02/2012 11:24, Michael Tokarev ha scritto: >>>> This removes quite some duplicated code. >> [] >>>> +static int nbd_co_rwv(BlockDriverState *bs, int64_t sector_num, >>>> + int nb_sectors, QEMUIOVector *qiov, int iswrite) >>> >>> Call this nbd_co_rw, and please pass the whole request.type down. >> >> Originally it is readV and writeV, so why it should not be rwV ? > > It's more consistent with block.c. > >> By passing whole request.type (NBD_CMD_WRITE or NBD_CMD_WRITE|NBD_CMD_FLAG_FUA >> or NBD_CMD_READ) the condition (iswrite currently) will be larger (request.type >> != NBD_CMD_READ). Also, if someday we'll have additional flag for READ as we >> already do for write, whole thing will be even more difficult to read. > > Sure, but why should a generic function deal with NBD_CMD_FLAG_FUA? I can pass both "iswrite" and request.type here - to avoid possible complications in the area of adding more nbd-specific meanings/flags to request.type. But that becomes more ugly. [] >>> ... but thinking more about it, why don't you leave >>> nbd_co_readv_1/nbd_co_writev_1 alone, and create a nbd_split_rw function >>> that takes a function pointer? >> >> Because each of these nbd_co_*_1 does the same thing, the diff. is >> only quiv->iov vs NULL. While reading the original code it was the >> first thing I did - consolidated nbd_co_*_1 into nbd_co_* ;) > > And offset. I needed to check that non-0 offsets are fine when the iov > is NULL; it's not obvious. It isn't indeed. Both send_request and recv_reply only checks iov and ignores offset if iov is NULL. >> Actually it might be a good idea to have single bdrv->bdrv_co_readwritev >> method instead of two -- the path of each read and write jumps between >> specific read-or-write routine and common readwrite routine several >> times. > > Small amounts of duplicate code can be better than functions with many > ifs or complicated conditions. Sure thing. But when the code path is so twisted - common->specific-> common-> specific - it makes very difficult to get the bigger picture. >> I see only one correction which needs (really!) to be done - that's >> fixing the bug with offset. Do you still not agree? > > I still disagree. :) I will accept the patch with the function pointer > though. With separate nbd_co_*_1 it isn't worth the effort. When it all is in a _small_ single routine (the resulting function is actually very small), whole logic is immediately visible. In particular, the FUA bit which is set for every _part_ of write request - it wasn't visible till I integrated nbd_co_writev_1 into nbd_co_writev. Thanks, /mjt