Re: [Qemu-devel] [RFC PATCH 5/7] block: Accept device model name for blockdev-open/close-tray

[Kevin Wolf <kwolf@redhat.com>]

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

Am 24.06.2016 um 22:39 hat Eric Blake geschrieben:
> On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> > This is an example conversion of a QMP command that operates on the
> > BlockBackend level to accept both the device model name (which is
> > supposed to become the primary interface) and the BlockBackend name.
> > 
> > Naming suggestions for the new QMP field are welcome. The obvious one
> > would be "device", but that's already taken...
> > 
> > We'll also want QAPI to understand that exactly one of the two fields
> > must be given, so we can remove/don't have to add explicit code in the
> > command implementations to check that.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  blockdev.c           | 60 +++++++++++++++++++++++++++++++++++++++-------------
> >  qapi/block-core.json | 14 ++++++++----
> >  qmp-commands.hx      | 12 +++++++----
> >  3 files changed, 63 insertions(+), 23 deletions(-)
> > 
> 
> > +++ b/qapi/block-core.json
> > @@ -2201,7 +2201,9 @@
> >  #   to it
> >  # - if the guest device does not have an actual tray
> >  #
> > -# @device: block device name
> > +# @device:  block device name (deprecated, use @id instead)
> > +#
> > +# @id:      the name or QOM path of the guest device
> >  #
> 
> Is this something we want to rush into 2.7, or is it big enough to
> convert all the commands that we may want to wait for 2.8?  Either way,
> we'll probably want to add a '(since 2.x)' hint, and maybe come up with
> a standard way to document mutually-exclusive members given that
> Marc-Andre is trying to automate documentation from the .json file.

I would definitely want to convert all commands at the same time. But if
we know what we want, I think we can still make 2.7.

The QAPI syntax won't be ready, I guess, but we can keep the two
optional fields and just document the requirements for now, and then
clean it up in 2.8. That would be an invisible change for clients. Hm,
except introspection maybe.

> >  # @force:  #optional if false (the default), an eject request will be sent to
> >  #          the guest if it has locked the tray (and the tray will not be opened
> > @@ -2211,7 +2213,8 @@
> >  # Since: 2.5
> >  ##
> >  { 'command': 'blockdev-open-tray',
> > -  'data': { 'device': 'str',
> > +  'data': { '*device': 'str',
> > +            '*id': 'str',
> >              '*force': 'bool' } }
> >  
> 
> Your idea about having some particular QAPI shorthand for marking two
> mutually-exclusive members may indeed be worth addressing, since we're
> going to have more cases of it.  But off-hand, I don't know what syntax
> would be best, particularly if we want to keep QAPI close to JSON.

Hm, I wonder if this is closer to a union or an alternate. Maybe like
this:

{ 'alternate': 'DeviceOrID',
  'data': { 'device': 'str' ,
            'id', 'str'},
  'discrimiate-by-key': true }

{ 'command': 'blockdev-open-tray',
  'data': { 'device': 'DeviceOrID', '*force': 'bool' } }

This would accept a command like:

{ "execute": "blockdev-open-tray", "arguments": { "id": "hda" } }

And turn it into a C struct:
{
    .device = {
        .type = DEVICE_OR_ID_KIND_ID,
        .u = {
            .device = NULL,
            .id = "hda",
        }
    },
    .has_force = false,
    .force = <undefined>
}

Though that might end up not being easier to implement in the command
handlers than what we have today. But it would be a precise description
of the protocol in the schema at least.

With two strings, it would be nicer to generate only a single string
field and the type in addition, but if we want to keep it generic and
allow e.g. an alternate of two different kinds of dicts or a string, I
think we need the structure as above.

Hm...

Different dicts makes me wonder if we want to have alternate dicts
embedded in the parent (kind of like flat unions).


So yes, that looks to me as if the QAPI part isn't for 2.7...

> > +++ b/qmp-commands.hx
> > @@ -4255,7 +4255,7 @@ EQMP
> >  
> >      {
> >          .name       = "blockdev-open-tray",
> > -        .args_type  = "device:s,force:b?",
> > +        .args_type  = "device:s?,id:s?,force:b?",
> >          .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
> 
> On the bright side, once Marc-Andre's work goes in, we won't have
> qmp-commands.hx to worry about.

Oh, nice. I forgot this part at first and wondered why it didn't work.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]