Re: [Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute

[Christoph Hellwig]

On Thu, Jan 21, 2010 at 03:19:28PM +0200, Naphtali Sprei wrote:
> 
> > 
> >  - we now normally set the read_only flag from bdrv_open2 when we do
> >    not have the O_RDWR flag set
> >  - but the block drivers also mess with it:
> >     o raw-posix superflously sets it when BDRV_O_RDWR is not in the
> >       open flags
> 
> Not sure where exactly is the issue. Can you please point the line ?

It's really just a now superflous place in the image driver that sets
the read_only flag.  Currently it's not clear who is supposed to set
the flag, we do it both from block.c and the image driver.

> >     o bochs, cloop, dmg and parallels set it unconditionally given
> >       that they do not support writing at all.  But they do not
> >       bother to reject opens without BDRV_O_RDWR
> 
> I just changed bochs and parallels not to ask for read-write.
> Should all of them test the flags for RDWR and returns failure ?

That would be most logical, but might cause regressions for existing
setups that did not bother to specify the read-only option on the
command line.  Another options might be to allow the driver to return
EROFS and the retry a read-only open for the block layer for these.

> >     o vvfat as usual is a complete mess setting and clearing it in
> >       various places
> 
> Fixed one occurance. More places ?

I mean the ->read_only flag setting and clearing.  As you've pulled
up the main place for setting it to the block layer the drivers
shouldn't mess with it anymore.

> >  - in addition to that bdrv_open2 also sets it after calling itself for
> >    the backing hd which seems superflous
> 
> Is this a problem ? I thought it's safer to mark it read-only, in case a 
> write operation requested somehow.

It's superflous, bdrv_open2 always does it based on the argument, so
no need to do it a second time for the snapshot.