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

[Kevin Wolf]

Am 12.07.2013 um 00:45 hat Eric Blake geschrieben:
> 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!

Glad to hear this!

> > 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 :)

Sometimes you have to remind the reader that this is just an RFC. ;-)

> > +
> > +    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 :)

Yes, thinking a bit more about this, I'd actually consider this approach
mergable now. It's kind of backwards because -drive should really be
using the new blockdev-add infrastructure instead of the other way
round, but if we're careful enough that no bad -drive features leak into
blockdev-add, something like this code might be a reasonable first step
before we refactor things.

In the end we should also probably pass around the QAPI C structs
instead of converting struct -> QDict -> QOpts -> QDict, but that
shouldn't matter for any external interfaces, so we can do this later as
well.

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

Interesting that this even works. I'll fix that, of course.

And I wholeheartedly agree about trailing commas.

> > +  } }
> > +
> > +{ '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.

In fact, I think it might be a schema-only patch at this point. :-)

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

Does it matter for you to have this immediately as a return value from
blockdev-add, or wouldn't a following query-block on this device achieve
the same?

I'm not against adding a return value when it's useful, but I don't think
we should just return arbitrary query-* results if we can't think of
anything else to return.

Kevin