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