All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH RESENT 0/3] chardev hotplug patch series
@ 2012-12-14  9:38 Gerd Hoffmann
  2012-12-14  9:38 ` [Qemu-devel] [PATCH 1/3] chardev: add error reporting for qemu_chr_new_from_opts Gerd Hoffmann
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Gerd Hoffmann @ 2012-12-14  9:38 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann

  Hi,

Got stuck in discussions & 1.3 freeze.  Resending series to resume merge
effort, almost unmodified, only rebased & trivial conflicts resolved.

please review & comment,
  Gerd

Gerd Hoffmann (3):
  chardev: add error reporting for qemu_chr_new_from_opts
  chardev: fix QemuOpts lifecycle
  chardev: add hotplug support.

 hmp-commands.hx  |   44 +++++++++++++++++++++++++++++
 hmp.c            |   29 +++++++++++++++++++
 hmp.h            |    3 ++
 qapi-schema.json |   43 +++++++++++++++++++++++++++++
 qemu-char.c      |   80 ++++++++++++++++++++++++++++++++++++++++++++---------
 qemu-char.h      |    6 +++-
 qmp-commands.hx  |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++
 vl.c             |    9 ++++--
 8 files changed, 272 insertions(+), 18 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 1/3] chardev: add error reporting for qemu_chr_new_from_opts
  2012-12-14  9:38 [Qemu-devel] [PATCH RESENT 0/3] chardev hotplug patch series Gerd Hoffmann
@ 2012-12-14  9:38 ` Gerd Hoffmann
  2012-12-14  9:38 ` [Qemu-devel] [PATCH 2/3] chardev: fix QemuOpts lifecycle Gerd Hoffmann
  2012-12-14  9:38 ` [Qemu-devel] [PATCH 3/3] chardev: add hotplug support Gerd Hoffmann
  2 siblings, 0 replies; 11+ messages in thread
From: Gerd Hoffmann @ 2012-12-14  9:38 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |   24 +++++++++++++++---------
 qemu-char.h |    3 ++-
 vl.c        |    9 ++++++---
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 242b799..5b91228 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2766,19 +2766,20 @@ static const struct {
 };
 
 CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
