[Qemu-devel] [PATCH 0/3] chardev hotplug patch series

[Qemu-devel] [PATCH 0/3] chardev hotplug patch series

[Gerd Hoffmann]

  Hi,

Here comes the updated chardev hotplug patch series.  Addressed most
review comments.  It's chardev-add and chardev-remove now.  Parameters
have been added to the schema.  Little nits and spell fixes here and
there.

Making 'backend' an enum is tricky given that the actual implementation
just turns the qdict into a QemuOpts, then goes piggyback on
qemu_chr_new_from_opts().

Not rebased (yet) on top of the qom queue as I expect more discussions.

please review,
  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  |   32 +++++++++++++++++++++
 hmp.c            |   23 +++++++++++++++
 hmp.h            |    2 +
 qapi-schema.json |   47 ++++++++++++++++++++++++++++++
 qemu-char.c      |   83 ++++++++++++++++++++++++++++++++++++++++++++---------
 qemu-char.h      |    5 ++-
 qmp-commands.hx  |   61 +++++++++++++++++++++++++++++++++++++++
 vl.c             |    8 ++++-
 8 files changed, 244 insertions(+), 17 deletions(-)

[Qemu-devel] [PATCH 1/3] chardev: add error reporting for qemu_chr_new_from_opts

[Gerd Hoffmann]

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

diff --git a/qemu-char.c b/qemu-char.c
index b082bae..e2e1da8 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2755,19 +2755,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++) {
@@ -2775,15 +2776,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;
     }
 
@@ -2813,6 +2814,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);
@@ -2822,7 +2824,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 486644b..adae13e 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -88,7 +88,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 5b357a3..d2f1c10 100644
--- a/vl.c
+++ b/vl.c
@@ -1940,10 +1940,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)
+    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

[Qemu-devel] [PATCH 2/3] chardev: fix QemuOpts lifecycle

[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 e2e1da8..be4ec61 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2763,13 +2763,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)
@@ -2778,14 +2778,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)
@@ -2806,7 +2806,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))
@@ -2832,7 +2837,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;
 }
 
@@ -2864,6 +2868,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 adae13e..99bc132 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -74,6 +74,7 @@ struct CharDriverState {
     char *filename;
     int opened;
     int avail_connections;
+    QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
-- 
1.7.1

[Qemu-devel] [PATCH 3/3] chardev: add hotplug support.

[Gerd Hoffmann]

This patch adds chardev_add and chardev_remove monitor commands.

They work similar to the netdev_{add,del} commands.  The hmp version of
chardev_add accepts like the -chardev command line option does.  The qmp
version expects the arguments being passed as named parameters.

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

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hmp-commands.hx  |   32 ++++++++++++++++++++++++++++
 hmp.c            |   23 ++++++++++++++++++++
 hmp.h            |    2 +
 qapi-schema.json |   47 +++++++++++++++++++++++++++++++++++++++++
 qemu-char.c      |   44 ++++++++++++++++++++++++++++++++++++++
 qemu-char.h      |    1 +
 qmp-commands.hx  |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 210 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index e0b537d..5207383 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1404,6 +1404,38 @@ passed since 1970, i.e. unix epoch.
 ETEXI
 
     {
+        .name       = "chardev_add",
+        .args_type  = "args:s",
+        .params     = "args",
+        .help       = "add chardev",
+        .mhandler.cmd = hmp_chardev_add,
+    },
+
+STEXI
+@item chardev_add args
+@findex chardev_add
+
+chardev_add accepts the same parameters as the -chardev command line switch.
+
+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 70bdec2..fc9d7bc 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1209,3 +1209,26 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
     qmp_screendump(filename, &err);
     hmp_handle_error(mon, &err);
 }
