Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

> On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <address@hidden> writes:
>> 
>> > On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
>> >> Instead of using the field 'readonly' of the BlockDriverState struct for 
>> >> passing the request,
>> >> pass the request in the flags parameter to the function.
>> >> 
>> >> Signed-off-by: Naphtali Sprei <address@hidden>
>> >
>> > Many changes seem to be about passing 0 to bdrv_open. This is not what
>> > the changelog says the patch does. Better split unrelated changes to a
>> > separate patch.
>> >
>> > One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
>> > this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
>> > 0.
>> 
>> BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
>> BDRV_DONT_SNAPSHOT, either.
>
> Well, this just mirros the file access macros: we have RDONLY, WRONLY
> and RDRW. I assume this similarity is just historical?

Plausible.

>> The old code can't quite devide whether BDRV_O_RDWR is a flag, or
>> whether to use bits BDRV_O_ACCESS for an access mode, with possible
>> values BDRV_O_RDONLY and BDRV_O_RDWR.  I asked Naphtali to clean this
>> up, and recommended to go with flag rather than access mode:
>> 
>>     In my opinion, any benefit in readability you might hope gain by
>>     having BDRV_O_RDONLY is outweighed by the tortuous bit twiddling you
>>     need to keep knowledge of its encoding out of its users.
>> 
>> http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg02504.html
>> 
>> [...]
>> >> @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
>> >>                  return 0;
>> >>              }
>> >>              action = SNAPSHOT_LIST;
>> >> +            bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
>> >
>> > bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need
>> > for comment then?
>> 
>> BDRV_O_RDWR is a flag, and this is the clean way to clear it.
>> 
>> "bdrv_oflags = BDRV_O_RDONLY" assumes that everything but the access
>> mode in bdrv_oflags is clear.  Tolerable, because the correctness
>> argument is fairly local, but the clean way to do it would be
>> 
>>     bdrv_oflags = (bdrv_oflags & ~ BDRV_O_ACCESS) | BDRV_O_RDONLY;
>> 
>> That's what I meant by "tortuous bit twiddling".
>> 
>> [...]
>
> Thinking about it, /* no need for RW */ comment can just go.

Agree.

>                                                               But other
> places in code just do flags = 0 maybe they should all do &=
> ~BDRV_O_RDWR?  I don't really have an opinion here but I do think this
> patch needs a better commit log (all it says "pass the request in the
> flags parameter to the function") and be split up:
> patch 1 - get rid of BDRV_O_RDONLY/BDRV_O_ACCESS
> patch 2 - pass the request in the flags parameter to the function
> patch 3 - any other fixups
>
> As it is, sometimes e.g. BDRV_O_RDWR is replaced with 0 sometimes as
> well, and it's hard to see why.

Fair enough.