Re: [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8030 bytes --]

On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> Currently qemu-img allows an image filename to be passed on the
> command line, but does not have a way to set any options except
> the format eg
> 
>    qemu-img info https://127.0.0.1/images/centos7.iso
> 
> This adds a --source arg (that is mutually exclusive with a
> positional filename arg and -f arg) that accepts a full option
> string, as well as the original syntax eg
> 
>    qemu-img info --source driver=http,url=https://127.0.0.1/images,sslverify=off

Don't we also need this for destinations and their matching options, for
'compare'(-F), 'convert'(-O) [oh yikes: convert takes more than one
input file - how will we support that?], and 'rebase'(-b/-F)?

The idea is nice, but I don't think we've fully covered the design space.

/me reads ahead

Oh, you DO add a --target, but didn't spell it out in the commit
message...[1]

> ---
>  include/qemu/option.h |   1 +
>  qemu-img.c            | 474 ++++++++++++++++++++++++++++++++++++++++++--------
>  util/qemu-option.c    |   6 +
>  3 files changed, 407 insertions(+), 74 deletions(-)

And where's the documentation patches, for the man page?

> +++ b/include/qemu/option.h
> @@ -104,6 +104,7 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
>                       Error **errp);
>  
>  QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
> +QemuOpts *qemu_opts_next(QemuOpts *opts);

Should exposing this be a separate patch?

>  QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>                             int fail_if_exists, Error **errp);

While we're touching QemuOpts, should we switch fail_if_exists to bool
(separate patch)?

>  void qemu_opts_reset(QemuOptsList *list);
> diff --git a/qemu-img.c b/qemu-img.c
> index 47f0006..7a6ce81 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -38,6 +38,7 @@
>  #include "block/blockjob.h"
>  #include "block/qapi.h"
>  #include <getopt.h>
> +#include <err.h>

No. Markus is trying to get rid of this.
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03492.html
because it does not give consistent error messages (things like
timestamps, for example).

>  
>  #define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION QEMU_PKGVERSION \
>                            ", Copyright (c) 2004-2008 Fabrice Bellard\n"
> @@ -51,6 +52,8 @@ enum {
>      OPTION_OUTPUT = 256,
>      OPTION_BACKING_CHAIN = 257,
>      OPTION_OBJECT = 258,
> +    OPTION_SOURCE = 259,
> +    OPTION_TARGET = 260,

[1]...not mentioned in the commit message.


> +static BlockBackend *img_open_opts(const char *id,
> +                                   QemuOpts *opts, int flags)
> +{
> +    QDict *options;
> +    Error *local_err = NULL;
> +    char *file = NULL;
> +    BlockBackend *blk;
> +    file = g_strdup(qemu_opt_get(opts, "file"));
> +    qemu_opt_unset(opts, "file");
> +    options = qemu_opts_to_qdict(opts, NULL);
> +    blk = blk_new_open(id, file, NULL, options, flags, &local_err);
> +    if (!blk) {
> +        error_report("Could not open '%s': %s", file ? file : "",
> +                     error_get_pretty(local_err));

Markus' series adds the ability to prefix your message rather than
calling error_get_pretty().  Particularly nice if blk_new_open() starts
appending hints to the error.  It might be worth waiting for his series
to land and then rebase yours on top of his.

> @@ -1111,6 +1199,7 @@ static int img_compare(int argc, char **argv)
>          static const struct option long_options[] = {
>              {"help", no_argument, 0, 'h'},
>              {"object", required_argument, 0, OPTION_OBJECT},
> +            {"source", required_argument, 0, OPTION_SOURCE},
>              {0, 0, 0, 0}
>          };
>          c = getopt_long(argc, argv, "hf:F:T:pqs",
> @@ -1148,6 +1237,20 @@ static int img_compare(int argc, char **argv)
>                  exit(1);
>              }
>              break;
> +        case OPTION_SOURCE:
> +            if (filename2) {
> +                errx(EXIT_FAILURE, "--source can only be specified twice");

Weird. Yes, we're reading two files, but --source/--target might be
easier to explain/enforce than --source/--source...

> +            } else if (filename1) {
> +                filename2 = optarg;
> +            } else {
> +                filename1 = optarg;
> +            }
> +            opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
> +                                           optarg, true);
> +            if (!opts) {
> +                exit(1);
> +            }
> +            break;
>          }
>      }
>  
> @@ -1156,12 +1259,20 @@ static int img_compare(int argc, char **argv)
>          progress = false;
>      }
>  
> -
> -    if (optind != argc - 2) {
> -        error_exit("Expecting two image file names");
> +    if (filename1) {
> +        if (optind != argc) {
> +            error_exit("--source and filenames are mutually exclusive");
> +        }
> +        if (!filename2) {
> +            error_exit("Expecting two --source arguments");
> +        }
> +    } else {
> +        if (optind != argc - 2) {
> +            error_exit("Expecting two image file names");
> +        }
> +        filename1 = argv[optind++];
> +        filename2 = argv[optind++];
>      }

...I think you managed to enforce that there are either two --source or
two positional arguments, and nothing else is valid, but it's not typical.

> -    }
> -    bs1 = blk_bs(blk1);
> +    opts = qemu_opts_find(&qemu_source_opts, NULL);
> +    if (opts) {
> +        if (fmt1 || fmt2) {
> +            error_report("--source and -f or -F are mutuall exclusive");

s/mutuall/mutually/


> @@ -1808,20 +1949,33 @@ static int img_convert(int argc, char **argv)
>      }
>      qemu_progress_init(progress, 1.0);
>  
> -
> -    bs_n = argc - optind - 1;
> -    out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
> +    if (!bs_n) {
> +        out_filename = (argc - optind - 1) >= 1 ? argv[argc - 1] : NULL;
> +    }
>  
>      if (options && has_help_option(options)) {
>          ret = print_block_option_help(out_filename, out_fmt);
>          goto out;
>      }
>  
> -    if (bs_n < 1) {
> -        error_exit("Must specify image file name");
> +    if (bs_n) {
> +        if (argc > (optind + 1)) {

Redundant inner ().

Overall, I'm left wondering whether requiring '--source FOO' vs.
positional 'FOO', and manually enforcing mutual exclusion between the
two, is necessary, or if we could stick with positional.  But I guess
the main argument is backwards-compatibility: previously, using
'driver=file,file=/path/to/file' as a filename would try to look in a
relative directory 'driver=file,file=', whereas your proposal of always
using the new '--source' option would make it obvious that we are
expecting to parse a QemuOpts string rather than defaulting to a literal
file name.

On the other hand, the existing positional parameters have allowed
'file:file:with_weird_name' to explicitly specify that we want to use
'./file:with_weird_name' as a relative file in the current directory
(that is, the first 'file:' prefix is sufficient to avoid any
back-compat issues with any other possible change in interpretation to a
prefix), so on that grounds, I'd argue that adding --source is not
necessary, and we can just require users to write
'file:$string_that_might_now_be_QemuOpts' anywhere they used to use
'$string_that_might_now_be_QemuOpts'.

Maybe other block developers have an opinion to offer on whether the
last three patches in this series should be adding a new --source option
as mutually exclusive with positional args, vs. just adding a new
interpretation of the existing mandatory positional arguments?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]