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
