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

[Eric Blake <eblake@redhat.com>]

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

On 11/08/2017 04:04 AM, Kevin Wolf wrote:

> 
> 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.

Having a driver-specific default for read-only MIGHT make sense, as a
plan for something down the road (it matches current behavior, after
all, in that some drivers force read-only as their default).  I guess
now is the time to decide WHAT we want to do after the deprecation
period ends, so that we're only making an incompatible change once, and
tweak the deprecation (and resulting warning messages in the meantime)
to fit in with that plan.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]