Re: [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector

[Paolo Bonzini]

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