Patchwork [3/3] chardev: add hotplug support.

login
register
mail settings
Submitter Gerd Hoffmann
Date Dec. 14, 2012, 1:18 p.m.
Message ID <50CB272C.5020103@redhat.com>
Download mbox | patch
Permalink /patch/206452/
State New
Headers show

Comments

Gerd Hoffmann - Dec. 14, 2012, 1:18 p.m.
Hi,

> { 'enum': 'ChardevFileMode', 'data':
>   # pty = console under Windows
>   # serial = tty under POSIX
>   [ 'file', 'pipe', 'parport', 'pty', 'serial' ] }

Hmm, why this enum?  I'd stay close to -chardev, i.e. specify the type
by backend name.

> { 'enum: 'ChardevFileSource', 'data':
>   [ 'path', 'fd' ] }

I guess I'd just create a new backend type for file descriptor passing
instead of fitting that into all the existing ones.

> { 'union': 'ChardevBackend', 'data': {

This union thing is new, isn't it?
Makes sense to use that indeed.

>   'socket': 'ChardevSocket',
>   'udp': 'UDPSocketAddress',
>   'file': 'ChardevFile',
>   'null': 'ChardevDummy',
>   'msmouse': 'ChardevDummy',
>   'braille': 'ChardevDummy',
>   'stdio': 'ChardevDummy',
>   'vc': 'ChardevVC',

I doubt we need them all hotpluggable.

cheers,
  Gerd
From 6ea61630245d8ff9f87ed56b825dcc5f8d1f6e6d Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 11 Oct 2012 14:53:00 +0200
Subject: [PATCH] chardev: add hotplug support.

This patch adds chardev_add and chardev_remove monitor commands.

chardev_add expects a backend struct filled in and creates the chardev
from that.  For now only file and tty backends are supported.

chardev_del just takes an id argument and zaps the chardev specified.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hmp-commands.hx  |   30 ++++++++++++++++++++++++++++++
 hmp.c            |   19 +++++++++++++++++++
 hmp.h            |    2 ++
 qapi-schema.json |   31 +++++++++++++++++++++++++++++++
 qemu-char.c      |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 qemu-char.h      |    2 ++
 qmp-commands.hx  |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 180 insertions(+), 0 deletions(-)
Paolo Bonzini - Dec. 14, 2012, 1:45 p.m.
Il 14/12/2012 14:18, Gerd Hoffmann ha scritto:
>   Hi,
> 
>> { 'enum': 'ChardevFileMode', 'data':
>>   # pty = console under Windows
>>   # serial = tty under POSIX
>>   [ 'file', 'pipe', 'parport', 'pty', 'serial' ] }
> 
> Hmm, why this enum?  I'd stay close to -chardev, i.e. specify the type
> by backend name.

Because...

>> { 'enum: 'ChardevFileSource', 'data':
>>   [ 'path', 'fd' ] }
> 
> I guess I'd just create a new backend type for file descriptor passing
> instead of fitting that into all the existing ones.

... are you passing a file descriptor for a pipe, a file or a
parallel/serial port?

(pty and console have no arguments, I misremembered).

>> { 'union': 'ChardevBackend', 'data': {
> 
> This union thing is new, isn't it?

Yeah, a few months old.

> Makes sense to use that indeed.
> 
>>   'socket': 'ChardevSocket',
>>   'udp': 'UDPSocketAddress',
>>   'file': 'ChardevFile',
>>   'null': 'ChardevDummy',
>>   'msmouse': 'ChardevDummy',
>>   'braille': 'ChardevDummy',
>>   'stdio': 'ChardevDummy',
>>   'vc': 'ChardevVC',
> 
> I doubt we need them all hotpluggable.

True, but:

1) I believe long term it's good to move away from QemuOpts; it's good
to provide a complete interface even if all we're doing for now is
building QemuOpts out of the struct.

2) most of them have no options and trivial anyway;

3) the complicated ones (socket, file, perhaps udp) are also the useful
ones.

Paolo
Gerd Hoffmann - Dec. 14, 2012, 2:05 p.m.
On 12/14/12 14:45, Paolo Bonzini wrote:
> Il 14/12/2012 14:18, Gerd Hoffmann ha scritto:
>>   Hi,
>>
>>> { 'enum': 'ChardevFileMode', 'data':
>>>   # pty = console under Windows
>>>   # serial = tty under POSIX
>>>   [ 'file', 'pipe', 'parport', 'pty', 'serial' ] }
>>
>> Hmm, why this enum?  I'd stay close to -chardev, i.e. specify the type
>> by backend name.
> 
> Because...
> 
>>> { 'enum: 'ChardevFileSource', 'data':
>>>   [ 'path', 'fd' ] }
>>
>> I guess I'd just create a new backend type for file descriptor passing
>> instead of fitting that into all the existing ones.
> 
> ... are you passing a file descriptor for a pipe, a file or a
> parallel/serial port?

The open function of the file-based backends basically do (1) create
file handles and (2) call qemu_chr_open_fd().  So of you already have an
fd the differences are gone.  Well, almost.  tty has an special ioctl
callback to configure line speed.

cheers,
  Gerd
Gerd Hoffmann - Dec. 14, 2012, 2:19 p.m.
Hi,

>> ... are you passing a file descriptor for a pipe, a file or a
>> parallel/serial port?
> 
> The open function of the file-based backends basically do (1) create
> file handles and (2) call qemu_chr_open_fd().  So of you already have an
> fd the differences are gone.  Well, almost.  tty has an special ioctl
> callback to configure line speed.

Also you might want to pass in a socket fd ...

So I really think a -chardev
fd,type={listening-stream-socket,connected-stream-socket,datagram-socket,tty,fd-readwrite,fd-writeonly}
(+ QMP for that) will be more useful.

cheers,
  Gerd
Paolo Bonzini - Dec. 14, 2012, 3:07 p.m.
Il 14/12/2012 15:19, Gerd Hoffmann ha scritto:
>   Hi,
> 
>>> ... are you passing a file descriptor for a pipe, a file or a
>>> parallel/serial port?
>>
>> The open function of the file-based backends basically do (1) create
>> file handles and (2) call qemu_chr_open_fd().

Right, and there's the ugliness where you can only specify an output
file, not an input.

>> So of you already have an
>> fd the differences are gone.  Well, almost.  tty has an special ioctl
>> callback to configure line speed.

What about this then:

{ 'union': 'ChardevFileSource',
  'data': {'path': 'str', 'fd': 'str'} }

{ 'type': 'ChardevFile',
  'data': {'*in': 'ChardevFileSource', '*out': 'ChardevFileSource'} }

{ 'enum': 'ChardevPortKind',
  'data': ['serial', 'parallel'] }

{ 'type': 'ChardevPort',
  'data': {'device': 'ChardevFileSource', 'type': 'ChardevPortKind'} }

{ 'type': 'ChardevSpice',
  'data': {'name': 'str', '*debug': 'bool'}}

{ 'type': 'ChardevVC',
  'data': {'*width': 'int', '*height': 'int',
           '*cols': 'int', '*rows': 'int'}}

{ 'type': 'ChardevSocket',
  'data': {'addr': 'SocketAddress', '*server': 'bool',
           '*wait': 'bool', '*nodelay': 'bool', '*telnet': 'bool'} }

{ 'type': 'ChardevUDP',
  'data': {'peer': 'SocketAddress',
           '*localhost': 'str', '*localport': 'str' } }

# For future extensibility...
{ 'ChardevDummy', 'data': {} }

{ 'union': 'ChardevBackend', 'data': {
  'socket': 'ChardevSocket',
  'winpipe': 'String',        # "pipe" for _WIN32 case
  'udp': 'ChardevUDP',
  'file': 'ChardevFile',
  'port': 'ChardevPort',
  'pty': 'ChardevDummy',
  'null': 'ChardevDummy',
  'msmouse': 'ChardevDummy',
  'braille': 'ChardevDummy',
  'stdio': 'ChardevDummy',
  'vc': 'ChardevVC',

  # Solely for HMP usage.
  'legacy': 'str'
}

> Also you might want to pass in a socket fd ...

This is already supported by socket_connect/socket_listen in
qemu-sockets.c (thus by the SocketAddress QAPI union).

Paolo

> So I really think a -chardev
> fd,type={listening-stream-socket,connected-stream-socket,datagram-socket,tty,fd-readwrite,fd-writeonly}
> (+ QMP for that) will be more useful.


> cheers,
>   Gerd
>

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 010b8c9..9a0b2eb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1485,6 +1485,36 @@  passed since 1970, i.e. unix epoch.
 ETEXI
 
     {
+        .name       = "chardev_add",
+        .args_type  = "id:s,backend:s,arg1:s",
+        .params     = "id backend arg",
+        .help       = "add chardev",
+        .mhandler.cmd = hmp_chardev_add,
+    },
+
+STEXI
+@item chardev_add id backend arg
+@findex chardev_add
+
+ETEXI
+
+    {
+        .name       = "chardev_remove",
+        .args_type  = "id:s",
+        .params     = "id",
+        .help       = "remove chardev",
+        .mhandler.cmd = hmp_chardev_remove,
+    },
+
+STEXI
+@item chardev_remove id
+@findex chardev_remove
+
+Removes the chardev @var{id}.
+
+ETEXI
+
+    {
         .name       = "info",
         .args_type  = "item:s?",
         .params     = "[subcommand]",
diff --git a/hmp.c b/hmp.c
index 180ba2b..c65f4f7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1335,3 +1335,22 @@  void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
     qmp_nbd_server_stop(&errp);
     hmp_handle_error(mon, &errp);
 }
+
+void hmp_chardev_add(Monitor *mon, const QDict *qdict)
+{
+    const char *id      = qdict_get_str(qdict, "id");
+    const char *backend = qdict_get_str(qdict, "backend");
+    const char *path    = qdict_get_str(qdict, "arg1");
+    Error *local_err = NULL;
+
+    qmp_chardev_add_path(id, path, backend, &local_err);
+    hmp_handle_error(mon, &local_err);
+}
+
+void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
+{
+    Error *local_err = NULL;
+
+    qmp_chardev_remove(qdict_get_str(qdict, "id"), &local_err);
+    hmp_handle_error(mon, &local_err);
+}
diff --git a/hmp.h b/hmp.h
index 0ab03be..e67e482 100644
--- a/hmp.h
+++ b/hmp.h
@@ -80,5 +80,7 @@  void hmp_screen_dump(Monitor *mon, const QDict *qdict);
 void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
 void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
 void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
+void hmp_chardev_add(Monitor *mon, const QDict *qdict);
+void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..7349757 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3017,3 +3017,34 @@ 
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-stop' }
+
+##
+# @chardev-add:
+#
+# Add a file chardev
+#
+# @id: the chardev's ID, must be unique
+# @backend: backend type and parameters
+#
+# Returns: Nothing on success
+#
+# Since: 1.3.0
+##
+{ 'type': 'ChardevFile', 'data': { 'path': 'str' } }
+{ 'union': 'ChardevBackend', 'data': { 'file': 'ChardevFile',
+                                       'tty': 'ChardevFile' } }
+{ 'command': 'chardev-add', 'data': {'id'      : 'str',
+                                     'backend' : 'ChardevBackend' } }
+
+##
+# @chardev-remove:
+#
+# Remove a chardev
+#
+# @id: the chardev's ID, must exist and not be in use
+#
+# Returns: Nothing on success
+#
+# Since: 1.3.0
+##
+{ 'command': 'chardev-remove', 'data': {'id': 'str'} }
diff --git a/qemu-char.c b/qemu-char.c
index 876714f..bf7fdb6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2922,3 +2922,49 @@  CharDriverState *qemu_char_get_next_serial(void)
     return serial_hds[next_serial++];
 }
 
+void qmp_chardev_add_path(const char *id, const char *path,
+                          const char *backend, Error **errp)
+{
+    QemuOpts *opts;
+
+    opts = qemu_opts_create(qemu_find_opts("chardev"), id, 1, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    qemu_opt_set(opts, "path", path);
+    qemu_opt_set(opts, "backend", backend);
+    qemu_chr_new_from_opts(opts, NULL, errp);
+}
+
+void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
+{
+    switch (backend->kind) {
+    case CHARDEV_BACKEND_KIND_FILE:
+        qmp_chardev_add_path(id, backend->file->path, "file", errp);
+        break;
+    case CHARDEV_BACKEND_KIND_TTY:
+        qmp_chardev_add_path(id, backend->tty->path, "tty", errp);
+        break;
+    case CHARDEV_BACKEND_KIND_MAX:
+        /* make gcc happy */
+        break;
+    }
+}
+
+void qmp_chardev_remove(const char *id, Error **errp)
+{
+    CharDriverState *chr;
+
+    chr = qemu_chr_find(id);
+    if (NULL == chr) {
+        error_setg(errp, "Chardev '%s' not found\n", id);
+        return;
+    }
+    if (chr->chr_can_read || chr->chr_read ||
+        chr->chr_event || chr->handler_opaque) {
+        error_setg(errp, "Chardev '%s' is busy\n", id);
+        return;
+    }
+    qemu_chr_delete(chr);
+}
diff --git a/qemu-char.h b/qemu-char.h
index f984071..18b9e99 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -239,6 +239,8 @@  void qemu_chr_info(Monitor *mon, QObject **ret_data);
 CharDriverState *qemu_chr_find(const char *name);
 
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
+void qmp_chardev_add_path(const char *id, const char *path,
+                          const char *backend, Error **errp);
 
 /* add an eventfd to the qemu devices that are polled */
 CharDriverState *qemu_chr_open_eventfd(int eventfd);
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5c692d0..10408e9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2654,3 +2654,53 @@  EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_target,
     },
+
+    {
+        .name       = "chardev-add",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_chardev_add,
+    },
+
+SQMP
+chardev-add
+----------------
+
+Add a chardev.
+
+Arguments:
+
+- "id": the chardev's ID, must be unique (json-string)
+- "backend": chardev backend type + parameters
+
+Example:
+
+-> { "execute"   : "chardev-add",
+     "arguments" : { "id"   : "foo",
+                     FIXME } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "chardev-remove",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_chardev_remove,
+    },
+
+
+SQMP
+chardev-remove
+--------------
+
+Remove a chardev.
+
+Arguments:
+
+- "id": the chardev's ID, must exist and not be in use (json-string)
+
+Example:
+
+-> { "execute": "chardev-remove", "arguments": { "id" : "foo" } }
+<- { "return": {} }
+
+EQMP