| Message ID | 6c82b83fae72a5b5909993f0cf65a90f12efb669.1491416061.git.jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/05/2017 02:28 PM, Jeff Cody wrote: > The BDRV_O_ALLOW_RDWR flag allows / prohibits the changing of > the BDS 'read_only' state, but there are a few places where it > is ignored. In the bdrv_set_read_only() helper, make sure to > honor the flag. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/block.c b/block.c > index f60d5ea..4a61ff0 100644 > --- a/block.c > +++ b/block.c > @@ -201,6 +201,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp) > return -EINVAL; > } > > + /* Do not clear read_only if it is prohibited */ > + if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) { > + error_setg(errp, "Node '%s' is read only", > + bdrv_get_device_or_node_name(bs)); > + return -EPERM; > + } > + > bs->read_only = read_only; > return 0; > } > Conceptually straightforward. looks like this might change behavior for... RBD and vvfat, right? RBD is the subject of this series so we'll just assume that was broken and stupid. What's vvfat's story? It always set the read-only property to false regardless of what you asked for?
On 04/05/2017 02:20 PM, John Snow wrote: > Conceptually straightforward. > > looks like this might change behavior for... RBD and vvfat, right? > RBD is the subject of this series so we'll just assume that was broken > and stupid. > > What's vvfat's story? It always set the read-only property to false > regardless of what you asked for? vvfat is even stupider than that - it has its own independent property 'rw' that determines whether to allow write operations, separate from the inherited BDS readonly property.
On Wed, Apr 05, 2017 at 02:26:53PM -0500, Eric Blake wrote: > On 04/05/2017 02:20 PM, John Snow wrote: > > > Conceptually straightforward. > > > > looks like this might change behavior for... RBD and vvfat, right? > > RBD is the subject of this series so we'll just assume that was broken > > and stupid. > > Yes on RBD, and that change is intentional. > > What's vvfat's story? It always set the read-only property to false > > regardless of what you asked for? > > vvfat is even stupider than that - it has its own independent property > 'rw' that determines whether to allow write operations, separate from > the inherited BDS readonly property. > Yes, it is very odd. But if we have copy_on_read enabled, or explicitly set the block device to read-only via QAPI or -drive, I think that those should take precedence.
On Wed, Apr 05, 2017 at 08:20:28PM -0400, Jeff Cody wrote: > On Wed, Apr 05, 2017 at 02:26:53PM -0500, Eric Blake wrote: > > On 04/05/2017 02:20 PM, John Snow wrote: > > > > > Conceptually straightforward. > > > > > > looks like this might change behavior for... RBD and vvfat, right? > > > RBD is the subject of this series so we'll just assume that was broken > > > and stupid. > > > > > Yes on RBD, and that change is intentional. > > > > What's vvfat's story? It always set the read-only property to false > > > regardless of what you asked for? > > > > vvfat is even stupider than that - it has its own independent property > > 'rw' that determines whether to allow write operations, separate from > > the inherited BDS readonly property. > > > > Yes, it is very odd. But if we have copy_on_read enabled, or explicitly set > the block device to read-only via QAPI or -drive, I think that those should > take precedence. > I meant to add an example - currently, with this drive commandline: "-drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on" The drive is not read-only, but writable. That seems broken. After this patch, this ends up throwing an error now (which I think is a logical thing to do): qemu-system-x86_64: -drive format=vvfat,dir=/tmp/vvfat,rw,if=virtio,readonly=on: Node '#block238' is read only How this affects vvfat (and rbd) should be documented in the commit message, however, so I should ammend that if we keep this behavior. One other possible option is to treat the vvfat 'rw' option as meaning "enable writes, iff the block device is writable". With that interpretation, we could do something different in the above scenario: silently fail to set bs->read_only to 'true' in the vvfat driver, and keep it r/o.
diff --git a/block.c b/block.c index f60d5ea..4a61ff0 100644 --- a/block.c +++ b/block.c @@ -201,6 +201,13 @@ int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp) return -EINVAL; } + /* Do not clear read_only if it is prohibited */ + if (!read_only && !(bs->open_flags & BDRV_O_ALLOW_RDWR)) { + error_setg(errp, "Node '%s' is read only", + bdrv_get_device_or_node_name(bs)); + return -EPERM; + } + bs->read_only = read_only; return 0; }
The BDRV_O_ALLOW_RDWR flag allows / prohibits the changing of the BDS 'read_only' state, but there are a few places where it is ignored. In the bdrv_set_read_only() helper, make sure to honor the flag. Signed-off-by: Jeff Cody <jcody@redhat.com> --- block.c | 7 +++++++ 1 file changed, 7 insertions(+)