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 <kw...@redhat.com>
> > ---
> > 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
