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