"Michael S. Tsirkin" <m...@redhat.com> 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 <nsp...@redhat.com>
>
> 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.
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".
[...]
