Re: [Qemu-devel] [PATCH] block: Deprecate bdrv_set_read_only() and users

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3560 bytes --]

Am 07.11.2017 um 21:29 hat Eric Blake geschrieben:
> On 11/07/2017 11:26 AM, Kevin Wolf wrote:
> > bdrv_set_read_only() is used by some block drivers to override the
> > read-only option given by the user. This is not how read-only images
> > generally work in QEMU: Instead of second guessing what the user really
> > meant (which currently includes making an image read-only even if the
> > user didn't only use the default, but explicitly said read-only=off), we
> > should error out if we can't provide what the user requested.
> > 
> > This adds deprecation warnings to all callers of bdrv_set_read_only() so
> > that the behaviour can be corrected after the usual deprecation period.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c       |  5 +++++
> >  block/bochs.c | 13 ++++++++++---
> >  block/cloop.c | 13 ++++++++++---
> >  block/dmg.c   | 12 +++++++++---
> >  block/rbd.c   | 14 ++++++++++----
> >  block/vvfat.c |  6 +++++-
> >  6 files changed, 49 insertions(+), 14 deletions(-)
> 
> Dan pointed out the missing documentation, but for the code itself, the
> approach looks sane (especially since it was my attempt to make it worse
> by extending the idiom to NBD that triggered you to write this patch).
> 
> Other documentation: In qapi/block-core.json, @BlockdevOptions, we
> probably ought to mention under @read-only that some block drivers
> require the use of an explicit read-only.

Well, they don't only need an explicitly set option, but the important
point is that they don't work with the default value. But I can add
something to this effect.

> > +++ b/block/vvfat.c
> > @@ -1259,7 +1259,11 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> >                         "Unable to set VVFAT to 'rw' when drive is read-only");
> >              goto fail;
> >          }
> > -    } else  {
> > +    } else  if (!bdrv_is_read_only(bs)) {
> > +        error_report("Opening non-rw vvfat images without an explicit "
> > +                     "read-only=on option is deprecated. Future versions "
> > +                     "will refuse to open the image instead of "
> > +                     "automatically marking the image read-only.");
> >          /* read only is the default for safety */
> >          ret = bdrv_set_read_only(bs, true, &local_err);
> 
> Is this also a good time to deprecate vvfat's duplication of rw vs.
> read-only, and consolidate that into a single option?  No other device
> defaults to read-only, so the deprecation period is a good point to warn
> that a future version may default to read-write without an explicit
> read-only.  I guess vvfat is the only driver with a device-specific QAPI
> change (for 'rw') that might be impacted if you make that additional change.

I would love to get rid of the duplication, but there's a reason why
vvfat defaults to read-only. I think we're relatively confident that a
read-only vvfat can be safely implemented (and hopefully is), but write
support is really a clever hack that may or may not work reliably
depending on how crazy the guest OS goes.

So if we removed the 'rw' option, would we want 'read-only' to default
to true for vvfat? I'm not sure if we want to go there, it would mean
making the default value of some base BlockdevOptions depend on the
driver.

On the other hand, I'm not sure how useful 'read-only' even is apart
from the protocol layer... Should it have been driver-specific? But it's
too late for that anyway.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]