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

Checking the status of an image when it is being used by guest is
usually useful, and there is no risk of corrupting data, so don't let
the upcoming image locking feature limit this use case.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

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;
>      }
>

On Fri, 12/02 01:52, Max Reitz wrote:
> > 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()).

Sounds good, will move it.

Fam