[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying ima
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args |
Date: |
Tue, 22 Dec 2015 10:33:48 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-block] [Qemu-devel] [PATCH 5/7] qemu-io: allow specifying image as a set of options args, (continued)
[Qemu-block] [PATCH 6/7] qemu-nbd: allow specifying image as a set of options args, Daniel P. Berrange, 2015/12/22
[Qemu-block] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Daniel P. Berrange, 2015/12/22
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args,
Eric Blake <=
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Daniel P. Berrange, 2015/12/22
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Eric Blake, 2015/12/22
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Daniel P. Berrange, 2015/12/22
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Eric Blake, 2015/12/22
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Daniel P. Berrange, 2015/12/23
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Paolo Bonzini, 2015/12/23
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Daniel P. Berrange, 2015/12/23
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Paolo Bonzini, 2015/12/23
- Re: [Qemu-block] [Qemu-devel] [PATCH 7/7] qemu-img: allow specifying image as a set of options args, Daniel P. Berrange, 2015/12/24