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.
[...]
> diff --git a/qemu-img.c b/qemu-img.c
> index 39fcf09..dc4c6eb 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
[...]
> @@ -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.
> ret = print_block_option_help(out_filename, out_fmt);
> goto out;
> }
> @@ -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.
[...]
> @@ -4064,13 +4090,22 @@ static int img_dd(int argc, char **argv)
> arg = NULL;
> }
>
> + if (tgt_image_opts && !(dd.flags & C_NOCREAT)) {
> + error_report("--target-image-opts requires use of -n flag");
*conv=nocreat
> + goto out;
> + }
> +
> + if (!out_fmt && !tgt_image_opts) {
> + out_fmt = "raw";
> + }
> +
> if (!(dd.flags & C_IF && dd.flags & C_OF)) {
> error_report("Must specify both input and output files");
> ret = -1;
> goto out;
> }
>
> - if (optionstr && has_help_option(optionstr)) {
> + if (out_fmt && optionstr && has_help_option(optionstr)) {
Same as in img_convert().
> ret = print_block_option_help(out.filename, out_fmt);
> goto out;
> }
[...]
> @@ -4152,7 +4187,6 @@ static int img_dd(int argc, char **argv)
>
> if (!(dd.flags & C_NOCREAT)) {
> qemu_opt_set_number(opts, BLOCK_OPT_SIZE, out_size, &error_abort);
> -
> ret = bdrv_create(drv, out.filename, opts, &local_err);
> if (ret < 0) {
> error_reportf_err(local_err,
I'm not sure whether this hunk is necessary...
[...]
> 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.
Max
> +to also use the @var{-n} parameter to skip image creation. This restriction
> +will be relaxed in a future release.
>
> @item fmt
> is the disk image format. It is guessed automatically in most cases. See
> below
>
signature.asc
Description: OpenPGP digital signature