-                                    void (*init)(struct CharDriverState *s))
+                                    void (*init)(struct CharDriverState *s),
+                                    Error **errp)
 {
     CharDriverState *chr;
     int i;
 
     if (qemu_opts_id(opts) == NULL) {
-        fprintf(stderr, "chardev: no id specified\n");
+        error_setg(errp, "chardev: no id specified\n");
         return NULL;
     }
 
     if (qemu_opt_get(opts, "backend") == NULL) {
-        fprintf(stderr, "chardev: \"%s\" missing backend\n",
-                qemu_opts_id(opts));
+        error_setg(errp, "chardev: \"%s\" missing backend\n",
+                   qemu_opts_id(opts));
         return NULL;
     }
     for (i = 0; i < ARRAY_SIZE(backend_table); i++) {
@@ -2786,15 +2787,15 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
             break;
     }
     if (i == ARRAY_SIZE(backend_table)) {
-        fprintf(stderr, "chardev: backend \"%s\" not found\n",
-                qemu_opt_get(opts, "backend"));
+        error_setg(errp, "chardev: backend \"%s\" not found\n",
+                   qemu_opt_get(opts, "backend"));
         return NULL;
     }
 
     chr = backend_table[i].open(opts);
     if (!chr) {
-        fprintf(stderr, "chardev: opening backend \"%s\" failed\n",
-                qemu_opt_get(opts, "backend"));
+        error_setg(errp, "chardev: opening backend \"%s\" failed\n",
+                   qemu_opt_get(opts, "backend"));
         return NULL;
     }
 
@@ -2824,6 +2825,7 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
     const char *p;
     CharDriverState *chr;
     QemuOpts *opts;
+    Error *err = NULL;
 
     if (strstart(filename, "chardev:", &p)) {
         return qemu_chr_find(p);
@@ -2833,7 +2835,11 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
     if (!opts)
         return NULL;
 
-    chr = qemu_chr_new_from_opts(opts, init);
+    chr = qemu_chr_new_from_opts(opts, init, &err);
+    if (error_is_set(&err)) {
+        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_free(err);
+    }
     if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
         monitor_init(chr, MONITOR_USE_READLINE);
     }
diff --git a/qemu-char.h b/qemu-char.h
index a121e04..d7eed34 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -89,7 +89,8 @@ struct CharDriverState {
  * Returns: a new character backend
  */
 CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
-                                    void (*init)(struct CharDriverState *s));
+                                    void (*init)(struct CharDriverState *s),
+                                    Error **errp);
 
 /**
  * @qemu_chr_new:
diff --git a/vl.c b/vl.c
index a3ab384..353817a 100644
--- a/vl.c
+++ b/vl.c
@@ -2057,11 +2057,14 @@ static int device_init_func(QemuOpts *opts, void *opaque)
 
 static int chardev_init_func(QemuOpts *opts, void *opaque)
 {
-    CharDriverState *chr;
+    Error *local_err = NULL;
 
-    chr = qemu_chr_new_from_opts(opts, NULL);
-    if (!chr)
+    qemu_chr_new_from_opts(opts, NULL, &local_err);
+    if (error_is_set(&local_err)) {
+        fprintf(stderr, "%s\n", error_get_pretty(local_err));
+        error_free(local_err);
         return -1;
+    }
     return 0;
 }
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 2/3] chardev: fix QemuOpts lifecycle
  2012-12-14  9:38 [Qemu-devel] [PATCH RESENT 0/3] chardev hotplug patch series Gerd Hoffmann
  2012-12-14  9:38 ` [Qemu-devel] [PATCH 1/3] chardev: add error reporting for qemu_chr_new_from_opts Gerd Hoffmann
@ 2012-12-14  9:38 ` Gerd Hoffmann
  2012-12-14  9:38 ` [Qemu-devel] [PATCH 3/3] chardev: add hotplug support Gerd Hoffmann
  2 siblings, 0 replies; 11+ messages in thread
From: Gerd Hoffmann @ 2012-12-14  9:38 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann

qemu_chr_new_from_opts handles QemuOpts release now, so callers don't
have to worry.  It will either be saved in CharDriverState, then
released in qemu_chr_delete, or in the error case released instantly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |   15 ++++++++++-----
 qemu-char.h |    1 +
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 5b91228..876714f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2774,13 +2774,13 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
 
     if (qemu_opts_id(opts) == NULL) {
         error_setg(errp, "chardev: no id specified\n");
-        return NULL;
+        goto err;
     }
 
     if (qemu_opt_get(opts, "backend") == NULL) {
         error_setg(errp, "chardev: \"%s\" missing backend\n",
                    qemu_opts_id(opts));
-        return NULL;
+        goto err;
     }
     for (i = 0; i < ARRAY_SIZE(backend_table); i++) {
         if (strcmp(backend_table[i].name, qemu_opt_get(opts, "backend")) == 0)
@@ -2789,14 +2789,14 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
     if (i == ARRAY_SIZE(backend_table)) {
         error_setg(errp, "chardev: backend \"%s\" not found\n",
                    qemu_opt_get(opts, "backend"));
-        return NULL;
+        goto err;
     }
 
     chr = backend_table[i].open(opts);
     if (!chr) {
         error_setg(errp, "chardev: opening backend \"%s\" failed\n",
                    qemu_opt_get(opts, "backend"));
-        return NULL;
+        goto err;
     }
 
     if (!chr->filename)
@@ -2817,7 +2817,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
         chr->avail_connections = 1;
     }
     chr->label = g_strdup(qemu_opts_id(opts));
+    chr->opts = opts;
     return chr;
+
+err:
+    qemu_opts_del(opts);
+    return NULL;
 }
 
 CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*init)(struct CharDriverState *s))
@@ -2843,7 +2848,6 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
     if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
         monitor_init(chr, MONITOR_USE_READLINE);
     }
-    qemu_opts_del(opts);
     return chr;
 }
 
@@ -2875,6 +2879,7 @@ void qemu_chr_delete(CharDriverState *chr)
         chr->chr_close(chr);
     g_free(chr->filename);
     g_free(chr->label);
+    qemu_opts_del(chr->opts);
     g_free(chr);
 }
 
diff --git a/qemu-char.h b/qemu-char.h
index d7eed34..f984071 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -75,6 +75,7 @@ struct CharDriverState {
     char *filename;
     int opened;
     int avail_connections;
+    QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.
  2012-12-14  9:38 [Qemu-devel] [PATCH RESENT 0/3] chardev hotplug patch series Gerd Hoffmann
  2012-12-14  9:38 ` [Qemu-devel] [PATCH 1/3] chardev: add error reporting for qemu_chr_new_from_opts Gerd Hoffmann
  2012-12-14  9:38 ` [Qemu-devel] [PATCH 2/3] chardev: fix QemuOpts lifecycle Gerd Hoffmann
@ 2012-12-14  9:38 ` Gerd Hoffmann
  2012-12-14 12:17   ` Paolo Bonzini
  2 siblings, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2012-12-14  9:38 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann

This patch adds chardev_add_file, chardev_add_tty and chardev_remove
monitor commands.

chardev_add_file and chardev_add_tty expect an id and a path, they
create a file/tty chardev.

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

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hmp-commands.hx  |   44 +++++++++++++++++++++++++++++++
 hmp.c            |   29 ++++++++++++++++++++
 hmp.h            |    3 ++
 qapi-schema.json |   43 ++++++++++++++++++++++++++++++
 qemu-char.c      |   41 +++++++++++++++++++++++++++++
 qemu-char.h      |    2 +
 qmp-commands.hx  |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 238 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 010b8c9..82a855a 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1485,6 +1485,50 @@ passed since 1970, i.e. unix epoch.
 ETEXI
 
     {
+        .name       = "chardev_add_file",
+        .args_type  = "id:s,path:s",
+        .params     = "id path ",
+        .help       = "add file chardev",
+        .mhandler.cmd = hmp_chardev_add_file,
+    },
+
+STEXI
+@item chardev_add_file id path
+@findex chardev_add_file
+
+ETEXI
+
+    {
+        .name       = "chardev_add_tty",
+        .args_type  = "id:s,path:s",
+        .params     = "id path ",
+        .help       = "add tty chardev",
+        .mhandler.cmd = hmp_chardev_add_tty,
+    },
+
+STEXI
+@item chardev_add_tty id path
+@findex chardev_add_tty
+
+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..8780e7c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1335,3 +1335,32 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
     qmp_nbd_server_stop(&errp);
     hmp_handle_error(mon, &errp);
 }
+
+static void hmp_chardev_add_path(Monitor *mon, const QDict *qdict,
+                                 const char *backend)
+{
+    const char *id   = qdict_get_str(qdict, "args");
+    const char *path = qdict_get_str(qdict, "path");
+    Error *local_err = NULL;
+
+    qmp_chardev_add_path(id, path, backend, &local_err);
+    hmp_handle_error(mon, &local_err);
+}
+
+void hmp_chardev_add_file(Monitor *mon, const QDict *qdict)
+{
+    hmp_chardev_add_path(mon, qdict, "file");
+}
+
+void hmp_chardev_add_tty(Monitor *mon, const QDict *qdict)
+{
+    hmp_chardev_add_path(mon, qdict, "tty");
+}
+
+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..8cd50d1 100644
--- a/hmp.h
+++ b/hmp.h
@@ -80,5 +80,8 @@ 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_file(Monitor *mon, const QDict *qdict);
+void hmp_chardev_add_tty(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..34c0e58 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3017,3 +3017,46 @@
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-stop' }
+
+##
+# @chardev-add-file:
+#
+# Add a file chardev
+#
+# @id: the chardev's ID, must be unique
+# @path: file path
+#
+# Returns: Nothing on success
+#
+# Since: 1.3.0
+##
+{ 'command': 'chardev-add-file', 'data': {'id'   : 'str',
+                                          'path' : 'str' } }
+
+##
+# @chardev-add-tty:
+#
+# Add a terminal chardev
+#
+# @id: the chardev's ID, must be unique
+# @path: device path
+#
+# Returns: Nothing on success
+#
+# Since: 1.3.0
+##
+{ 'command': 'chardev-add-tty', 'data': {'id'   : 'str',
+                                         'path' : 'str' } }
+
+##
+# @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..169743b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2922,3 +2922,44 @@ 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_file(const char *id, const char *path, Error **errp)
+{
+    qmp_chardev_add_path(id, path, "file", errp);
+}
+
+void qmp_chardev_add_tty(const char *id, const char *path, Error **errp)
+{
+    qmp_chardev_add_path(id, path, "tty", errp);
+}
+
+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..5ff8cd1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2654,3 +2654,79 @@ EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_target,
     },
+
+    {
+        .name       = "chardev-add-file",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_chardev_add_file,
+    },
+
+SQMP
+chardev-add-file
+----------------
+
+Add a file chardev.
+
+Arguments:
+
+- "id": the chardev's ID, must be unique (json-string)
+- "path": file path (json-string)
+
+Example:
+
+-> { "execute"   : "chardev-add-file",
+     "arguments" : { "id"   : "foo",
+                     "path" : "/tmp/foo" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "chardev-add-tty",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_chardev_add_tty,
+    },
+
+SQMP
+chardev-add-tty
+---------------
+
+Add a terminal chardev.
+
+Arguments:
+
+- "id": the chardev's ID, must be unique (json-string)
+- "path": device path (json-string)
+
+Example:
+
+-> { "execute"   : "chardev-add-tty",
+     "arguments" : { "id"   : "serial",
+                     "path" : "/dev/ttyS0" } }
+<- { "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
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.
  2012-12-14  9:38 ` [Qemu-devel] [PATCH 3/3] chardev: add hotplug support Gerd Hoffmann
@ 2012-12-14 12:17   ` Paolo Bonzini
  2012-12-14 13:05     ` Eric Blake
  2012-12-14 13:18     ` Gerd Hoffmann
  0 siblings, 2 replies; 11+ messages in thread
From: Paolo Bonzini @ 2012-12-14 12:17 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

Il 14/12/2012 10:38, Gerd Hoffmann ha scritto:
> This patch adds chardev_add_file, chardev_add_tty and chardev_remove
> monitor commands.
> 
> chardev_add_file and chardev_add_tty expect an id and a path, they
> create a file/tty chardev.

I'd rather avoid introducing this interface.  Using multiple commands is
different from all previous examples, both HMP and QMP (including recent
ones such as the NBD server).  It is also hard to extend, for example
file descriptor passing is hard to retrofit.

Perhaps you can define a QAPI union and slowly build it up?  Something
that ultimately can become this:

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

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

{ 'type': 'ChardevFile',
  'data': {'source': 'string', 'source-type': 'ChardevFileSource',
           'mode': 'ChardevFileMode'}}

{ 'type': 'ChardevVC',
  'data': {'width': 'int', 'height': 'int', '*characters': 'bool'}}

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

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

{ 'union': 'ChardevBackend', 'data': {
  'socket': 'ChardevSocket',
  'udp': 'UDPSocketAddress',
  'file': 'ChardevFile',
  'null': 'ChardevDummy',
  'msmouse': 'ChardevDummy',
  'braille': 'ChardevDummy',
  'stdio': 'ChardevDummy',
  'vc': 'ChardevVC',

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

{ 'command': 'chardev-add', 'data': {
  'backend': 'ChardevBackend', 'id': 'str', '*mux': 'bool' } }

A simple conversion from this to QemuOpts should be easy, later the
backends can move to taking a ChardevBackend *.

Paolo

> chardev_del just takes an id argument and zaps the chardev specified.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hmp-commands.hx  |   44 +++++++++++++++++++++++++++++++
>  hmp.c            |   29 ++++++++++++++++++++
>  hmp.h            |    3 ++
>  qapi-schema.json |   43 ++++++++++++++++++++++++++++++
>  qemu-char.c      |   41 +++++++++++++++++++++++++++++
>  qemu-char.h      |    2 +
>  qmp-commands.hx  |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 238 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 010b8c9..82a855a 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1485,6 +1485,50 @@ passed since 1970, i.e. unix epoch.
>  ETEXI
>  
>      {
> +        .name       = "chardev_add_file",
> +        .args_type  = "id:s,path:s",
> +        .params     = "id path ",
> +        .help       = "add file chardev",
> +        .mhandler.cmd = hmp_chardev_add_file,
> +    },
> +
> +STEXI
> +@item chardev_add_file id path
> +@findex chardev_add_file
> +
> +ETEXI
> +
> +    {
> +        .name       = "chardev_add_tty",
> +        .args_type  = "id:s,path:s",
> +        .params     = "id path ",
> +        .help       = "add tty chardev",
> +        .mhandler.cmd = hmp_chardev_add_tty,
> +    },
> +
> +STEXI
> +@item chardev_add_tty id path
> +@findex chardev_add_tty
> +
> +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..8780e7c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1335,3 +1335,32 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
>      qmp_nbd_server_stop(&errp);
>      hmp_handle_error(mon, &errp);
>  }
> +
> +static void hmp_chardev_add_path(Monitor *mon, const QDict *qdict,
> +                                 const char *backend)
> +{
> +    const char *id   = qdict_get_str(qdict, "args");
> +    const char *path = qdict_get_str(qdict, "path");
> +    Error *local_err = NULL;
> +
> +    qmp_chardev_add_path(id, path, backend, &local_err);
> +    hmp_handle_error(mon, &local_err);
> +}
> +
> +void hmp_chardev_add_file(Monitor *mon, const QDict *qdict)
> +{
> +    hmp_chardev_add_path(mon, qdict, "file");
> +}
> +
> +void hmp_chardev_add_tty(Monitor *mon, const QDict *qdict)
> +{
> +    hmp_chardev_add_path(mon, qdict, "tty");
> +}
> +
> +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..8cd50d1 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -80,5 +80,8 @@ 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_file(Monitor *mon, const QDict *qdict);
> +void hmp_chardev_add_tty(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..34c0e58 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3017,3 +3017,46 @@
>  # Since: 1.3.0
>  ##
>  { 'command': 'nbd-server-stop' }
> +
> +##
> +# @chardev-add-file:
> +#
> +# Add a file chardev
> +#
> +# @id: the chardev's ID, must be unique
> +# @path: file path
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 1.3.0
> +##
> +{ 'command': 'chardev-add-file', 'data': {'id'   : 'str',
> +                                          'path' : 'str' } }
> +
> +##
> +# @chardev-add-tty:
> +#
> +# Add a terminal chardev
> +#
> +# @id: the chardev's ID, must be unique
> +# @path: device path
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 1.3.0
> +##
> +{ 'command': 'chardev-add-tty', 'data': {'id'   : 'str',
> +                                         'path' : 'str' } }
> +
> +##
> +# @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..169743b 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2922,3 +2922,44 @@ 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_file(const char *id, const char *path, Error **errp)
> +{
> +    qmp_chardev_add_path(id, path, "file", errp);
> +}
> +
> +void qmp_chardev_add_tty(const char *id, const char *path, Error **errp)
> +{
> +    qmp_chardev_add_path(id, path, "tty", errp);
> +}
> +
> +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..5ff8cd1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2654,3 +2654,79 @@ EQMP
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_query_target,
>      },
> +
> +    {
> +        .name       = "chardev-add-file",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_chardev_add_file,
> +    },
> +
> +SQMP
> +chardev-add-file
> +----------------
> +
> +Add a file chardev.
> +
> +Arguments:
> +
> +- "id": the chardev's ID, must be unique (json-string)
> +- "path": file path (json-string)
> +
> +Example:
> +
> +-> { "execute"   : "chardev-add-file",
> +     "arguments" : { "id"   : "foo",
> +                     "path" : "/tmp/foo" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name       = "chardev-add-tty",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_chardev_add_tty,
> +    },
> +
> +SQMP
> +chardev-add-tty
> +---------------
> +
> +Add a terminal chardev.
> +
> +Arguments:
> +
> +- "id": the chardev's ID, must be unique (json-string)
> +- "path": device path (json-string)
> +
> +Example:
> +
> +-> { "execute"   : "chardev-add-tty",
> +     "arguments" : { "id"   : "serial",
> +                     "path" : "/dev/ttyS0" } }
> +<- { "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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.
  2012-12-14 12:17   ` Paolo Bonzini
@ 2012-12-14 13:05     ` Eric Blake
  2012-12-14 13:18     ` Gerd Hoffmann
  1 sibling, 0 replies; 11+ messages in thread
From: Eric Blake @ 2012-12-14 13:05 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Gerd Hoffmann, qemu-devel

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

On 12/14/2012 05:17 AM, Paolo Bonzini wrote:
> Il 14/12/2012 10:38, Gerd Hoffmann ha scritto:
>> This patch adds chardev_add_file, chardev_add_tty and chardev_remove
>> monitor commands.
>>
>> chardev_add_file and chardev_add_tty expect an id and a path, they
>> create a file/tty chardev.
> 
> I'd rather avoid introducing this interface.  Using multiple commands is
> different from all previous examples, both HMP and QMP (including recent
> ones such as the NBD server).  It is also hard to extend, for example
> file descriptor passing is hard to retrofit.

File descriptor passing via magic /dev/fdset/nnn should probably already
work.  That said, a single command that uses a QAPI union, rather than
one command per source type, would be nicer from the UI perspective, and
it is the QMP UI perspective that libvirt is concerned about.

> 
> Perhaps you can define a QAPI union and slowly build it up?  Something
> that ultimately can become this:
> 
> { 'enum': 'ChardevFileMode', 'data':
>   # pty = console under Windows
>   # serial = tty under POSIX
>   [ 'file', 'pipe', 'parport', 'pty', 'serial' ] }
> 
> { 'enum: 'ChardevFileSource', 'data':
>   [ 'path', 'fd' ] }
> 
> { 'type': 'ChardevFile',
>   'data': {'source': 'string', 'source-type': 'ChardevFileSource',
>            'mode': 'ChardevFileMode'}}
> 
> { 'type': 'ChardevVC',
>   'data': {'width': 'int', 'height': 'int', '*characters': 'bool'}}
> 
> { 'type': 'ChardevSocket',
>   'data': {'addr': 'SocketAddress', '*server': 'bool',
>            '*wait': 'bool', '*nodelay': 'bool', '*telnet': 'bool'} }
> 
> # For future extensibility...
> { 'ChardevDummy', 'data': {} }
> 
> { 'union': 'ChardevBackend', 'data': {
>   'socket': 'ChardevSocket',
>   'udp': 'UDPSocketAddress',
>   'file': 'ChardevFile',
>   'null': 'ChardevDummy',
>   'msmouse': 'ChardevDummy',
>   'braille': 'ChardevDummy',
>   'stdio': 'ChardevDummy',
>   'vc': 'ChardevVC',
> 
>   # Solely for HMP usage.
>   'legacy': 'str'
> }
> 
> { 'command': 'chardev-add', 'data': {
>   'backend': 'ChardevBackend', 'id': 'str', '*mux': 'bool' } }

Yes, this looks nicer.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.
  2012-12-14 12:17   ` Paolo Bonzini
  2012-12-14 13:05     ` Eric Blake
@ 2012-12-14 13:18     ` Gerd Hoffmann
  2012-12-14 13:45       ` Paolo Bonzini
  1 sibling, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2012-12-14 13:18 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

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

  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


[-- Attachment #2: 0001-chardev-add-hotplug-support.patch --]
[-- Type: text/plain, Size: 7216 bytes --]

>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(-)

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
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.
  2012-12-14 13:18     ` Gerd Hoffmann
@ 2012-12-14 13:45       ` Paolo Bonzini
  2012-12-14 14:05         ` Gerd Hoffmann
  0 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2012-12-14 13:45 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.
  2012-12-14 13:45       ` Paolo Bonzini
@ 2012-12-14 14:05         ` Gerd Hoffmann
  2012-12-14 14:19           ` Gerd Hoffmann
  0 siblings, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2012-12-14 14:05 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.
  2012-12-14 14:05         ` Gerd Hoffmann
@ 2012-12-14 14:19           ` Gerd Hoffmann
  2012-12-14 15:07             ` Paolo Bonzini
  0 siblings, 1 reply; 11+ messages in thread
From: Gerd Hoffmann @ 2012-12-14 14:19 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel

  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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.
  2012-12-14 14:19           ` Gerd Hoffmann
@ 2012-12-14 15:07             ` Paolo Bonzini
  0 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2012-12-14 15:07 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: qemu-devel

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-12-14 15:07 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-14  9:38 [Qemu-devel] [PATCH RESENT 0/3] chardev hotplug patch series Gerd Hoffmann
2012-12-14  9:38 ` [Qemu-devel] [PATCH 1/3] chardev: add error reporting for qemu_chr_new_from_opts Gerd Hoffmann
2012-12-14  9:38 ` [Qemu-devel] [PATCH 2/3] chardev: fix QemuOpts lifecycle Gerd Hoffmann
2012-12-14  9:38 ` [Qemu-devel] [PATCH 3/3] chardev: add hotplug support Gerd Hoffmann
2012-12-14 12:17   ` Paolo Bonzini
2012-12-14 13:05     ` Eric Blake
2012-12-14 13:18     ` Gerd Hoffmann
2012-12-14 13:45       ` Paolo Bonzini
2012-12-14 14:05         ` Gerd Hoffmann
2012-12-14 14:19           ` Gerd Hoffmann
2012-12-14 15:07             ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.