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

[Fam Zheng <famz@redhat.com>]

On Tue, 05/24 20:06, Max Reitz wrote:
> 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 <famz@redhat.com>
> > ---
> >  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.

OK, will drop the dead assignment above.

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

"-L" is a shorthand flag, for finer control (shared locking mode and separate
modes for src and dest), I would recommend using --image-opts then.

Fam