"Michael S. Tsirkin" <m...@redhat.com> writes: > On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote: >> "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. > > 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.
