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

[Eric Blake]

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 <address@hidden>
> ---
>  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.

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

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

signature.asc
Description: OpenPGP digital signature