Message ID | 1357566928-25361-12-git-send-email-kraxel@redhat.com |
---|---|
State | New |
Headers | show |
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.
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
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
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)
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(-)