Patchwork [11/11] chardev: add pty chardev support to chardev-add (qmp)

login
register
mail settings
Submitter Gerd Hoffmann
Date Jan. 7, 2013, 1:55 p.m.
Message ID <1357566928-25361-12-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/209932/
State New
Headers show

Comments

Gerd Hoffmann - Jan. 7, 2013, 1:55 p.m.
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 <kraxel@redhat.com>
---
 qapi-schema.json |    7 ++++++-
 qemu-char.c      |   24 +++++++++++++++++++++---
 2 files changed, 27 insertions(+), 4 deletions(-)
Eric Blake - Jan. 9, 2013, 5:37 p.m.
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 <kraxel@redhat.com>
> ---
>  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.
Gerd Hoffmann - Jan. 10, 2013, 9:14 a.m.
Hi,

> 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' } }

Yes, should do, we don't have to pass back the chardev type here.

thanks,
  Gerd
Paolo Bonzini - Jan. 10, 2013, 10:42 a.m.
Il 09/01/2013 18:37, Eric Blake ha scritto:
> 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" } } }

They do work with raw types.

If it is conceivable to add more data in the future, however, it's
better to use the wrapped types.

Paolo

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index 39e0ab4..63c61c4 100644
--- a/qapi-schema.json
+++ 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' }
 
 ##
 # @chardev-remove:
diff --git a/qemu-char.c b/qemu-char.c
index 01d65e2..abce95f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3129,9 +3129,13 @@  static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
                                    is_telnet, is_waitconnect, errp);
 }
 
-void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
+ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
+                               Error **errp)
 {
-    CharDriverState *chr;
+    ChardevReturn *ret = g_new0(ChardevReturn, 1);
+    CharDriverState *chr = NULL;
+
+    ret->kind = CHARDEV_RETURN_KIND_NODATA;
 
     switch (backend->kind) {
     case CHARDEV_BACKEND_KIND_FILE:
@@ -3143,12 +3147,25 @@  void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
     case CHARDEV_BACKEND_KIND_SOCKET:
         chr = qmp_chardev_open_socket(backend->socket, errp);
         break;
+#ifdef HAVE_CHARDEV_TTY
+    case CHARDEV_BACKEND_KIND_PTY:
+    {
+        /* qemu_chr_open_pty sets "path" in opts */
+        QemuOpts *opts;
+        opts = qemu_opts_create_nofail(qemu_find_opts("chardev"));
+        chr = qemu_chr_open_pty(opts);
+        ret->kind = CHARDEV_RETURN_KIND_PTY;
+        ret->pty = g_strdup(qemu_opt_get(opts, "path"));
+        qemu_opts_del(opts);
+        break;
+    }
+#endif
     case CHARDEV_BACKEND_KIND_NULL:
         chr = qemu_chr_open_null(NULL);
         break;
     default:
         error_setg(errp, "unknown chardev backend (%d)", backend->kind);
-        return;
+        break;
     }
 
     if (chr == NULL && !error_is_set(errp)) {
@@ -3159,6 +3176,7 @@  void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
         chr->avail_connections = 1;
         QTAILQ_INSERT_TAIL(&chardevs, chr, next);
     }
+    return ret;
 }
 
 void qmp_chardev_remove(const char *id, Error **errp)