Re: [Qemu-devel] [PATCH for-2.10 2/9] block: do not set BDS read_only if copy_on_read enabled

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, Apr 05, 2017 at 02:28:44PM -0400, Jeff Cody wrote:

Minor comments but:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> diff --git a/block.c b/block.c
> index 7b4c7ef..f60d5ea 100644
> --- a/block.c
> +++ b/block.c
> @@ -192,9 +192,17 @@ void path_combine(char *dest, int dest_size,
>      }
>  }
>  
> -void bdrv_set_read_only(BlockDriverState *bs, bool read_only)
> +int bdrv_set_read_only(BlockDriverState *bs, bool read_only, Error **errp)
>  {
> +    /* Do not set read_only if copy_on_read is enabled */
> +    if (bs->copy_on_read && read_only) {
> +        error_setg(errp, "Cannot set node '%s' to r/o while COW enabled",

Users might be puzzled by "COR".  The -drive option is called
"copy-on-read" so spelling it out is clearer than using an acronym.

> @@ -1173,7 +1171,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>          if (ret < 0) {
>              goto fail;
>          }
> -        bdrv_set_read_only(bs, false);
> +        ret = bdrv_set_read_only(bs, false, &local_err);
> +        if (ret < 0) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }

read_only = false by default.  There's no need to set it now that you've
moved the bdrv_set_read_only(bs, true) call.

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