[Qemu-devel] QMP interface for drive-add (or even blockdev-add)

[Qemu-devel] QMP interface for drive-add (or even blockdev-add)

[Kevin Wolf]

[ CCed qemu-devel, Stefan and Luiz ]

Am 15.05.2013 um 19:43 hat Eric Blake geschrieben:
> On 05/15/2013 09:58 AM, Kevin Wolf wrote:
> > 
> > Not really. Basically what I have in mind is the obvious mapping of the
> > command line arguments (using the downstream command here because
> > upstream doesn't even have a QMP drive_add yet):
> > 
> >     -drive format=qcow2,id=some_disk,file.driver=nbd,\
> >         file.host=localhost,lazy_refcounts=on
> > 
> > would become something like:
> > 
> >     {
> >         "execute": "__com.redhat_drive_add",
> >         "arguments": {
> >             "format": "qcow2",
> >             "id": "some_disk",
> >             "file": {
> >                 "driver": "nbd",
> >                 "host": "localhost"
> >             }
> >             "lazy_refcounts": true,
> >         }
> >     }
> 
> Yeah, that might work.  More interesting, like you say, would be
> learning how to know what options are available, and figuring out how to
> write that into a maintainable JSON spec.

Actually, there is one important point to note in this proposal: It's
that driver-specific and generic options are mixed in the same
namespace. This allows a conversion of one option after another and the
API doesn't change after each step.

On the other hand, they behave somewhat different (only converted
options can be overridden for backing files), so maybe we actually have
to switch everything within one release.

> > I'm not sure how well that works with the existing implementation of
> > QAPI etc. but it's what I think makes the most sense from a protocol
> > level perspective. And that should take precedence.
> > 
> > By the way, if you have any ideas about how to make driver-specific
> > options discoverable for libvirt, I'd be glad to hear them. Should it be
> > a QMP query command that returns a list of all drivers and what options
> > they support? Or can we somehow handle it with (dynamic) schema
> > introspection? Or would a command line option similar to '-device help'
> > be more helpful than a monitor command because you need to know the
> > valid values before starting qemu?
> 
> Introspection would work, although having a dedicated command might make
> the information easier to get at.  Libvirt would prefer to get
> information through QMP, not a command-line option.  But don't worry
> about timing - when libvirt starts, we spawn a dummy qemu just to do QMP
> queries of everything we need to know (and cache those results), so that
> when we finally spawn qemu command lines for real, we've already learned
> from the earlier QMP calls what is available on the command line. 

Okay, that's already a very helpful information to have. I believe
exposing things over QMP is more convenient for qemu as well.

> Also,
> the recent addition of query-command-line-options might be extensible -
> right now, the QAPI type CommandLineParameterType is hard-coded to one
> of four basic types; but we could extend it to list a fifth type, maybe
> named 'qapi', and where we can then expose an additional argument
> stating what QAPI type to introspect for further details.  That is,
> something like:
> 
> ->{"execute":"query-command-line-options",
>    "arguments":{"option":"drive"} }
> <-{"return":[
>    {"option":"drive", "parameters":[
>     {"name":"file", "type":"qapi", "qapi":"BlockFile"},
>     {"name":"backing", "type":"qapi", "BlockBackingFile"}
>    ]}]}

Here you're assuming a different layout than I outlined above. I guess
we'd have to directly make "parameters" the union...

> ->{"execute":"query-qapi-type", "arguments":{"type":"BlockFile"} }
> <-{"return":{"union":"BlockFile", "data":{
>     "nbd":"BlockFileNBD",
>     "qcow2":"BlockFileQcow2"} }
> ->{"execute":"query-qapi-type", "arguments":{"type":"BlockFileNBD"} }
> <-{"return":{"type","BlockFileNBD", "data":{
>     "port":"int", "host":"str", ...
>    } }
> 
> which would be enough to know that if I want to open an NBD disk, I can
> use -drive file=nbd:,file.port=1234,file.host=::1, because the 'file'
> option to -drive has a union type that I can introspect to learn what
> additional file.suboptions I can specify when using a given arm of the
> union.

...but I agree that using a union here may be workable. However, it
brings us back to the question asked in a different thread: How to
expose which drivers are actually available? I still think we need a
dynamic schema for introspection. (But how should doing this dynmically
work if QAPI maps this to a C union?)

Another thing that we'll probably want from QAPI is some kind of
inheritance, like this:

{ "type": "BlockFileBase", data: { "driver": "str", "cache": "CacheEnum",
  ... }

{ "type": "BlockFileNBD", "extends": "BlockFileBase", "data": {
    "port": "*int", "host": "str", ... }}

> In working all of this out upstream, we may decide to tweak the command
> line syntax (after all, how do I know that file.driver controls which
> arm of the union to take, and therefore which other file.FOO sub-options
> are available?), so I'm glad we disabled sub-maps in qemu 1.5 until we
> can match the QMP exposure of submaps.

Okay, let's take a step back here. The idea was more or less that you
can specify each BlockDriverState by itself in the end, like this:

{ "execute": "blockdev-add", "data": {
    "id": "my_file", "driver": "file", "filename": "test.qcow2" }}

{ "execute": "blockdev-add", "data": {
    "id": "my_qcow2", "driver": "qcow2", "file": "my_file" }}

But at least as an interface for human users this would become very
tedious, so "file" became inlined and you specify only the qcow2 image
and reference the options of the underlying raw-posix driver by using
its file option:

{
    "execute": "blockdev-add",
    "data": {
        "id": "my_qcow2",
        "driver": "qcow2",
        "file": {
            "id": "my_file",
            "driver": "file",
            "filename": "test.qcow2"
        }
    }
}

So far I've neglected the option to refer to an existing
BlockDriverState using its ID instead of specifying a new BDS inline.
I'm not even sure yet how we can make it an option to use either a string
or an embedded struct, but we'll need both options.

In this example, you can also see that "file.driver" doesn't specify
which arm of the union "my_qcow2" takes, but it's the type of "my_file"
that it influences.

Anyway, I hope this makes it a bit clearer what my thoughts behind this
system were and where I was planning to go with it.

> But I'm definitely interested in
> helping get the design off the ground; should we move this to the
> upstream qemu list?

Yes, definitely. Done. :-)

Kevin

Re: [Qemu-devel] QMP interface for drive-add (or even blockdev-add)

[Eric Blake]

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

On 05/16/2013 02:24 AM, Kevin Wolf wrote:
> Another thing that we'll probably want from QAPI is some kind of
> inheritance, like this:
> 
> { "type": "BlockFileBase", data: { "driver": "str", "cache": "CacheEnum",
>   ... }
> 
> { "type": "BlockFileNBD", "extends": "BlockFileBase", "data": {
>     "port": "*int", "host": "str", ... }}

This sounds independently useful.

> 
>> In working all of this out upstream, we may decide to tweak the command
>> line syntax (after all, how do I know that file.driver controls which
>> arm of the union to take, and therefore which other file.FOO sub-options
>> are available?), so I'm glad we disabled sub-maps in qemu 1.5 until we
>> can match the QMP exposure of submaps.
> 
> Okay, let's take a step back here. The idea was more or less that you
> can specify each BlockDriverState by itself in the end, like this:
> 
> { "execute": "blockdev-add", "data": {
>     "id": "my_file", "driver": "file", "filename": "test.qcow2" }}
> 
> { "execute": "blockdev-add", "data": {
>     "id": "my_qcow2", "driver": "qcow2", "file": "my_file" }}

So, you are using "driver" as the key for which arm of the discriminated
union supplies the remaining arguments.  That would look more like:

{ 'type': 'BlockDriverFile', 'data': {
  'name': 'str' } }
{ 'type': 'BlockDriverQcow2', 'data': {
  'file': 'str' } }
{ 'type': 'BlockDriverNBD', 'data': {
  '*port': 'int', '*host': 'str' } }

{ 'union': 'BlockDriver', 'data': {
  'file':  'BlockDriverFile',
  'qcow2': 'BlockDriverQcow2',
  'nbd':   'BlockDriverNBD' } }

{ 'command': 'blockdev-add', 'data': {
  'id': 'str', 'driver': 'BlockDriver' }

and called like:

{ "execute": "blockdev-add", "arguments": {
  "id": "my_file",
  "driver": { "type": "file", "data": { "name": "test.qcow2" } } } }
{ "execute": "blockdev-add", "arguments": {
  "id": "my_qcow2",
  "driver": { "type": "qcow2", "data": { "file": "my_file" } } } }

Dynamic introspection on 'BlockDriver' would let us know what 'type'
fields we can supply, and the type field then determines what we can use
in the 'data' dictionary.

While QMP remains quite nested to make the use of 'union' easy to parse
and introspect, simplifying things into a flatter namespace for the
command line still makes sense:

-drive id=my_file,driver=file,driver.name=test.qcow2
-drive id=my_qcow2,driver=qcow2,driver.file=my_file

seems legible enough and still something that the command line parser
can explode back into the fully-structured QMP command.

> 
> But at least as an interface for human users this would become very
> tedious, so "file" became inlined and you specify only the qcow2 image
> and reference the options of the underlying raw-posix driver by using
> its file option:
> 
> {
>     "execute": "blockdev-add",
>     "data": {
>         "id": "my_qcow2",
>         "driver": "qcow2",
>         "file": {
>             "id": "my_file",
>             "driver": "file",
>             "filename": "test.qcow2"
>         }
>     }
> }

So we want nesting.  Still might be possible, by tweaking the types I
gave above:

{ 'type': 'BlockDriverFile', 'data': {
  'name': 'str' } }
{ 'union': 'BlockDriverQcow2', 'data': {
  'file': 'str',
  'block': 'BlockDriverNested' } }
{ 'type': 'BlockDriverNBD', 'data': {
  '*port': 'int', '*host': 'str' } }
{ 'type': 'BlockDriverNested', 'data': {
  'id': 'str', 'driver': 'BlockDriver' } }

{ 'union': 'BlockDriver', 'data': {
  'file':  'BlockDriverFile',
  'qcow2': 'BlockDriverQcow2',
  'nbd':   'BlockDriverNBD' } }

{ 'command': 'blockdev-add', 'data': {
  'id': 'str', 'driver': 'BlockDriver' }

and called like this in longhand:

{ "execute": "blockdev-add", "arguments": {
  "id": "my_file",
  "driver": { "type": "file", "data": { "name": "test.qcow2" } } } }
{ "execute": "blockdev-add", "arguments": {
  "id": "my_qcow2",
  "driver": { "type": "qcow2", "data": {
     "type": "file", "data": "my_file" } } } }

on the command line as:
-drive id=my_file,driver=file,driver.name=test.qcow2
-drive id=my_qcow2,driver=qcow2,driver.file=my_file

or this in shorthand:

{ "execute": "blockdev-add", "arguments": {
  "id": "my_qcow2",
  "driver": { "type": "qcow2", "data": { "type": "block", "data": {
     "id": "my_file", "driver": {
        "type": "file", "data": { "name": "test.qcow2" } } } } } } }

on the command line as:
-drive id=my_qcow2,driver=qcow2,driver.block.id=my_file,\
driver.block.driver=file, driver.block.name=test.qcow2



Hmm, looking at that QMP, it starts to nest rather deeply, I see a
couple of possibilities for less QMP syntax:

Right now, the qapi-schema.json file requires discriminated unions that
are a dictionary of exactly two members: a 'type' member that specifies
which arm of the union, and a 'data' member that specifies a JSON object
of all remaining data (and I don't even know if we can use 'str', or if
we have to use the 'String' wrapper, in the above attempt to make
BlockDriverQcow2.file be a string).  Your idea of inheritance means we
could design an inherited union - by inheriting from a base class, the
union gains all the dictionary members of the base class plus the
additional 'type' and 'data' member.  Something like:

{ 'type': BlockDriverBase', 'data': {
  'id': 'str' }

{ 'type': 'BlockDriverFile', 'data': {
  'name': 'str' } }
{ 'union': 'BlockDriverQcow2', 'data': {
  'file': 'str',
  'block': 'BlockDriver' } }
{ 'type': 'BlockDriverNBD', 'data': {
  '*port': 'int', '*host': 'str' } }

{ 'union': 'BlockDriver', 'extends': 'BlockDriverBase', 'data': {
  'file':  'BlockDriverFile',
  'qcow2': 'BlockDriverQcow2',
  'nbd':   'BlockDriverNBD' } }

{ 'command': 'blockdev-add', 'data': {
  'driver': 'BlockDriver' }

and called like:

{ "execute": "blockdev-add", "arguments": {
  "driver": { "id": "my_file", "type": "file",
              "data": { "name": "test.qcow2" } } } }
{ "execute": "blockdev-add", "arguments": {
  "driver": { "id": "my_qcow2", "type": "qcow2",
              "data": { "type": "file", "data": "my_file" } } } }

or in shorthand:

{ "execute": "blockdev-add", "arguments": {
  "driver": { "id": "my_qcow2", "type": "qcow2",
              "data": { "type": "block", "data": {
                        "id": "my_file", "type": "file",
                        "data": { "name": "test.qcow2" } } } } } }


Next, I've noticed that qapi-schema currently always specifies a
dictionary object for 'data' when defining a 'command', which requires
another level of dictionary nesting for using a union.  But since a
union is always a dictionary, what if we modify the generator to
directly allow a union type for 'data' rather than the usual object
notation?  Likewise, what if we tweak the QMP parser to so that a
metasyntax of 'name':'uniontype' allows a shorthand of 'foo':{...}
anywhere we currently have to use 'name':{'type':'foo','data':{...}}
longhand.  (That is, anywhere the parser expects a union, if the
dictionary is missing the 'type' and 'data' fields but has an unknown
'name'/'value' pair, then reconstruct a 'type' and 'data' field
accordingly).  Observe:

{ 'type': BlockDriverBase', 'data': {
  'id': 'str' }

{ 'type': 'BlockDriverFile', 'data': {
  'name': 'str' } }
{ 'union': 'BlockDriverQcow2', 'data': {
  'file': 'str',
  'block': 'BlockDriver' } }
{ 'type': 'BlockDriverNBD', 'data': {
  '*port': 'int', '*host': 'str' } }

{ 'union': 'BlockDriver', 'extends': 'BlockDriverBase', 'data': {
  'file':  'BlockDriverFile',
  'qcow2': 'BlockDriverQcow2',
  'nbd':   'BlockDriverNBD' } }

{ 'command': 'blockdev-add', 'data': 'BlockDriver' }

and called like:

{ "execute": "blockdev-add", "arguments": {
  "id": "my_file", "file": { "name": "test.qcow2" } } }
{ "execute": "blockdev-add", "arguments": {
  "id": "my_qcow2", "qcow2": { "file": "my_file" } } } }

or with nesting:

{ "execute": "blockdev-add", "arguments": {
  "id": "my_qcow2", "qcow2": { "block": {
     "id": "my_file", "file": { "name": "test.qcow2" } } } } }

and where the parser treats it identically with the longer:

{ "execute": "blockdev-add", "arguments": {
  "id": "my_qcow2", "type": "qcow2",
  "data": { "type": "block", "data": {
     "id": "my_file", "type": "file",
     "data": { "name": "test.qcow2" } } } } }


Still slightly more verbose than your proposal (I had to add a "file" or
"block" discriminator to control whether qcow2 referred to its
underlying block device by name or by inline nested struct), but seems
like it might be easier to code into current qapi.


> 
> So far I've neglected the option to refer to an existing
> BlockDriverState using its ID instead of specifying a new BDS inline.

I've covered that - see how I made BlockDriverQcow2 a union, which was
discriminated either by type=file data=name, or type=block data=nested
dictionary.

> I'm not even sure yet how we can make it an option to use either a string
> or an embedded struct, but we'll need both options.

My ideas above fit it into the existing framework by using nested unions.

> 
> In this example, you can also see that "file.driver" doesn't specify
> which arm of the union "my_qcow2" takes, but it's the type of "my_file"
> that it influences.
> 
> Anyway, I hope this makes it a bit clearer what my thoughts behind this
> system were and where I was planning to go with it.

And I hope my ideas also help in coming up with a usable design and/or
improvements to how we generate code based on qapi JSON callouts.

-- 
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: 621 bytes --]

Re: [Qemu-devel] QMP interface for drive-add (or even blockdev-add)

[Kevin Wolf]

Am 16.05.2013 um 21:05 hat Eric Blake geschrieben:
> On 05/16/2013 02:24 AM, Kevin Wolf wrote:
> > Okay, let's take a step back here. The idea was more or less that you
> > can specify each BlockDriverState by itself in the end, like this:
> > 
> > { "execute": "blockdev-add", "data": {
> >     "id": "my_file", "driver": "file", "filename": "test.qcow2" }}
> > 
> > { "execute": "blockdev-add", "data": {
> >     "id": "my_qcow2", "driver": "qcow2", "file": "my_file" }}
> 
> So, you are using "driver" as the key for which arm of the discriminated
> union supplies the remaining arguments.  That would look more like:
> 
> { 'type': 'BlockDriverFile', 'data': {
>   'name': 'str' } }
> { 'type': 'BlockDriverQcow2', 'data': {
>   'file': 'str' } }
> { 'type': 'BlockDriverNBD', 'data': {
>   '*port': 'int', '*host': 'str' } }
> 
> { 'union': 'BlockDriver', 'data': {
>   'file':  'BlockDriverFile',
>   'qcow2': 'BlockDriverQcow2',
>   'nbd':   'BlockDriverNBD' } }
> 
> { 'command': 'blockdev-add', 'data': {
>   'id': 'str', 'driver': 'BlockDriver' }
> 
> and called like:
> 
> { "execute": "blockdev-add", "arguments": {
>   "id": "my_file",
>   "driver": { "type": "file", "data": { "name": "test.qcow2" } } } }
> { "execute": "blockdev-add", "arguments": {
>   "id": "my_qcow2",
>   "driver": { "type": "qcow2", "data": { "file": "my_file" } } } }
> 
> Dynamic introspection on 'BlockDriver' would let us know what 'type'
> fields we can supply, and the type field then determines what we can use
> in the 'data' dictionary.

There is one central difference between your version and my proposal: I
have a single namespace for both common and driver-specific options,
whereas you split off the driver-specific options into a nested struct.
It contributes a bit to the excessive nesting, but okay.

> While QMP remains quite nested to make the use of 'union' easy to parse
> and introspect, simplifying things into a flatter namespace for the
> command line still makes sense:
> 
> -drive id=my_file,driver=file,driver.name=test.qcow2
> -drive id=my_qcow2,driver=qcow2,driver.file=my_file
> 
> seems legible enough and still something that the command line parser
> can explode back into the fully-structured QMP command.

On the command line, I don't think we should distinguish two different
types of options and require a lot of typing for the repeated 'driver.'

But the other reason is that we want to be able to move the
implementation from the generic block layer to the block drivers, or
vice versa, and if we have to specify 'driver' for one but not for the
other, it has become part of the API and we can't move it any more. This
argument is valid for QMP as well.

> > But at least as an interface for human users this would become very
> > tedious, so "file" became inlined and you specify only the qcow2 image
> > and reference the options of the underlying raw-posix driver by using
> > its file option:
> > 
> > {
> >     "execute": "blockdev-add",
> >     "data": {
> >         "id": "my_qcow2",
> >         "driver": "qcow2",
> >         "file": {
> >             "id": "my_file",
> >             "driver": "file",
> >             "filename": "test.qcow2"
> >         }
> >     }
> > }
> 
> So we want nesting.  Still might be possible, by tweaking the types I
> gave above:
> 
> { 'type': 'BlockDriverFile', 'data': {
>   'name': 'str' } }
> { 'union': 'BlockDriverQcow2', 'data': {
>   'file': 'str',
>   'block': 'BlockDriverNested' } }
> { 'type': 'BlockDriverNBD', 'data': {
>   '*port': 'int', '*host': 'str' } }
> { 'type': 'BlockDriverNested', 'data': {
>   'id': 'str', 'driver': 'BlockDriver' } }
> 
> { 'union': 'BlockDriver', 'data': {
>   'file':  'BlockDriverFile',
>   'qcow2': 'BlockDriverQcow2',
>   'nbd':   'BlockDriverNBD' } }
> 
> { 'command': 'blockdev-add', 'data': {
>   'id': 'str', 'driver': 'BlockDriver' }
> 
> and called like this in longhand:
> 
> { "execute": "blockdev-add", "arguments": {
>   "id": "my_file",
>   "driver": { "type": "file", "data": { "name": "test.qcow2" } } } }
> { "execute": "blockdev-add", "arguments": {
>   "id": "my_qcow2",
>   "driver": { "type": "qcow2", "data": {
>      "type": "file", "data": "my_file" } } } }
> 
> on the command line as:
> -drive id=my_file,driver=file,driver.name=test.qcow2
> -drive id=my_qcow2,driver=qcow2,driver.file=my_file
> 
> or this in shorthand:
> 
> { "execute": "blockdev-add", "arguments": {
>   "id": "my_qcow2",
>   "driver": { "type": "qcow2", "data": { "type": "block", "data": {
>      "id": "my_file", "driver": {
>         "type": "file", "data": { "name": "test.qcow2" } } } } } } }
> 
> on the command line as:
> -drive id=my_qcow2,driver=qcow2,driver.block.id=my_file,\
> driver.block.driver=file, driver.block.name=test.qcow2

driver.block.driver.name you mean, right? ;-)

And actually using qcow2 directly as the union for str and
BlockDriverNested isn't possible, we need to nest even deeper: One
reason is that qcow2 has more options than just the image file, the
other one is that this is obviously not a qcow2-specific thing but
applies to every BlockDriverState reference in the whole schema (qcow2
currently has two of them: the image file and optionally a backing file).

> Hmm, looking at that QMP, it starts to nest rather deeply, I see a
> couple of possibilities for less QMP syntax:
> 
> Right now, the qapi-schema.json file requires discriminated unions that
> are a dictionary of exactly two members: a 'type' member that specifies
> which arm of the union, and a 'data' member that specifies a JSON object
> of all remaining data (and I don't even know if we can use 'str', or if
> we have to use the 'String' wrapper, in the above attempt to make
> BlockDriverQcow2.file be a string).

A quick compile test suggests that using 'str' works.

> Your idea of inheritance means we
> could design an inherited union - by inheriting from a base class, the
> union gains all the dictionary members of the base class plus the
> additional 'type' and 'data' member.  Something like:
> 
> { 'type': BlockDriverBase', 'data': {
>   'id': 'str' }
> 
> { 'type': 'BlockDriverFile', 'data': {
>   'name': 'str' } }
> { 'union': 'BlockDriverQcow2', 'data': {
>   'file': 'str',
>   'block': 'BlockDriver' } }
> { 'type': 'BlockDriverNBD', 'data': {
>   '*port': 'int', '*host': 'str' } }
> 
> { 'union': 'BlockDriver', 'extends': 'BlockDriverBase', 'data': {
>   'file':  'BlockDriverFile',
>   'qcow2': 'BlockDriverQcow2',
>   'nbd':   'BlockDriverNBD' } }
> 
> { 'command': 'blockdev-add', 'data': {
>   'driver': 'BlockDriver' }
> 
> and called like:
> 
> { "execute": "blockdev-add", "arguments": {
>   "driver": { "id": "my_file", "type": "file",
>               "data": { "name": "test.qcow2" } } } }
> { "execute": "blockdev-add", "arguments": {
>   "driver": { "id": "my_qcow2", "type": "qcow2",
>               "data": { "type": "file", "data": "my_file" } } } }
> 
> or in shorthand:
> 
> { "execute": "blockdev-add", "arguments": {
>   "driver": { "id": "my_qcow2", "type": "qcow2",
>               "data": { "type": "block", "data": {
>                         "id": "my_file", "type": "file",
>                         "data": { "name": "test.qcow2" } } } } } }

I believe the easier way would be to make the discriminator part of the
union, like this:

{ 'type': 'BlockDriverFile', 'data': {
   'name': 'str' } }
{ 'type': 'BlockDriverQcow2', 'data': {
   'file': 'BlockDeviceRef' } }
{ 'union': 'BlockDriver', 'discriminator': 'driver', 'data': {
   'file':  'BlockDriverFile',
   'qcow2': 'BlockDriverQcow2',
   'nbd':   'BlockDriverNBD' } }

{ 'command': 'blockdev-add', 'data': {
  'id': 'str', 'driver': 'BlockDriver' }

And then call it as:

{ "execute": "blockdev-add", "arguments": {
    "id": "my_file", "driver": {
        "driver": "file", "filename": "test.qcow2" } } }


Or maybe it's even possible to combine both to end up with:

{ 'union': 'BlockDriver', 'discriminator': 'driver',
    'extends': 'BlockDriverBase', 'data': { ... } }

which would be called like:

{ "execute": "blockdev-add", "arguments": {
    "id": "my_file", "driver": "file", "filename": "test.qcow2" } }

> Next, I've noticed that qapi-schema currently always specifies a
> dictionary object for 'data' when defining a 'command', which requires
> another level of dictionary nesting for using a union.  But since a
> union is always a dictionary, what if we modify the generator to
> directly allow a union type for 'data' rather than the usual object
> notation?

Sounds useful. Same thing for referencing a struct type instead of
declaring a new one inline. Especially transaction operations and the
corresponding standalone commands could probably share their options
most of the time.

> Likewise, what if we tweak the QMP parser to so that a
> metasyntax of 'name':'uniontype' allows a shorthand of 'foo':{...}
> anywhere we currently have to use 'name':{'type':'foo','data':{...}}
> longhand.  (That is, anywhere the parser expects a union, if the
> dictionary is missing the 'type' and 'data' fields but has an unknown
> 'name'/'value' pair, then reconstruct a 'type' and 'data' field
> accordingly).  Observe:
> 
> { 'type': BlockDriverBase', 'data': {
>   'id': 'str' }
> 
> { 'type': 'BlockDriverFile', 'data': {
>   'name': 'str' } }
> { 'union': 'BlockDriverQcow2', 'data': {
>   'file': 'str',
>   'block': 'BlockDriver' } }
> { 'type': 'BlockDriverNBD', 'data': {
>   '*port': 'int', '*host': 'str' } }
> 
> { 'union': 'BlockDriver', 'extends': 'BlockDriverBase', 'data': {
>   'file':  'BlockDriverFile',
>   'qcow2': 'BlockDriverQcow2',
>   'nbd':   'BlockDriverNBD' } }
> 
> { 'command': 'blockdev-add', 'data': 'BlockDriver' }
> 
> and called like:
> 
> { "execute": "blockdev-add", "arguments": {
>   "id": "my_file", "file": { "name": "test.qcow2" } } }
> { "execute": "blockdev-add", "arguments": {
>   "id": "my_qcow2", "qcow2": { "file": "my_file" } } } }

If we wanted to keep the nesting for driver-specific options (which I,
as I said above, don't want), this would look rather nice. But I think
in order to keep both on the same level, using both 'discriminator' and
'extends' in a union definition would work better.

> or with nesting:
> 
> { "execute": "blockdev-add", "arguments": {
>   "id": "my_qcow2", "qcow2": { "block": {
>      "id": "my_file", "file": { "name": "test.qcow2" } } } } }
> 
> and where the parser treats it identically with the longer:
> 
> { "execute": "blockdev-add", "arguments": {
>   "id": "my_qcow2", "type": "qcow2",
>   "data": { "type": "block", "data": {
>      "id": "my_file", "type": "file",
>      "data": { "name": "test.qcow2" } } } } }
> 
> 
> Still slightly more verbose than your proposal (I had to add a "file" or
> "block" discriminator to control whether qcow2 referred to its
> underlying block device by name or by inline nested struct), but seems
> like it might be easier to code into current qapi.

Hm, yes, we haven't got rid of that part yet, and I'm not sure how easy
it would be.

Maybe we should always use ID references in QMP and allow convenient
nesting only on the command line. Or somehow insert the "block"
boilerplate automagically when converting from the command line to QMP.


The other thing that I'm not sure about is whether we should teach QAPI
to parse certain data structures just into QDicts instead of C structs,
or if dealing with the big unions inside the block layer actually makes
sense.

Kevin

Re: [Qemu-devel] QMP interface for drive-add (or even blockdev-add)

[Stefan Hajnoczi]

On Wed, May 22, 2013 at 03:53:05PM +0200, Kevin Wolf wrote:
> Am 16.05.2013 um 21:05 hat Eric Blake geschrieben:
> > On 05/16/2013 02:24 AM, Kevin Wolf wrote:
> The other thing that I'm not sure about is whether we should teach QAPI
> to parse certain data structures just into QDicts instead of C structs,
> or if dealing with the big unions inside the block layer actually makes
> sense.

This is an interesting question.  It's very convenient from the code
side - we don't have to worry about laying down a schema.

However, the point of QAPI is to offer that schema that allows for us to
reason about things like compatibility (hard to sneak in a patch that
modifies the schema, easy to sneak in a patch that modifies block driver
parameter code) and eliminates the boilerplate of type-checking/basic
input validation.

Even if it requires some effort, I think we should avoid tunneling
schema-less data over QAPI.

Stefan

Re: [Qemu-devel] QMP interface for drive-add (or even blockdev-add)

[Kevin Wolf]

Am 23.05.2013 um 13:57 hat Stefan Hajnoczi geschrieben:
> On Wed, May 22, 2013 at 03:53:05PM +0200, Kevin Wolf wrote:
> > Am 16.05.2013 um 21:05 hat Eric Blake geschrieben:
> > > On 05/16/2013 02:24 AM, Kevin Wolf wrote:
> > The other thing that I'm not sure about is whether we should teach QAPI
> > to parse certain data structures just into QDicts instead of C structs,
> > or if dealing with the big unions inside the block layer actually makes
> > sense.
> 
> This is an interesting question.  It's very convenient from the code
> side - we don't have to worry about laying down a schema.
> 
> However, the point of QAPI is to offer that schema that allows for us to
> reason about things like compatibility (hard to sneak in a patch that
> modifies the schema, easy to sneak in a patch that modifies block driver
> parameter code) and eliminates the boilerplate of type-checking/basic
> input validation.
> 
> Even if it requires some effort, I think we should avoid tunneling
> schema-less data over QAPI.

Note that I'm talking _only_ about the C side here. Everything that goes
through QMP is an external API and must described by a schema, I fully
agree there.

The question is whether QAPI must, after validating the input against
the schema, parse it into C structs or whether it should be able to fill
QDicts and pass those around.

Maybe it's just an unjustified feeling, but a C union of the option
structs for all image formats feels very ugly for me, whereas I think
a union is perfectly fine in the JSON schema.

Kevin

Re: [Qemu-devel] QMP interface for drive-add (or even blockdev-add)

[Stefan Hajnoczi]

On Thu, May 23, 2013 at 4:41 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 23.05.2013 um 13:57 hat Stefan Hajnoczi geschrieben:
>> On Wed, May 22, 2013 at 03:53:05PM +0200, Kevin Wolf wrote:
>> > Am 16.05.2013 um 21:05 hat Eric Blake geschrieben:
>> > > On 05/16/2013 02:24 AM, Kevin Wolf wrote:
>> > The other thing that I'm not sure about is whether we should teach QAPI
>> > to parse certain data structures just into QDicts instead of C structs,
>> > or if dealing with the big unions inside the block layer actually makes
>> > sense.
>>
>> This is an interesting question.  It's very convenient from the code
>> side - we don't have to worry about laying down a schema.
>>
>> However, the point of QAPI is to offer that schema that allows for us to
>> reason about things like compatibility (hard to sneak in a patch that
>> modifies the schema, easy to sneak in a patch that modifies block driver
>> parameter code) and eliminates the boilerplate of type-checking/basic
>> input validation.
>>
>> Even if it requires some effort, I think we should avoid tunneling
>> schema-less data over QAPI.
>
> Note that I'm talking _only_ about the C side here. Everything that goes
> through QMP is an external API and must described by a schema, I fully
> agree there.
>
> The question is whether QAPI must, after validating the input against
> the schema, parse it into C structs or whether it should be able to fill
> QDicts and pass those around.

I see.  In that case using a QDict seems fine.

Stefan