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

[Kevin Wolf <kwolf@redhat.com>]

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)

> @@ -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.

> @@ -154,6 +162,34 @@ static void QEMU_NORETURN help(void)
>      exit(EXIT_SUCCESS);
>  }
>  
> +static QemuOptsList qemu_object_opts = {
> +    .name = "object",
> +    .implied_opt_name = "qom-type",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
> +    .desc = {
> +        { }
> +    },
> +};
> +
> +static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    Error *err = NULL;
> +    OptsVisitor *ov;
> +    QDict *pdict;
> +
> +    ov = opts_visitor_new(opts);
> +    pdict = qemu_opts_to_qdict(opts, NULL);
> +
> +    user_creatable_add(pdict, opts_get_visitor(ov), &err);
> +    opts_visitor_cleanup(ov);
> +    QDECREF(pdict);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
>  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).

>      for(;;) {
> -        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?

> +            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.

> +            break;
>          }
>      }

Kevin