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