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

["Daniel P. Berrange" <berrange@redhat.com>]

On Wed, Apr 26, 2017 at 09:23:25PM +0200, Max Reitz wrote:
> On 24.04.2017 11:16, Daniel P. Berrange wrote:
> > The '--image-opts' flags 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: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  qemu-img-cmds.hx |  4 +--
> >  qemu-img.c       | 86 ++++++++++++++++++++++++++++++++++++++++----------------
> >  qemu-img.texi    | 12 ++++++--
> >  3 files changed, 73 insertions(+), 29 deletions(-)
> 
> I'm afraid this will need a rebase onto block-next now (because of
> Peter's "qemu-img: simplify img_convert" patch).

Ok.

> Besides the obvious rebase conflicts, there are some less obvious things
> that may/should thus be changed:
> 
> > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> > index 8ac7822..93b50ef 100644
> > --- a/qemu-img-cmds.hx
> > +++ b/qemu-img-cmds.hx
> 
> [...]
> 
> > @@ -2090,6 +2100,12 @@ 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");
> > +        ret = -1;
> 
> Depending on whether you decide to put this below
> bdrv_parse_cache_mode() or above, you might actually no longer have to
> set ret anymore.

ok

> 
> > +        goto fail_getopt;
> > +    }
> > +
> >      /* Initialize before goto out */
> >      if (quiet) {
> >          progress = 0;
> > @@ -2100,8 +2116,14 @@ static int img_convert(int argc, char **argv)
> >      out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
> >  
> >      if (options && has_help_option(options)) {
> > -        ret = print_block_option_help(out_filename, out_fmt);
> > -        goto out;
> > +        if (out_fmt) {
> > +            ret = print_block_option_help(out_filename, out_fmt);
> > +            goto out;
> > +        } else {
> > +            error_report("Option help requires a format be specified");
> > +            ret = -1;
> 
> Since this is most likely above bdrv_parse_cache_mode() (and may thus
> end up above the previous hunk?), you probably don't have to set ret
> here either (Sorry...).
> 
> The changes from v4 look good, but the rebase will result in non-trivial
> changes, so I giving an R-b would not be of any real use, I'm afraid...

No problem, i'll respin.


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