On Wed, Jan 27, 2016 at 02:26:53PM +0100, Kevin Wolf wrote:
> Am 26.01.2016 um 14:34 hat Daniel P. Berrange geschrieben:
> > Allow creation of user creatable object types with qemu-img
> > via a new --object command line arg. This will be used to supply
> > passwords and/or encryption keys to the various block driver
> > backends via the recently added 'secret' object type.
> >
> > # printf letmein > mypasswd.txt
> > # qemu-img info --object secret,id=sec0,file=mypasswd.txt \
> > ...other info args...
> >
> > Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
>
> > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> > index 9567774..5bb1de7 100644
> > --- a/qemu-img-cmds.hx
> > +++ b/qemu-img-cmds.hx
> > @@ -10,68 +10,68 @@ STEXI
> > ETEXI
> >
> > DEF("check", img_check,
> > - "check [-q] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache]
> > filename")
> > + "check [-q] [--object objectdef] [-f fmt] [--output=ofmt] [-r [leaks |
> > all]] [-T src_cache] filename")
> > STEXI
> > -@item check [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]]
> > [-T @var{src_cache}] @var{filename}
> > +@item check [--object objectdef] [-q] [-f @var{fmt}] [--output=@var{ofmt}]
> > [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
> > ETEXI
>
> s/objectdef/@var{objectdef}/ (in each command)
Ok, will fix.
> > @@ -94,6 +98,10 @@ static void QEMU_NORETURN help(void)
> > "\n"
> > "Command parameters:\n"
> > " 'filename' is a disk image filename\n"
> > + " 'objectdef' is a QEMU user creatable object definition. See
> > the @code{qemu(1)}\n"
> > + " manual page for a description of the object properties.
> > The common object\n"
> > + " type that it makes sense to define is the @code{secret}
> > object, which is used\n"
> > + " to supply passwords and/or encryption keys.\n"
> > " 'fmt' is the disk image format. It is guessed automatically
> > in most cases\n"
> > " 'cache' is the cache mode used to write the output disk
> > image, the valid\n"
> > " options are: 'none', 'writeback' (default, except for
> > convert), 'writethrough',\n"
>
> This one in contrast is printed literally on stdout, so using @code{} is
> not a great idea.
Opps, yes.
> > static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...)
> > {
> > int ret = 0;
> > @@ -273,9 +309,17 @@ static int img_create(int argc, char **argv)
> > char *options = NULL;
> > Error *local_err = NULL;
> > bool quiet = false;
> > + QemuOpts *opts;
>
> There are places where we declare variables only used by a specific
> option locally with a new block after the case label. This could be
> another one for which it is appropriate - it's not used after the option
> parsing any more (and it can't be used there because it may still be
> uninitialised).
Ok, will put the decl inside the switch case that uses it.
> > - c = getopt(argc, argv, "F:b:f:he6o:q");
> > + int option_index = 0;
> > + static const struct option long_options[] = {
> > + {"help", no_argument, 0, 'h'},
> > + {"object", required_argument, 0, OPTION_OBJECT},
> > + {0, 0, 0, 0}
> > + };
> > + c = getopt_long(argc, argv, "F:b:f:he6o:q",
> > + long_options, &option_index);
> > if (c == -1) {
> > break;
> > }
> > @@ -317,6 +361,13 @@ static int img_create(int argc, char **argv)
> > case 'q':
> > quiet = true;
> > break;
> > + case OPTION_OBJECT:
> > + opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
> > + optarg, true);
>
> Any reason for using qemu_add_opts() to register the opts list and then
> finding it again by name instead of just using &qemu_object_opts here?
No reason at all really, other than copying the pattern from vl.c.
I'll change to use &qemu_object_opts instead.
>
> > + if (!opts) {
> > + exit(1);
> > + }
>
> You seem to introduce a lot of exit(1) calls even where the surrounding
> code prefers return 1.
>
> Also, for other patches Eric has been asking to use EXIT_FAILURE instead
> of 1 in new code, and I think that makes sense, too.
For added fun several img_XXX commands are slightly different. I'll go
through and make each one consistent with surrounding code, either a
return, or a goto as applicable.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
