Re: [Qemu-block] [PATCH v5 14/27] qemu-img: Add "-L" option to sub commands

[Max Reitz]

On 17.05.2016 09:35, Fam Zheng wrote:
> If specified, BDRV_O_NO_LOCK flag will be set when opening the image.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  qemu-img.c | 89 
> ++++++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 72 insertions(+), 17 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 4792366..b13755b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -1206,6 +1220,7 @@ static int img_compare(int argc, char **argv)
>      qemu_progress_init(progress, 2.0);
>  
>      flags = 0;
> +    flags |= nolock ? BDRV_O_NO_LOCK : 0;

This reads weird. I'd either put this line below bdrv_parse_cache_mode()
or drop the line initializing src_flags to 0 (and make this a plain
assignment).

>      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>      if (ret < 0) {
>          error_report("Invalid source cache option: %s", cache);

[...]

> @@ -1907,6 +1926,7 @@ static int img_convert(int argc, char **argv)
>      }
>  
>      src_flags = 0;
> +    src_flags |= nolock ? BDRV_O_NO_LOCK : 0;

Same here.

Also: Should we have distinct flags for source and target for convert?
For instance, I can imagine someone wanting not to lock the source but
leave the target in default exclusive mode.

>      ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
>      if (ret < 0) {
>          error_report("Invalid source cache option: %s", src_cache);

[...]

> @@ -2881,6 +2924,7 @@ static int img_rebase(int argc, char **argv)
>      qemu_progress_print(0, 100);
>  
>      flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
> +    flags |= nolock ? BDRV_O_NO_LOCK : 0;
>      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>      if (ret < 0) {
>          error_report("Invalid cache option: %s", cache);

What about the source/base image? And again, would it make sense to have
distinct flags for source and target?

Max

signature.asc
Description: OpenPGP digital signature