+
+void hmp_chardev_add(Monitor *mon, const QDict *qdict)
+{
+    const char *args = qdict_get_str(qdict, "args");
+    Error *err = NULL;
+    QemuOpts *opts;
+
+    opts = qemu_opts_parse(qemu_find_opts("chardev"), args, 1);
+    if (opts == NULL) {
+        error_setg(&err, "Parsing chardev args failed\n");
+    } else {
+        qemu_chr_new_from_opts(opts, NULL, &err);
+    }
+    hmp_handle_error(mon, &err);
+}
+
+void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    qmp_chardev_remove(qdict_get_str(qdict, "id"),
+                       &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 71ea384..cabe944 100644
--- a/hmp.h
+++ b/hmp.h
@@ -75,5 +75,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
 void hmp_send_key(Monitor *mon, const QDict *qdict);
 void hmp_screen_dump(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 f9dbdae..fab6bbc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2796,3 +2796,50 @@
 # Since: 0.14.0
 ##
 { 'command': 'screendump', 'data': {'filename': 'str'} }
+
+##
+# @chardev-add:
+#
+# Add a chardev
+#
+# @id: the chardev's ID, must be unique
+# @backend: the chardev backend: "file", "socket", ...
+# @path: file / device / unix socket path
+# @name: spice channel name
+# @host: host name
+# @port: port number
+# @server: create socket in server mode
+# @wait: wait for connect
+# @ipv4: force ipv4-only
+# @ipv6: force ipv6-only
+# @telnet: telnet negotiation
+#
+# Returns: Nothing on success
+#
+# Since: 1.3.0
+##
+{ 'command': 'chardev-add', 'data': {'id'      : 'str',
+                                     'backend' : 'str',
+                                     '*path'   : 'str',
+                                     '*name'   : 'str',
+                                     '*host'   : 'str',
+                                     '*port'   : 'str',
+                                     '*server' : 'bool',
+                                     '*wait'   : 'bool',
+                                     '*ipv4'   : 'bool',
+                                     '*ipv6'   : 'bool',
+                                     '*telnet' : 'bool' },
+  'gen': 'no' }
+
+##
+# @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 be4ec61..cbfa24e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2911,3 +2911,47 @@ CharDriverState *qemu_char_get_next_serial(void)
     return serial_hds[next_serial++];
 }
 
+int qmp_chardev_add(Monitor *mon, const QDict *qdict, QObject **ret)
+{
+    Error *err = NULL;
+    QemuOptsList *opts_list;
+    QemuOpts *opts;
+
+    opts_list = qemu_find_opts_err("chardev", &err);
+    if (error_is_set(&err)) {
+        goto exit_err;
+    }
+
+    opts = qemu_opts_from_qdict(opts_list, qdict, &err);
+    if (error_is_set(&err)) {
+        goto exit_err;
+    }
+
+    qemu_chr_new_from_opts(opts, NULL, &err);
+    if (error_is_set(&err)) {
+        goto exit_err;
+    }
+    return 0;
+
+exit_err:
+    qerror_report_err(err);
+    error_free(err);
+    return -1;
+}
+
+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 99bc132..407e0fe 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -238,6 +238,7 @@ 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);
+int qmp_chardev_add(Monitor *mon, const QDict *qdict, QObject **ret);
 
 /* 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 2f8477e..ea5fc71 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2576,3 +2576,64 @@ EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_target,
     },
+
+    {
+        .name       = "chardev-add",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_chardev_add,
+    },
+
+SQMP
+chardev-add
+-----------
+
+Add a chardev.
+
+Arguments:
+
+- "id": the chardev's ID, must be unique (json-string)
+- "backend": the chardev backend: "file", "socket", ... (json-string)
+- "path": file / device / unix socket path (json-string, optional)
+- "name": spice channel name (json-string, optional)
+- "host": host name (json-string, optional)
+- "port": port number (json-string, optional)
+- "server": create socket in server mode (json-bool, optional)
+- "wait": wait for connect (json-bool, optional)
+- "ipv4": force ipv4-only (json-bool, optional)
+- "ipv6": force ipv6-only (json-bool, optional)
+- "telnet": telnet negotiation (json-bool, optional)
+
+Example:
+
+-> { "execute": "chardev-add", "arguments": { "id"      : "foo",
+                                              "backend" : "socket",
+                                              "path"    : "/tmp/foo",
+                                              "server"  : true,
+                                              "wait"    : false } }
+<- { "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

Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.

[Eric Blake]

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

On 10/17/2012 04:09 AM, Gerd Hoffmann wrote:
> This patch adds chardev_add and chardev_remove monitor commands.
> 
> They work similar to the netdev_{add,del} commands.  The hmp version of
> chardev_add accepts like the -chardev command line option does.  The qmp
> version expects the arguments being passed as named parameters.
> 
> chardev_del just takes an id argument and zaps the chardev specified.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

> +##
> +# @chardev-add:
> +#
> +# Add a chardev
> +#
> +# @id: the chardev's ID, must be unique
> +# @backend: the chardev backend: "file", "socket", ...

This still needs to be an enum.

> +# @path: file / device / unix socket path
> +# @name: spice channel name
> +# @host: host name
> +# @port: port number
> +# @server: create socket in server mode
> +# @wait: wait for connect
> +# @ipv4: force ipv4-only
> +# @ipv6: force ipv6-only
> +# @telnet: telnet negotiation

But this part 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: 617 bytes --]

Re: [Qemu-devel] [PATCH 1/3] chardev: add error reporting for qemu_chr_new_from_opts

[Luiz Capitulino]

On Wed, 17 Oct 2012 12:09:53 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  qemu-char.c |   24 +++++++++++++++---------
>  qemu-char.h |    3 ++-
>  vl.c        |    8 ++++++--
>  3 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index b082bae..e2e1da8 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2755,19 +2755,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++) {
> @@ -2775,15 +2776,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;
>      }
>  
> @@ -2813,6 +2814,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);
> @@ -2822,7 +2824,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 486644b..adae13e 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -88,7 +88,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 5b357a3..d2f1c10 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1940,10 +1940,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)
> +    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;
>  }
>  

Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.

[Paolo Bonzini]

Il 17/10/2012 17:26, Eric Blake ha scritto:
> On 10/17/2012 04:09 AM, Gerd Hoffmann wrote:
>> This patch adds chardev_add and chardev_remove monitor commands.
>>
>> They work similar to the netdev_{add,del} commands.  The hmp version of
>> chardev_add accepts like the -chardev command line option does.  The qmp
>> version expects the arguments being passed as named parameters.
>>
>> chardev_del just takes an id argument and zaps the chardev specified.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
> 
>> +##
>> +# @chardev-add:
>> +#
>> +# Add a chardev
>> +#
>> +# @id: the chardev's ID, must be unique
>> +# @backend: the chardev backend: "file", "socket", ...
> 
> This still needs to be an enum.
> 
>> +# @path: file / device / unix socket path
>> +# @name: spice channel name
>> +# @host: host name
>> +# @port: port number
>> +# @server: create socket in server mode
>> +# @wait: wait for connect
>> +# @ipv4: force ipv4-only
>> +# @ipv6: force ipv6-only
>> +# @telnet: telnet negotiation
> 
> But this part looks nicer.

Note that this schema is just for documentation.  It is not used at QEMU
build time.

Paolo

Re: [Qemu-devel] [PATCH 2/3] chardev: fix QemuOpts lifecycle

[Luiz Capitulino]

On Wed, 17 Oct 2012 12:09:54 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> 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 e2e1da8..be4ec61 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2763,13 +2763,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)
> @@ -2778,14 +2778,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)
> @@ -2806,7 +2806,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))
> @@ -2832,7 +2837,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;
>  }
>  
> @@ -2864,6 +2868,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);
>  }

Didn't consider this in my first review, but is chardev_init_func() fine
with this? Basically, if we chardev_remove a device created by it (is this
possible?) then the opts will be freed but it will remain inserted in its
QemuOptsList.

>  
> diff --git a/qemu-char.h b/qemu-char.h
> index adae13e..99bc132 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -74,6 +74,7 @@ struct CharDriverState {
>      char *filename;
>      int opened;
>      int avail_connections;
> +    QemuOpts *opts;
>      QTAILQ_ENTRY(CharDriverState) next;
>  };
>  

Re: [Qemu-devel] [PATCH 2/3] chardev: fix QemuOpts lifecycle

[Gerd Hoffmann]

  Hi,

>> @@ -2864,6 +2868,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);
>>  }
> 
> Didn't consider this in my first review, but is chardev_init_func() fine
> with this? Basically, if we chardev_remove a device created by it (is this
> possible?) then the opts will be freed but it will remain inserted in its
> QemuOptsList.

Hmm? Don't see your issue here.
qemu_opts_del will also unlink from QemuOptsList ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.

[Gerd Hoffmann]

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

On 10/17/12 12:09, Gerd Hoffmann wrote:
> This patch adds chardev_add and chardev_remove monitor commands.
> 
> They work similar to the netdev_{add,del} commands.  The hmp version of
> chardev_add accepts like the -chardev command line option does.  The qmp
> version expects the arguments being passed as named parameters.

Trying another approach, see attached patch.  This adds backend-specific
qemu commands to add chardevs, with just the parameters needed for the
specific backend.  Starting with file and tty, both accepting a path.
'file' is nice for testing, 'tty' very useful when hotplugging serial
devices on the host.  Others can be added as needed.  We probably don't
need all of them.  For example hotplugging the 'stdio' chardev doesn't
make much sense.

Advantage #1: Cleaner API.
Advantage #2: No legacy syntax headache when qomifying chardevs.

Comments?

cheers,
  Gerd

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

From 7f80af55530c9a448e4aed5a41c76a8e9514a27c 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_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 e0b537d..ecfc497 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1404,6 +1404,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 70bdec2..346dd29 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1209,3 +1209,32 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
     qmp_screendump(filename, &err);
     hmp_handle_error(mon, &err);
 }
+
+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 71ea384..107941d 100644
--- a/hmp.h
+++ b/hmp.h
@@ -75,5 +75,8 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
 void hmp_send_key(Monitor *mon, const QDict *qdict);
 void hmp_screen_dump(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 f9dbdae..76e765b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2796,3 +2796,46 @@
 # Since: 0.14.0
 ##
 { 'command': 'screendump', 'data': {'filename': 'str'} }
+
+##
+# @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 be4ec61..76c3396 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2911,3 +2911,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 99bc132..ffe4c79 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -238,6 +238,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 2f8477e..73c02ed 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2576,3 +2576,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

Re: [Qemu-devel] [PATCH 2/3] chardev: fix QemuOpts lifecycle

[Luiz Capitulino]

On Thu, 18 Oct 2012 11:26:39 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> >> @@ -2864,6 +2868,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);
> >>  }
> > 
> > Didn't consider this in my first review, but is chardev_init_func() fine
> > with this? Basically, if we chardev_remove a device created by it (is this
> > possible?) then the opts will be freed but it will remain inserted in its
> > QemuOptsList.
> 
> Hmm? Don't see your issue here.
> qemu_opts_del will also unlink from QemuOptsList ...

Missed that fact, thanks for clarifying:

	Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] chardev: add hotplug support.

[Paolo Bonzini]

Il 18/10/2012 12:03, Gerd Hoffmann ha scritto:
> Trying another approach, see attached patch.  This adds backend-specific
> qemu commands to add chardevs, with just the parameters needed for the
> specific backend.  Starting with file and tty, both accepting a path.
> 'file' is nice for testing, 'tty' very useful when hotplugging serial
> devices on the host.  Others can be added as needed.  We probably don't
> need all of them.  For example hotplugging the 'stdio' chardev doesn't
> make much sense.
> 
> Advantage #1: Cleaner API.
> Advantage #2: No legacy syntax headache when qomifying chardevs.

Disadvantage #1: Harder to extend, more code
Disadvantage #2: Different from all other kinds of host devices.

I still prefer the other one.

Paolo

[Qemu-devel] [PATCH 1/3] chardev: add error reporting for qemu_chr_new_from_opts

[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