Re: [Qemu-devel] [PATCH 2/7] qemu-img: add support for --object command line arg

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4795 bytes --]

On 12/22/2015 04:06 AM, Daniel P. Berrange wrote:
> Allow creation of user creatable object types with qemu-img
> via a --object command line arg. This will be used to supply

Does this read better as "a dash-dash-object", or "an object", in which
case you may have an article mismatch?  You can skirt the issue by
adding an adjective: "a new --object" works with either pronunciation of
"--object" :)

> passwords and/or encryption keys to the various block driver
> backends via the recently added 'secret' object type.
> 
>  # echo -n letmein > mypasswd.txt

'echo -n' is not portable; although it doesn't matter here, I tend to
favor 'printf letmein' for both its portability and for less typing.

>  # qemu-img info --object secret,id=sec0,file=mypasswd.txt \
>       ...other info args...
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-img-cmds.hx |  44 ++++----
>  qemu-img.c       | 300 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  qemu-img.texi    |   8 ++
>  3 files changed, 322 insertions(+), 30 deletions(-)

How will libvirt discover whether qemu-img is new enough to support this
syntax?  Then again, qemu-img isn't used quite as heavily as qemu, and
the speedups we gain by using QMP instead of -help scraping on qemu
don't matter quite as much as what we can attempt with -help scraping on
qemu-img.


> @@ -94,6 +98,11 @@ 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 \n"

Trailing whitespace on the user's terminal.

> +           "    the @code{qemu(1)} manual page for a description of the object\n"
> +           "    properties. The only object type that it makes sense to define\n"
> +           "    is the @code{secret} object, which is used to supply passwords\n"
> +           "    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"

You wrapped the text to fit in 80 source columns, but the lines below
wrapped to keep it at 80 user display columns (at the expense of longer
source text).  I'd actually lean towards the longer lines in this case,
even if we have to manually ignore checkpatch.pl.

[GNU coreutils does it like:

    printf("\
long line starting in column 0\n\
etc.");

so that you can fit much closer to 80 output characters while still
staying within 80 source columns; but I don't think we need the churn of
taking on that style]


> +static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    Error *err = NULL;
> +    char *type = NULL;
> +    char *id = NULL;
> +    void *dummy = NULL;

Drop this.

> +    OptsVisitor *ov;
> +    QDict *pdict;

Add a Visitor *v; helper variable.

> +
> +    ov = opts_visitor_new(opts);
> +    pdict = qemu_opts_to_qdict(opts, NULL);
> +
> +    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);

This conflicts with my pending patches:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03863.html

If mine go in first, you'll want this to be:

visit_start_struct(v, NULL, NULL, 0, &err);

And even if yours goes in first, you should make it look more like this,
so I don't have to fix it up after you:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03862.html

(since it looks like you copied from there anyways :)


> +
> +    user_creatable_add(type, id, pdict, opts_get_visitor(ov), &err);
> +    if (err) {
> +        goto out;
> +    }
> +    visit_end_struct(opts_get_visitor(ov), &err);

visit_end_struct() needs to be called unconditionally if
visit_start_struct() succeeded.  Again, if my series goes in first,
rebase it to look like my changes to vl.c; if yours goes in first, I'll
have to touch up your additions to match what I do elsewhere in my series.

> @@ -319,6 +397,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);
> +            if (!opts) {
> +                exit(1);

Not for this patch, but maybe someday we should switch to
exit(EXIT_FAILURE) throughout the file.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]