Re: [Qemu-devel] [PATCH v4 02/10] qemu-img: add support for --object command line arg

["Daniel P. Berrange" <berrange@redhat.com>]

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 <berrange@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 :|