Re: [Qemu-devel] [PATCH 11/11] chardev: add pty chardev support to chardev-add (qmp)

[Eric Blake]

On 01/07/2013 06:55 AM, Gerd Hoffmann wrote:
> The ptsname is returned directly, so there is no need to
> use query-chardev to figure the pty device path.
> 
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  qapi-schema.json |    7 ++++++-
>  qemu-char.c      |   24 +++++++++++++++++++++---
>  2 files changed, 27 insertions(+), 4 deletions(-)

It would be nice to document an example with the return type in
qmp-commands.hx.  In fact, it might be worth declaring just:

{ 'union': 'ChardevReturn', 'data': { 'nodata' : 'ChardevDummy' } }

earlier in patch 4/11, so that we are consistently documenting the final
API all along, rather than changing the return type and having to
revisit earlier examples as part of this patch [but see below about an
alternate proposal to leave earlier examples untouched].

In the long run, it won't matter as long as the series is applied as a
whole - but if someone backports just part of the series, then we want
to avoid the case where the backport has different return semantics than
upstream, because they didn't pick up the change in return type from
this patch.

> +++ b/qapi-schema.json
> @@ -3054,10 +3054,15 @@
>  { 'union': 'ChardevBackend', 'data': { 'file'   : 'ChardevFile',
>                                         'port'   : 'ChardevPort',
>                                         'socket' : 'ChardevSocket',
> +                                       'pty'    : 'ChardevDummy',
>                                         'null'   : 'ChardevDummy' } }
>  
> +{ 'union': 'ChardevReturn', 'data': { 'pty'    : 'str',
> +                                      'nodata' : 'ChardevDummy' } }
> +
>  { 'command': 'chardev-add', 'data': {'id'      : 'str',
> -                                     'backend' : 'ChardevBackend' } }
> +                                     'backend' : 'ChardevBackend' },
> +  'returns' : 'ChardevReturn' }

If I'm reading it correctly, this means my earlier example for 4/11 becomes:

-> { "execute" : "chardev-add",
     "arguments" : { "id" : "foo",
                     "backend" : { "type" : "null" } } }
<- { "return": { "type" : "nodata"} }

or maybe the more verbose:

<- { "return": { "type" : "nodata", "data" : {} } }

and for this patch, we add:

-> { "execute" : "chardev-add",
     "arguments" : { "id" : "foo",
                     "backend" : { "type" : "pty", "data" : { } } }
<- { "return": { "type" : "pty", "data" : "/dev/pty0" } }

It also raises the question of whether unions even work with raw types,
or whether you have to always go through a struct, in which case you
should have used the 'String' wrapper instead of 'str', looking like:

{ 'union': 'ChardevReturn', 'data': { 'pty'    : 'String',
                                      'nodata' : 'ChardevDummy' } }
...
<- { "return": { "type" : "pty", "data" : { "str" : "/dev/pty0" } } }





But do we really need it to be that complex?  Why not just use a type
with an optional member, instead of going through a union:

{ 'type' : 'ChardevReturn', 'data': { '*pty' : 'str' } }

so that adding a null device returns the same as it always did:

{ "return": {} }

and so that adding a pty looks nicer:

{ "return": { "pty" : "/dev/pty0" } }


Libvirt will be able to cope either way, but I'd prefer a less complex
solution.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature