[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related
From: |
Michael S. Tsirkin |
Subject: |
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. |
Date: |
Mon, 18 Jan 2010 12:48:17 +0200 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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?
> 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. 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.
--
MST
- [Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute, Naphtali Sprei, 2010/01/17
- [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., Michael S. Tsirkin, 2010/01/17
- 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, 2010/01/18
- 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.,
Michael S. Tsirkin <=
- 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, 2010/01/18
- 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., Jamie Lokier, 2010/01/19
- 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, 2010/01/20
- 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., Michael S. Tsirkin, 2010/01/20
- 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, 2010/01/20
- 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., Michael S. Tsirkin, 2010/01/20
- 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, 2010/01/20
- 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., Jamie Lokier, 2010/01/20
- [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., Naphtali Sprei, 2010/01/18
Re: [Qemu-devel] [PATCH v2 1/4] Make CDROM a read-only drive, Jamie Lokier, 2010/01/19