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

[Michael Tokarev <mjt@tls.msk.ru>]

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.

And as I mentioned, this place is the most questionable
due to its relative ugliness (but the result is much more
understandable, IMHO anyway).  This ugliness comes from
the original code, I tried to not change it much.

>> +/* defines for is_write for bdrv_*_rw_vector */
>> +#define BDRV_READ	false
>> +#define BDRV_WRITE	true
>> +
> 
> Please no, if you have to do this just change to bits.  This would have
> the advantage of passing all the flags, including COPY_ON_READ.  In some
> sense discard could be treated as a write too.

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.

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.

> I don't oppose this change completely, in fact I think adding the flags
> to co_readv/co_writev would be a good change.

The series is an RCF for the _current_ situation, which does not
pass any flags.  Which other flags do you want to pass?


>           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.

Thanks,

/mjt