Re: [Qemu-devel] [RFC PATCH 11/11] [WIP] block: Implement 'blockdev-add' QMP command

[Eric Blake]

On 07/09/2013 03:53 AM, Kevin Wolf wrote:
> This is just a quick hack to test things

The rest of the series is mostly good to go, but not worth pushing until
this is flushed out.  But I love where it's headed!

> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  blockdev.c       | 32 ++++++++++++++++++++++++++++++++
>  qapi-schema.json | 29 +++++++++++++++++++++++++++++
>  qmp-commands.hx  |  6 ++++++
>  3 files changed, 67 insertions(+)


> 
> diff --git a/blockdev.c b/blockdev.c
> index e71c4ee..e3a4fb8 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1741,6 +1741,38 @@ void qmp_block_job_complete(const char *device, Error 
> **errp)
>      block_job_complete(job, errp);
>  }
>  
> +#include "qapi-visit.h"
> +#include "qapi/qmp-output-visitor.h"
> +
> +void qmp_blockdev_add(BlockOptions *options, Error **errp)
> +{
> +    QString *str;
> +    QmpOutputVisitor *ov = qmp_output_visitor_new();
> +    QObject *obj;
> +    visit_type_BlockOptions(qmp_output_get_visitor(ov),
> +                            &options, NULL, errp);
> +    obj = qmp_output_get_qobject(ov);
> +    str = qobject_to_json_pretty(obj);
> +    assert(str != NULL);
> +    fprintf(stderr, "\n>>>>>>%s\n<<<<<<\n", qstring_get_str(str));
> +    qdict_flatten(obj);
> +    str = qobject_to_json_pretty(obj);
> +    fprintf(stderr, "\n----->%s\n<-----\n", qstring_get_str(str));

Proof that it's a hack, with embedded debug messages :)

> +
> +    Error *local_err = NULL;
> +    QemuOpts *opts = qemu_opts_from_qdict(&qemu_drive_opts, 
> qobject_to_qdict(obj), &local_err);
> +    if (error_is_set(&local_err)) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +    } else {
> +        drive_init(opts, IF_NONE);
> +    }
> +
> +    qobject_decref(obj);
> +    qmp_output_visitor_cleanup(ov);
> +    QDECREF(str);
> +}

Rather elegant, now that the conversion between QMP and command line is
all hidden behind common visitors, all described by a nice QAPI
structure.  Which is really what we've been wanting :)

> +
>  static void do_qmp_query_block_jobs_one(void *opaque, BlockDriverState *bs)
>  {
>      BlockJobInfoList **prev = opaque;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a90aeb1..9f1cc8d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3702,3 +3702,32 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +
> +{ 'type': 'BlockOptionsBase',
> +  'data': { 'driver': 'str', '*read-only': 'bool' } }

Needs docs, of course; but as for the struct itself, it looks okay.

> +
> +{ 'type': 'BlockOptionsFile',
> +  'data': { 'filename': 'str' } }
> +
> +{ 'type': 'BlockOptionsRaw',
> +  'data': { 'file': 'BlockRef' } }
> +
> +{ 'type': 'BlockOptionsQcow2',
> +  'data': { '*lazy-refcounts': 'bool', 'file': 'BlockRef' } }
> +
> +{ 'union': 'BlockOptions',
> +  'base': 'BlockOptionsBase',
> +  'discriminator': 'driver',
> +  'data': {
> +      'file': 'BlockOptionsFile'
> +      'raw': 'BlockOptionsRaw'
> +      'qcow2': 'BlockOptionsQcow2'

Missing two ','; I'm surprised we haven't patched the qapi parser to be
stricter on that front (especially once that Amos' patches for
introspection demand valid JSON).  [And I sooooo wish that JSON had
followed C99's lead of allowing trailing comma]

> +  } }
> +
> +{ 'union': 'BlockRef',
> +  'discriminator': {},
> +  'data': { 'definition': 'BlockOptions'
> +            'reference': 'str' } }

Demonstrating both named and anonymous discriminated unions, which
serves as a good exercise of the earlier patches.

> +
> +{ 'command': 'blockdev-add', 'data': { 'options': 'BlockOptions' } }

Sounds nice - and seems like it should be easy enough to extend BlockRef
and/or BlockOptions to have a way to pass an fd even for backing files,
getting to (my) end goal of using fd passing to get SELinux labeling for
NFS files out of the box from libvirt.

Should this command return anything?  For example, returning a
BlockDeviceInfo (with its recursive listing of the entire backing chain)
might be useful to check that qemu's view of the world matches what the
caller passed in.  Particularly important if we are able to let the user
choose whether to pass the full chain or to just pass the top-most image
and let qemu chase down the metadata in that image to open additional
files for the rest of the chain.

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 362f0e1..fe32ae7 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3043,3 +3043,9 @@ Example:
>  <- { "return": {} }
>  
>  EQMP
> +
> +    {
> +        .name       = "blockdev-add",
> +        .args_type  = "options:q",
> +        .mhandler.cmd_new = qmp_marshal_input_blockdev_add,
> +    },
> 

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

signature.asc
Description: OpenPGP digital signature