Re: [Qemu-block] [PATCH v7 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Daniel P. Berrange]

On Wed, May 03, 2017 at 09:50:49PM +0200, Max Reitz wrote:
> On 02.05.2017 16:47, Daniel P. Berrange wrote:
> > The '--image-opts' flag indicates whether the source filename
> > includes options. The target filename has to remain in the
> > plain filename format though, since it needs to be passed to
> > bdrv_create().  When using --skip-create though, it would be
> > possible to use image-opts syntax. This adds --target-image-opts
> > to indicate that the target filename includes options. Currently
> > this mandates use of the --skip-create flag too.
> > 
> > Reviewed-by: Fam Zheng <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> 
> Sure you want to keep this, considering that there are quite some
> changes since v5?
> 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  qemu-img-cmds.hx |  4 +--
> >  qemu-img.c       | 77 
> > +++++++++++++++++++++++++++++++++++++-------------------
> >  qemu-img.texi    | 12 +++++++--
> >  3 files changed, 63 insertions(+), 30 deletions(-)
> 
> [...]
> 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index d8fdcb1..94c8cea 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> 
> [...]
> 
> > @@ -1900,7 +1901,7 @@ static int img_convert(int argc, char **argv)
> >      char *options = NULL;
> >      Error *local_err = NULL;
> >      bool writethrough, src_writethrough, quiet = false, image_opts = false,
> > -         skip_create = false, progress = false;
> > +        skip_create = false, progress = false, tgt_image_opts = false;
> 
> Not sure about the indentation here. (I personally don't like spanning
> the declaration over multiple lines in the first place, but that's a
> different topic.) Indenting consecutive lines by four spaces is
> standard, but the indentation by five spaces had a reason.
> 
> I guess I'd personally rather keep the five-space indentation...

This change was just automatic reindent by the editor, I'll put it
back to 5.

> 
> >      int64_t ret = -EINVAL;
> >  
> >      ImgConvertState s = (ImgConvertState) {
> 
> [...]
> 
> > @@ -2047,12 +2056,22 @@ static int img_convert(int argc, char **argv)
> >          goto fail_getopt;
> >      }
> >  
> > +    if (tgt_image_opts && !skip_create) {
> > +        error_report("--target-image-opts requires use of -n flag");
> > +        goto fail_getopt;
> > +    }
> > +
> >      s.src_num = argc - optind - 1;
> >      out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
> >  
> >      if (options && has_help_option(options)) {
> > -        ret = print_block_option_help(out_filename, out_fmt);
> > -        goto fail_getopt;
> > +        if (out_fmt) {
> > +            ret = print_block_option_help(out_filename, out_fmt);
> > +            goto out;
> 
> Shouldn't this remain goto fail_getopt;?

Yes

> 
> > +        } else {
> > +            error_report("Option help requires a format be specified");
> > +            goto fail_getopt;
> > +        }
> >      }
> >  
> >      if (s.src_num < 1) {
> 
> [...]
> 
> Why did you remove the compress &&
> !out_bs->drv->bdrv_co_pwritev_compressed check? I liked it. :-(

That's a rebase / conflict resolution mistake

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|