Re: [Qemu-devel] [PATCH 04/14] qemu-img: Set "share-rw" flag in read-only commands

[Max Reitz <mreitz@redhat.com>]

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

On 31.10.2016 16:38, Fam Zheng wrote:
> Checking the status of an image when it is being used by guest is
> usually useful,

True for qemu-img info and maybe even qemu-img compare (and qemu-img map
is just a debugging tool, so that's fine, too), but I don't think
qemu-img check is very useful. You're not unlikely to see leaks and
maybe even errors (with lazy_refcounts=on) which don't mean anything
because they will go away once the VM is shut down.

>                 and there is no risk of corrupting data, so don't let
> the upcoming image locking feature limit this use case.

I agree that there is no harm in doing it, but for qemu-img check I also
think it isn't very useful either.

Anyway, you can keep it since I think it should not be doing anything:
The formats implementing qemu-img check should clear BDRV_O_SHARE_RW
anyway (unless overridden however that may work).

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-img.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index afcd51f..b2f4077 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -679,6 +679,10 @@ static int img_check(int argc, char **argv)
>              break;
>          }
>      }
> +
> +    if (!(flags & BDRV_O_RDWR)) {
> +        flags |= BDRV_O_SHARE_RW;
> +    }

If you want to keep this for img_check() (and I'm not going to stop you
if you do), I think it would be better to put this right in front of
img_open() to make it really clear that both are not set at the same
time (without having to look into bdrv_parse_cache_mode()).

Max

>      if (optind != argc - 1) {
>          error_exit("Expecting one image file name");
>      }
> @@ -1231,6 +1235,7 @@ static int img_compare(int argc, char **argv)
>          goto out3;
>      }
>  
> +    flags |= BDRV_O_SHARE_RW;
>      blk1 = img_open(image_opts, filename1, fmt1, flags, writethrough, quiet);
>      if (!blk1) {
>          ret = 2;
> @@ -2279,7 +2284,8 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
>          g_hash_table_insert(filenames, (gpointer)filename, NULL);
>  
>          blk = img_open(image_opts, filename, fmt,
> -                       BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false);
> +                       BDRV_O_NO_BACKING | BDRV_O_NO_IO | BDRV_O_SHARE_RW,
> +                       false, false);
>          if (!blk) {
>              goto err;
>          }
> @@ -2605,7 +2611,7 @@ static int img_map(int argc, char **argv)
>          return 1;
>      }
>  
> -    blk = img_open(image_opts, filename, fmt, 0, false, false);
> +    blk = img_open(image_opts, filename, fmt, BDRV_O_SHARE_RW, false, false);
>      if (!blk) {
>          return 1;
>      }
> 



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