On Fri, Feb 03, 2017 at 11:32:13PM +0100, Max Reitz wrote:
> On 03.02.2017 13:02, 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.
> >
> > Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> > ---
> > qemu-img-cmds.hx | 6 +--
> > qemu-img.c | 131
> > ++++++++++++++++++++++++++++++++++++-------------------
> > qemu-img.texi | 12 ++++-
> > 3 files changed, 98 insertions(+), 51 deletions(-)
>
> Apart from what the commit message says, it also introduces that switch
> for dd, which I again don't like too much (quelle surprise), if only
> because it requires conv=nocreat and thus patch 5 to be useful.
Once again, I'll drop the 'dd' parts of this patch.
> > @@ -1918,7 +1933,7 @@ static int img_convert(int argc, char **argv)
> > bs_n = argc - optind - 1;
> > out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
> >
> > - if (options && has_help_option(options)) {
> > + if (out_fmt && options && has_help_option(options)) {
>
> "!out_fmt && options && has_help_option(options)" should probably be an
> error.
Ok.
> > @@ -1987,22 +2002,22 @@ static int img_convert(int argc, char **argv)
> > goto out;
> > }
> >
> > - /* Find driver and parse its options */
> > - drv = bdrv_find_format(out_fmt);
> > - if (!drv) {
> > - error_report("Unknown file format '%s'", out_fmt);
> > - ret = -1;
> > - goto out;
> > - }
> > + if (!skip_create) {
> > + /* Find driver and parse its options */
> > + drv = bdrv_find_format(out_fmt);
> > + if (!drv) {
> > + error_report("Unknown file format '%s'", out_fmt);
> > + ret = -1;
> > + goto out;
> > + }
> >
> > - proto_drv = bdrv_find_protocol(out_filename, true, &local_err);
> > - if (!proto_drv) {
> > - error_report_err(local_err);
> > - ret = -1;
> > - goto out;
> > - }
> > + proto_drv = bdrv_find_protocol(out_filename, true, &local_err);
> > + if (!proto_drv) {
> > + error_report_err(local_err);
> > + ret = -1;
> > + goto out;
> > + }
> >
> > - if (!skip_create) {
> > if (!drv->create_opts) {
> > error_report("Format driver '%s' does not support image
> > creation",
> > drv->format_name);
>
> Compression may be used with -n. This involves the check whether
> drv->bdrv_co_pwritev_compressed is NULL or not -- which is bad if drv is
> still NULL:
>
> $ ./qemu-img create -f qcow2 foo.qcow2 64M
> Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encryption=off
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> $ ./qemu-img convert -c -O qcow2 -n null-co:// foo.qcow2
> [1] 17179 segmentation fault (core dumped) ./qemu-img convert -c -O
> qcow2 -n null-co:// foo.qcow
>
> Therefore, you should probably only do the check whether compression is
> supported if drv is non-NULL; and if it is NULL, do the check again
> after the target image has been opened and its driver is known.
Oh, yes, that's a fun combination. I'll do that.
> > diff --git a/qemu-img.texi b/qemu-img.texi
> > index 01acfb8..bda3cc3 100644
> > --- a/qemu-img.texi
> > +++ b/qemu-img.texi
> > @@ -45,9 +45,17 @@ keys.
> >
> > @item --image-opts
> >
> > -Indicates that the @var{filename} parameter is to be interpreted as a
> > +Indicates that the source @var{filename} parameter is to be interpreted as
> > a
> > full option string, not a plain filename. This parameter is mutually
> > -exclusive with the @var{-f} and @var{-F} parameters.
> > +exclusive with the @var{-f} parameter.
>
> -F is correct, that's the one for qemu-img compare, after all (which
> takes two source filenames).
>
> > +
> > +@item --target-image-opts
> > +
> > +Indicates that the target @var{filename} parameter(s) are to be
> > interpreted a
> > +a full option string, not a plain filename. This parameter is mutually
> > +exclusive with the @var{-F} or @var{-O} parameters. It is currently
> > required
>
> -F is unrelated to a target ("output") file.
Opps, yes.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
