[Qemu-devel] [PATCH 00/11] chardev hotplug patch series

[Qemu-devel] [PATCH 00/11] chardev hotplug patch series

[Gerd Hoffmann]

  Hi,

Next round of the chardev hotplug patch series, with better
commit messages, and tested this time.

cheers,
  Gerd

Gerd Hoffmann (11):
  chardev: add error reporting for qemu_chr_new_from_opts
  chardev: fix QemuOpts lifecycle
  chardev: reduce chardev ifdef mess a bit
  chardev: add qmp hotplug commands, with null chardev support
  chardev: add hmp hotplug commands
  chardev: add file chardev support to chardev-add (qmp)
  chardev: add tty chardev support to chardev-add (qmp)
  chardev: add serial chardev support to chardev-add (qmp)
  chardev: add parport chardev support to chardev-add (qmp)
  chardev: add socket chardev support to chardev-add (qmp)
  chardev: add pty chardev support to chardev-add (qmp)

 hmp-commands.hx     |   32 ++++
 hmp.c               |   23 +++
 hmp.h               |    2 +
 include/char/char.h |    4 +-
 qapi-schema.json    |   59 +++++++
 qemu-char.c         |  477 ++++++++++++++++++++++++++++++++++++++++-----------
 qmp-commands.hx     |   50 ++++++
 vl.c                |    9 +-
 8 files changed, 549 insertions(+), 107 deletions(-)

[Qemu-devel] [PATCH 01/11] chardev: add error reporting for qemu_chr_new_from_opts

[Gerd Hoffmann]

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

diff --git a/include/char/char.h b/include/char/char.h
index baa5d03..1952a10 100644
--- a/include/char/char.h
+++ b/include/char/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/qemu-char.c b/qemu-char.c
index f41788c..91f64e9 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2779,19 +2779,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++) {
@@ -2799,15 +2800,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;
     }
 
@@ -2837,6 +2838,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);
@@ -2846,7 +2848,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/vl.c b/vl.c
index f056c95..f5f2026 100644
--- a/vl.c
+++ b/vl.c
@@ -2052,11 +2052,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

Re: [Qemu-devel] [PATCH 01/11] chardev: add error reporting for qemu_chr_new_from_opts

[Paolo Bonzini]

Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> ---
>  include/char/char.h |    3 ++-
>  qemu-char.c         |   24 +++++++++++++++---------
>  vl.c                |    9 ++++++---
>  3 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/include/char/char.h b/include/char/char.h
> index baa5d03..1952a10 100644
> --- a/include/char/char.h
> +++ b/include/char/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/qemu-char.c b/qemu-char.c
> index f41788c..91f64e9 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2779,19 +2779,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++) {
> @@ -2799,15 +2800,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;
>      }
>  
> @@ -2837,6 +2838,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);
> @@ -2846,7 +2848,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/vl.c b/vl.c
> index f056c95..f5f2026 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2052,11 +2052,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;
>  }
>  
> 

[Qemu-devel] [PATCH 02/11] 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>
---
 include/char/char.h |    1 +
 qemu-char.c         |   20 ++++++++++++++------
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/include/char/char.h b/include/char/char.h
index 1952a10..c91ce3c 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -75,6 +75,7 @@ struct CharDriverState {
     char *filename;
     int opened;
     int avail_connections;
+    QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
 };
 
diff --git a/qemu-char.c b/qemu-char.c
index 91f64e9..a29c2bb 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2787,13 +2787,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)
@@ -2802,14 +2802,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)
@@ -2830,7 +2830,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))
@@ -2856,7 +2861,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;
 }
 
@@ -2884,10 +2888,14 @@ void qemu_chr_fe_close(struct CharDriverState *chr)
 void qemu_chr_delete(CharDriverState *chr)
 {
     QTAILQ_REMOVE(&chardevs, chr, next);
-    if (chr->chr_close)
+    if (chr->chr_close) {
         chr->chr_close(chr);
+    }
     g_free(chr->filename);
     g_free(chr->label);
+    if (chr->opts) {
+        qemu_opts_del(chr->opts);
+    }
     g_free(chr);
 }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 02/11] chardev: fix QemuOpts lifecycle

[Paolo Bonzini]

Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
> 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>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> ---
>  include/char/char.h |    1 +
>  qemu-char.c         |   20 ++++++++++++++------
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/include/char/char.h b/include/char/char.h
> index 1952a10..c91ce3c 100644
> --- a/include/char/char.h
> +++ b/include/char/char.h
> @@ -75,6 +75,7 @@ struct CharDriverState {
>      char *filename;
>      int opened;
>      int avail_connections;
> +    QemuOpts *opts;
>      QTAILQ_ENTRY(CharDriverState) next;
>  };
>  
> diff --git a/qemu-char.c b/qemu-char.c
> index 91f64e9..a29c2bb 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2787,13 +2787,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)
> @@ -2802,14 +2802,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)
> @@ -2830,7 +2830,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))
> @@ -2856,7 +2861,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;
>  }
>  
> @@ -2884,10 +2888,14 @@ void qemu_chr_fe_close(struct CharDriverState *chr)
>  void qemu_chr_delete(CharDriverState *chr)
>  {
>      QTAILQ_REMOVE(&chardevs, chr, next);
> -    if (chr->chr_close)
> +    if (chr->chr_close) {
>          chr->chr_close(chr);
> +    }
>      g_free(chr->filename);
>      g_free(chr->label);
> +    if (chr->opts) {
> +        qemu_opts_del(chr->opts);
> +    }
>      g_free(chr);
>  }
>  
> 

[Qemu-devel] [PATCH 03/11] chardev: reduce chardev ifdef mess a bit

[Gerd Hoffmann]

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

diff --git a/qemu-char.c b/qemu-char.c
index a29c2bb..c511de3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -856,6 +856,8 @@ static void cfmakeraw (struct termios *termios_p)
     || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) \
     || defined(__GLIBC__)
 
+#define HAVE_CHARDEV_TTY 1
+
 typedef struct {
     int fd;
     int connected;
@@ -1244,14 +1246,12 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
     chr->chr_close = qemu_chr_close_tty;
     return chr;
 }
-#else  /* ! __linux__ && ! __sun__ */
-static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
-{
-    return NULL;
-}
 #endif /* __linux__ || __sun__ */
 
 #if defined(__linux__)
+
+#define HAVE_CHARDEV_PARPORT 1
+
 typedef struct {
     int fd;
     int mode;
@@ -1395,6 +1395,9 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
 #endif /* __linux__ */
 
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__)
+
+#define HAVE_CHARDEV_PARPORT 1
+
 static int pp_ioctl(CharDriverState *chr, int cmd, void *arg)
 {
     int fd = (int)(intptr_t)chr->opaque;
@@ -2755,19 +2758,16 @@ static const struct {
 #else
     { .name = "file",      .open = qemu_chr_open_file_out },
     { .name = "pipe",      .open = qemu_chr_open_pipe },
-    { .name = "pty",       .open = qemu_chr_open_pty },
     { .name = "stdio",     .open = qemu_chr_open_stdio },
 #endif
 #ifdef CONFIG_BRLAPI
     { .name = "braille",   .open = chr_baum_init },
 #endif
-#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \
-    || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) \
-    || defined(__FreeBSD_kernel__)
+#ifdef HAVE_CHARDEV_TTY
     { .name = "tty",       .open = qemu_chr_open_tty },
+    { .name = "pty",       .open = qemu_chr_open_pty },
 #endif
-#if defined(__linux__) || defined(__FreeBSD__) || defined(__DragonFly__) \
-    || defined(__FreeBSD_kernel__)
+#ifdef HAVE_CHARDEV_PARPORT
     { .name = "parport",   .open = qemu_chr_open_pp },
 #endif
 #ifdef CONFIG_SPICE
-- 
1.7.1

Re: [Qemu-devel] [PATCH 03/11] chardev: reduce chardev ifdef mess a bit

[Paolo Bonzini]

Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> ---
>  qemu-char.c |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index a29c2bb..c511de3 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -856,6 +856,8 @@ static void cfmakeraw (struct termios *termios_p)
>      || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) \
>      || defined(__GLIBC__)
>  
> +#define HAVE_CHARDEV_TTY 1
> +
>  typedef struct {
>      int fd;
>      int connected;
> @@ -1244,14 +1246,12 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
>      chr->chr_close = qemu_chr_close_tty;
>      return chr;
>  }
> -#else  /* ! __linux__ && ! __sun__ */
> -static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
> -{
> -    return NULL;
> -}
>  #endif /* __linux__ || __sun__ */
>  
>  #if defined(__linux__)
> +
> +#define HAVE_CHARDEV_PARPORT 1
> +
>  typedef struct {
>      int fd;
>      int mode;
> @@ -1395,6 +1395,9 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
>  #endif /* __linux__ */
>  
>  #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__)
> +
> +#define HAVE_CHARDEV_PARPORT 1
> +
>  static int pp_ioctl(CharDriverState *chr, int cmd, void *arg)
>  {
>      int fd = (int)(intptr_t)chr->opaque;
> @@ -2755,19 +2758,16 @@ static const struct {
>  #else
>      { .name = "file",      .open = qemu_chr_open_file_out },
>      { .name = "pipe",      .open = qemu_chr_open_pipe },
> -    { .name = "pty",       .open = qemu_chr_open_pty },
>      { .name = "stdio",     .open = qemu_chr_open_stdio },
>  #endif
>  #ifdef CONFIG_BRLAPI
>      { .name = "braille",   .open = chr_baum_init },
>  #endif
> -#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \
> -    || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) \
> -    || defined(__FreeBSD_kernel__)
> +#ifdef HAVE_CHARDEV_TTY
>      { .name = "tty",       .open = qemu_chr_open_tty },
> +    { .name = "pty",       .open = qemu_chr_open_pty },
>  #endif
> -#if defined(__linux__) || defined(__FreeBSD__) || defined(__DragonFly__) \
> -    || defined(__FreeBSD_kernel__)
> +#ifdef HAVE_CHARDEV_PARPORT
>      { .name = "parport",   .open = qemu_chr_open_pp },
>  #endif
>  #ifdef CONFIG_SPICE
> 

[Qemu-devel] [PATCH 04/11] chardev: add qmp hotplug commands, with null chardev support

[Gerd Hoffmann]

Add chardev-add and chardev-remove qmp commands.  Hotplugging
a null chardev is supported for now, more will be added later.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi-schema.json |   32 ++++++++++++++++++++++++++++++++
 qemu-char.c      |   39 +++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..e3f0d44 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3017,3 +3017,35 @@
 # 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.4
+##
+{ 'type': 'ChardevDummy', 'data': { } }
+
+{ 'union': 'ChardevBackend', 'data': { 'null' : 'ChardevDummy' } }
+
+{ '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.4
+##
+{ 'command': 'chardev-remove', 'data': {'id': 'str'} }
diff --git a/qemu-char.c b/qemu-char.c
index c511de3..c9ae8f3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2938,3 +2938,42 @@ CharDriverState *qemu_char_get_next_serial(void)
     return serial_hds[next_serial++];
 }
 
+void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
+{
+    CharDriverState *chr;
+
+    switch (backend->kind) {
+    case CHARDEV_BACKEND_KIND_NULL:
+        chr = qemu_chr_open_null(NULL);
+        break;
+    default:
+        error_setg(errp, "unknown chardev backend (%d)", backend->kind);
+        return;
+    }
+
+    if (chr == NULL) {
+        error_setg(errp, "Failed to create chardev");
+    }
+    if (chr) {
+        chr->label = g_strdup(id);
+        chr->avail_connections = 1;
+        QTAILQ_INSERT_TAIL(&chardevs, chr, next);
+    }
+}
+
+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", id);
+        return;
+    }
+    if (chr->chr_can_read || chr->chr_read ||
+        chr->chr_event || chr->handler_opaque) {
+        error_setg(errp, "Chardev '%s' is busy", id);
+        return;
+    }
+    qemu_chr_delete(chr);
+}
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5c692d0..11d5a1b 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  = "id:s,backend:q",
+        .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  = "id:s",
+        .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 04/11] chardev: add qmp hotplug commands, with null chardev support

[Eric Blake]

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

On 01/07/2013 06:55 AM, Gerd Hoffmann wrote:
> Add chardev-add and chardev-remove qmp commands.  Hotplugging
> a null chardev is supported for now, more will be added later.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi-schema.json |   32 ++++++++++++++++++++++++++++++++
q>  qemu-char.c      |   39 +++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 121 insertions(+), 0 deletions(-)

> +##
> +# @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.4
> +##
> +{ 'type': 'ChardevDummy', 'data': { } }
> +
> +{ 'union': 'ChardevBackend', 'data': { 'null' : 'ChardevDummy' } }

Don't we usually document types independently from their usage, so that
they aren't appearing in between the documentation for chardev-add and
its actual implementation?

> +
> +{ 'command': 'chardev-add', 'data': {'id'      : 'str',
> +                                     'backend' : 'ChardevBackend' } }
> +

> +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": {} }

Let's get that FIXME dealt with:

-> { "execute" : "chardev-add",
     "arguments" : { "id" : "foo",
                     "backend" : { "type" : "null" } } }
<- { "return": {} }

if I'm reading the spec correctly.  Or does it have to be more verbose?

-> { "execute" : "chardev-add",
     "arguments" : { "id" : "foo",
                     "backend" : { "type" : "null", "data" : {} } } }
<- { "return": {} }

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

Re: [Qemu-devel] [PATCH 04/11] chardev: add qmp hotplug commands, with null chardev support

[Gerd Hoffmann]

  Hi,

>> +-> { "execute"   : "chardev-add", +     "arguments" : { "id"   :
>> "foo", +                     FIXME } } +<- { "return": {} }
> 
> Let's get that FIXME dealt with:
> 
> -> { "execute" : "chardev-add", "arguments" : { "id" : "foo", 
> "backend" : { "type" : "null" } } } <- { "return": {} }
> 
> if I'm reading the spec correctly.  Or does it have to be more
> verbose?
> 
> -> { "execute" : "chardev-add", "arguments" : { "id" : "foo", 
> "backend" : { "type" : "null", "data" : {} } } } <- { "return": {}
> }

I've tested with the long form, dunno whenever the qemu marshaller
also accepts the short one.

cheers,
  Gerd

[Qemu-devel] [PATCH 05/11] chardev: add hmp hotplug commands

[Gerd Hoffmann]

Add chardev-add and chardev-remove commands to the human monitor.
chardev-add accepts the same syntax as -chardev, chardev-remove
expects a chardev id.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
 hmp.c           |   23 +++++++++++++++++++++++
 hmp.h           |    2 ++
 3 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 010b8c9..67569ef 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1485,6 +1485,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 9e9e624..68929b4 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1336,3 +1336,26 @@ 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 *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 *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 21f3e05..700fbdc 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
-- 
1.7.1

Re: [Qemu-devel] [PATCH 05/11] chardev: add hmp hotplug commands

[Paolo Bonzini]

Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
> +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);

This ought to use qmp_chardev_add and a generic opts->ChardevBackend
conversion.

But IMHO, this kind of intermediate conversion is okay, with the
"correct" thing deferred; being able to play with hotplug from HMP is
worth the small wart.  It's really Luiz's decision, so I'm not giving
the reviewed-by (yet).

Paolo

> +    }
> +    hmp_handle_error(mon, &err);
> +}
> +

Re: [Qemu-devel] [PATCH 05/11] chardev: add hmp hotplug commands

[Gerd Hoffmann]

On 01/10/13 11:33, Paolo Bonzini wrote:
> Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
>> +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);
> 
> This ought to use qmp_chardev_add and a generic opts->ChardevBackend
> conversion.
> 
> But IMHO, this kind of intermediate conversion is okay, with the
> "correct" thing deferred; being able to play with hotplug from HMP is
> worth the small wart.  It's really Luiz's decision, so I'm not giving
> the reviewed-by (yet).

Once qmp_chardev_add() can handle everything supported by
qemu_chr_new_from_opts we can flip over, make qmp_chardev_add the
primary interface and qemu_chr_new_from_opts legacy (which then does the
opts->ChardevBackend conversion and calls qmp_chardev_add).

We are not there yet, even with the full series applied.
And even when we arrive there some day we don't have to touch
hmp_chardev_add when making the switch ;)

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 05/11] chardev: add hmp hotplug commands

[Paolo Bonzini]

Il 10/01/2013 11:53, Gerd Hoffmann ha scritto:
>> > 
>> > This ought to use qmp_chardev_add and a generic opts->ChardevBackend
>> > conversion.
>> > 
>> > But IMHO, this kind of intermediate conversion is okay, with the
>> > "correct" thing deferred; being able to play with hotplug from HMP is
>> > worth the small wart.  It's really Luiz's decision, so I'm not giving
>> > the reviewed-by (yet).
> Once qmp_chardev_add() can handle everything supported by
> qemu_chr_new_from_opts we can flip over, make qmp_chardev_add the
> primary interface and qemu_chr_new_from_opts legacy (which then does the
> opts->ChardevBackend conversion and calls qmp_chardev_add).
> 
> We are not there yet, even with the full series applied.

Yup.  Strange ones like msmouse, braille, etc. are missing.

> And even when we arrive there some day we don't have to touch
> hmp_chardev_add when making the switch ;)

Indeed.

Paolo

Re: [Qemu-devel] [PATCH 05/11] chardev: add hmp hotplug commands

[Luiz Capitulino]

On Thu, 10 Jan 2013 11:33:35 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
> > +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);
> 
> This ought to use qmp_chardev_add and a generic opts->ChardevBackend
> conversion.
> 
> But IMHO, this kind of intermediate conversion is okay, with the
> "correct" thing deferred; being able to play with hotplug from HMP is
> worth the small wart.  It's really Luiz's decision, so I'm not giving
> the reviewed-by (yet).

Fine with me. But please, add a comment if you respin.

[Qemu-devel] [PATCH 06/11] chardev: add file chardev support to chardev-add (qmp)

[Gerd Hoffmann]

Add support for file chardevs.  Output file is mandatory,
input file is optional.  Both file names and file descriptor
passing is supported.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi-schema.json |    9 +++++-
 qemu-char.c      |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index e3f0d44..8904d36 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3030,9 +3030,16 @@
 #
 # Since: 1.4
 ##
+{ 'union': 'ChardevFileSource', 'data': { 'path' : 'str',
+                                          'fd'   : 'str' } }
+
+{ 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
+                                   'out' : 'ChardevFileSource' } }
+
 { 'type': 'ChardevDummy', 'data': { } }
 
-{ 'union': 'ChardevBackend', 'data': { 'null' : 'ChardevDummy' } }
+{ 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
+                                       'null' : 'ChardevDummy' } }
 
 { 'command': 'chardev-add', 'data': {'id'      : 'str',
                                      'backend' : 'ChardevBackend' } }
diff --git a/qemu-char.c b/qemu-char.c
index c9ae8f3..91b0a57 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2938,11 +2938,86 @@ CharDriverState *qemu_char_get_next_serial(void)
     return serial_hds[next_serial++];
 }
 
+#ifdef _WIN32
+
+static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
+{
+    HANDLE out;
+
+    if (file->in) {
+        error_setg(errp, "input file not supported");
+        return NULL;
+    }
+    if (file->out->kind != CHARDEV_FILE_SOURCE_KIND_PATH) {
+        error_setg(errp, "only file paths supported");
+        return NULL;
+    }
+
+    out = CreateFile(file->out->path, GENERIC_WRITE, FILE_SHARE_READ, NULL,
+                     OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
+    if (out == INVALID_HANDLE_VALUE) {
+        error_setg(errp, "open %s failed", file->out->path);
+        return NULL;
+    }
+    return qemu_chr_open_win_file(out);
+}
+
+#else /* WIN32 */
+
+static int qmp_chardev_open_file_source(ChardevFileSource *src, int flags,
+                                        Error **errp)
+{
+    int fd = -1;
+
+    switch (src->kind) {
+    case CHARDEV_FILE_SOURCE_KIND_PATH:
+        TFR(fd = qemu_open(src->path, flags, 0666));
+        if (fd == -1) {
+            error_setg(errp, "open %s: %s", src->path, strerror(errno));
+        }
+        break;
+    case CHARDEV_FILE_SOURCE_KIND_FD:
+        fd = monitor_get_fd(cur_mon, src->fd, errp);
+        break;
+    default:
+        error_setg(errp, "unknown chardev file source (%d)", src->kind);
+        break;
+    }
+    return fd;
+}
+
+static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
+{
+    int flags, in = -1, out = -1;
+
+    flags = O_WRONLY | O_TRUNC | O_CREAT | O_BINARY;
+    out = qmp_chardev_open_file_source(file->out, flags, errp);
+    if (error_is_set(errp)) {
+        return NULL;
+    }
+
+    if (file->in) {
+        flags = O_RDONLY;
+        in = qmp_chardev_open_file_source(file->in, flags, errp);
+        if (error_is_set(errp)) {
+            qemu_close(out);
+            return NULL;
+        }
+    }
+
+    return qemu_chr_open_fd(in, out);
+}
+
+#endif /* WIN32 */
+
 void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
 {
     CharDriverState *chr;
 
     switch (backend->kind) {
+    case CHARDEV_BACKEND_KIND_FILE:
+        chr = qmp_chardev_open_file(backend->file, errp);
+        break;
     case CHARDEV_BACKEND_KIND_NULL:
         chr = qemu_chr_open_null(NULL);
         break;
@@ -2951,7 +3026,7 @@ void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
         return;
     }
 
-    if (chr == NULL) {
+    if (chr == NULL && !error_is_set(errp)) {
         error_setg(errp, "Failed to create chardev");
     }
     if (chr) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 06/11] chardev: add file chardev support to chardev-add (qmp)

[Eric Blake]

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

On 01/07/2013 06:55 AM, Gerd Hoffmann wrote:
> Add support for file chardevs.  Output file is mandatory,
> input file is optional.  Both file names and file descriptor
> passing is supported.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi-schema.json |    9 +++++-
>  qemu-char.c      |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 84 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index e3f0d44..8904d36 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3030,9 +3030,16 @@
>  #
>  # Since: 1.4
>  ##
> +{ 'union': 'ChardevFileSource', 'data': { 'path' : 'str',
> +                                          'fd'   : 'str' } }
> +
> +{ 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
> +                                   'out' : 'ChardevFileSource' } }
> +
>  { 'type': 'ChardevDummy', 'data': { } }
>  
> -{ 'union': 'ChardevBackend', 'data': { 'null' : 'ChardevDummy' } }
> +{ 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
> +                                       'null' : 'ChardevDummy' } }
>  
>  { 'command': 'chardev-add', 'data': {'id'      : 'str',
>                                       'backend' : 'ChardevBackend' } }

An example in qmp-commands.hx would be helpful; am I correct that an
example would be:

-> { "execute" : "chardev-add",
     "arguments" : { "id" : "foo",
                     "backend" : { "type" : "file", "data" : {
         "in"  : { "type" : "fd", "data" : "namedfd" },
         "out" : { "type" : "path", "data" : "/path/to/file" } } } } }
<- { "return": {} }

where namedfd was previously given via 'getfd'?

Do we need the complexity of supporting fd passing explicitly?  'getfd'
is less than ideal compared to 'add-fd', and for 'add-fd', we would pass
via "path":"/dev/fdset/nnn".  That is, why do we need to bend over
backwards to support an alternate syntax for fd passing in a new
command, when we can already use existing commands to get fd passing for
free?

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

Re: [Qemu-devel] [PATCH 06/11] chardev: add file chardev support to chardev-add (qmp)

[Gerd Hoffmann]

On 01/09/13 19:12, Eric Blake wrote:
> On 01/07/2013 06:55 AM, Gerd Hoffmann wrote:
>> Add support for file chardevs.  Output file is mandatory, input
>> file is optional.  Both file names and file descriptor passing is
>> supported.
>> 
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- 
>> qapi-schema.json |    9 +++++- qemu-char.c      |   77
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files
>> changed, 84 insertions(+), 2 deletions(-)
>> 
>> diff --git a/qapi-schema.json b/qapi-schema.json index
>> e3f0d44..8904d36 100644 --- a/qapi-schema.json +++
>> b/qapi-schema.json @@ -3030,9 +3030,16 @@ # # Since: 1.4 ## +{
>> 'union': 'ChardevFileSource', 'data': { 'path' : 'str', +
>> 'fd'   : 'str' } } + +{ 'type': 'ChardevFile', 'data': { '*in' :
>> 'ChardevFileSource', +                                   'out' :
>> 'ChardevFileSource' } } + { 'type': 'ChardevDummy', 'data': { }
>> }
>> 
>> -{ 'union': 'ChardevBackend', 'data': { 'null' : 'ChardevDummy' }
>> } +{ 'union': 'ChardevBackend', 'data': { 'file' :
>> 'ChardevFile', +                                       'null' :
>> 'ChardevDummy' } }
>> 
>> { 'command': 'chardev-add', 'data': {'id'      : 'str', 'backend'
>> : 'ChardevBackend' } }
> 
> An example in qmp-commands.hx would be helpful; am I correct that
> an example would be:
> 
> -> { "execute" : "chardev-add", "arguments" : { "id" : "foo", 
> "backend" : { "type" : "file", "data" : { "in"  : { "type" : "fd",
> "data" : "namedfd" }, "out" : { "type" : "path", "data" :
> "/path/to/file" } } } } } <- { "return": {} }
> 
> where namedfd was previously given via 'getfd'?

Yes.

> Do we need the complexity of supporting fd passing explicitly?
> 'getfd' is less than ideal compared to 'add-fd', and for 'add-fd',
> we would pass via "path":"/dev/fdset/nnn".  That is, why do we need
> to bend over backwards to support an alternate syntax for fd
> passing in a new command, when we can already use existing commands
> to get fd passing for free?

Oh, didn't know that.  Was just following what SocketAddress does and
what Paolo suggested.  It isn't needed indeed.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 06/11] chardev: add file chardev support to chardev-add (qmp)

[Paolo Bonzini]

Il 10/01/2013 10:10, Gerd Hoffmann ha scritto:
> 
>> > Do we need the complexity of supporting fd passing explicitly?
>> > 'getfd' is less than ideal compared to 'add-fd', and for 'add-fd',
>> > we would pass via "path":"/dev/fdset/nnn".  That is, why do we need
>> > to bend over backwards to support an alternate syntax for fd
>> > passing in a new command, when we can already use existing commands
>> > to get fd passing for free?
> Oh, didn't know that.  Was just following what SocketAddress does and
> what Paolo suggested.  It isn't needed indeed.

Yeah, I hadn't followed fdset either.  We really need more documentation.

Paolo

[Qemu-devel] [PATCH 07/11] chardev: add tty chardev support to chardev-add (qmp)

[Gerd Hoffmann]

Simliar to file, except that no separate in/out files are supported
because it's pointless for direct device access.  Also the special
tty ioctl hooks (pass through linespeed settings etc) are activated.
Both file names and file descriptor passing is supported.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi-schema.json |    6 ++++++
 qemu-char.c      |   53 +++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 8904d36..7e5c8c2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3036,9 +3036,15 @@
 { 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
                                    'out' : 'ChardevFileSource' } }
 
+{ 'enum': 'ChardevPortKind', 'data': [ 'tty' ] }
+
+{ 'type': 'ChardevPort', 'data': { 'device' : 'ChardevFileSource',
+                                   'type'   : 'ChardevPortKind'} }
+
 { 'type': 'ChardevDummy', 'data': { } }
 
 { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
+                                       'port' : 'ChardevPort',
                                        'null' : 'ChardevDummy' } }
 
 { 'command': 'chardev-add', 'data': {'id'      : 'str',
diff --git a/qemu-char.c b/qemu-char.c
index 91b0a57..764321b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1230,21 +1230,27 @@ static void qemu_chr_close_tty(CharDriverState *chr)
     }
 }
 
+static CharDriverState *qemu_chr_open_tty_fd(int fd)
+{
+    CharDriverState *chr;
+
+    tty_serial_init(fd, 115200, 'N', 8, 1);
+    chr = qemu_chr_open_fd(fd, fd);
+    chr->chr_ioctl = tty_serial_ioctl;
+    chr->chr_close = qemu_chr_close_tty;
+    return chr;
+}
+
 static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
 {
     const char *filename = qemu_opt_get(opts, "path");
-    CharDriverState *chr;
     int fd;
 
     TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK));
     if (fd < 0) {
         return NULL;
     }
-    tty_serial_init(fd, 115200, 'N', 8, 1);
-    chr = qemu_chr_open_fd(fd, fd);
-    chr->chr_ioctl = tty_serial_ioctl;
-    chr->chr_close = qemu_chr_close_tty;
-    return chr;
+    return qemu_chr_open_tty_fd(fd);
 }
 #endif /* __linux__ || __sun__ */
 
@@ -2962,6 +2968,15 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
     return qemu_chr_open_win_file(out);
 }
 
+static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
+{
+    switch (port->type) {
+    default:
+        error_setg(errp, "unknown chardev port (%d)", port->type);
+        return NULL;
+    }
+}
+
 #else /* WIN32 */
 
 static int qmp_chardev_open_file_source(ChardevFileSource *src, int flags,
@@ -2978,6 +2993,9 @@ static int qmp_chardev_open_file_source(ChardevFileSource *src, int flags,
         break;
     case CHARDEV_FILE_SOURCE_KIND_FD:
         fd = monitor_get_fd(cur_mon, src->fd, errp);
+        if (flags & O_NONBLOCK) {
+            socket_set_nonblock(fd);
+        }
         break;
     default:
         error_setg(errp, "unknown chardev file source (%d)", src->kind);
@@ -3008,6 +3026,26 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
     return qemu_chr_open_fd(in, out);
 }
 
+static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
+{
+    int flags, fd;
+
+    switch (port->type) {
+#ifdef HAVE_CHARDEV_TTY
+    case CHARDEV_PORT_KIND_TTY:
+        flags = O_RDWR | O_NONBLOCK;
+        fd = qmp_chardev_open_file_source(port->device, flags, errp);
+        if (error_is_set(errp)) {
+            return NULL;
+        }
+        return qemu_chr_open_tty_fd(fd);
+#endif
+    default:
+        error_setg(errp, "unknown chardev port (%d)", port->type);
+        return NULL;
+    }
+}
+
 #endif /* WIN32 */
 
 void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
@@ -3018,6 +3056,9 @@ void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
     case CHARDEV_BACKEND_KIND_FILE:
         chr = qmp_chardev_open_file(backend->file, errp);
         break;
+    case CHARDEV_BACKEND_KIND_PORT:
+        chr = qmp_chardev_open_port(backend->port, errp);
+        break;
     case CHARDEV_BACKEND_KIND_NULL:
         chr = qemu_chr_open_null(NULL);
         break;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 07/11] chardev: add tty chardev support to chardev-add (qmp)

[Paolo Bonzini]

Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
> Simliar to file, except that no separate in/out files are supported
> because it's pointless for direct device access.  Also the special
> tty ioctl hooks (pass through linespeed settings etc) are activated.
> Both file names and file descriptor passing is supported.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi-schema.json |    6 ++++++
>  qemu-char.c      |   53 +++++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8904d36..7e5c8c2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3036,9 +3036,15 @@
>  { 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
>                                     'out' : 'ChardevFileSource' } }
>  
> +{ 'enum': 'ChardevPortKind', 'data': [ 'tty' ] }
> +
> +{ 'type': 'ChardevPort', 'data': { 'device' : 'ChardevFileSource',
> +                                   'type'   : 'ChardevPortKind'} }
> +
>  { 'type': 'ChardevDummy', 'data': { } }
>  
>  { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
> +                                       'port' : 'ChardevPort',
>                                         'null' : 'ChardevDummy' } }
>  
>  { 'command': 'chardev-add', 'data': {'id'      : 'str',
> diff --git a/qemu-char.c b/qemu-char.c
> index 91b0a57..764321b 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1230,21 +1230,27 @@ static void qemu_chr_close_tty(CharDriverState *chr)
>      }
>  }
>  
> +static CharDriverState *qemu_chr_open_tty_fd(int fd)
> +{
> +    CharDriverState *chr;
> +
> +    tty_serial_init(fd, 115200, 'N', 8, 1);
> +    chr = qemu_chr_open_fd(fd, fd);
> +    chr->chr_ioctl = tty_serial_ioctl;
> +    chr->chr_close = qemu_chr_close_tty;
> +    return chr;
> +}
> +
>  static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
>  {
>      const char *filename = qemu_opt_get(opts, "path");
> -    CharDriverState *chr;
>      int fd;
>  
>      TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK));
>      if (fd < 0) {
>          return NULL;
>      }
> -    tty_serial_init(fd, 115200, 'N', 8, 1);
> -    chr = qemu_chr_open_fd(fd, fd);
> -    chr->chr_ioctl = tty_serial_ioctl;
> -    chr->chr_close = qemu_chr_close_tty;
> -    return chr;
> +    return qemu_chr_open_tty_fd(fd);
>  }
>  #endif /* __linux__ || __sun__ */
>  
> @@ -2962,6 +2968,15 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
>      return qemu_chr_open_win_file(out);
>  }
>  
> +static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
> +{
> +    switch (port->type) {
> +    default:
> +        error_setg(errp, "unknown chardev port (%d)", port->type);
> +        return NULL;
> +    }
> +}
> +
>  #else /* WIN32 */
>  
>  static int qmp_chardev_open_file_source(ChardevFileSource *src, int flags,
> @@ -2978,6 +2993,9 @@ static int qmp_chardev_open_file_source(ChardevFileSource *src, int flags,
>          break;
>      case CHARDEV_FILE_SOURCE_KIND_FD:
>          fd = monitor_get_fd(cur_mon, src->fd, errp);
> +        if (flags & O_NONBLOCK) {
> +            socket_set_nonblock(fd);
> +        }
>          break;
>      default:
>          error_setg(errp, "unknown chardev file source (%d)", src->kind);
> @@ -3008,6 +3026,26 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
>      return qemu_chr_open_fd(in, out);
>  }
>  
> +static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
> +{
> +    int flags, fd;
> +
> +    switch (port->type) {
> +#ifdef HAVE_CHARDEV_TTY
> +    case CHARDEV_PORT_KIND_TTY:
> +        flags = O_RDWR | O_NONBLOCK;
> +        fd = qmp_chardev_open_file_source(port->device, flags, errp);
> +        if (error_is_set(errp)) {
> +            return NULL;
> +        }
> +        return qemu_chr_open_tty_fd(fd);
> +#endif
> +    default:
> +        error_setg(errp, "unknown chardev port (%d)", port->type);
> +        return NULL;
> +    }
> +}
> +
>  #endif /* WIN32 */
>  
>  void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
> @@ -3018,6 +3056,9 @@ void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
>      case CHARDEV_BACKEND_KIND_FILE:
>          chr = qmp_chardev_open_file(backend->file, errp);
>          break;
> +    case CHARDEV_BACKEND_KIND_PORT:
> +        chr = qmp_chardev_open_port(backend->port, errp);
> +        break;
>      case CHARDEV_BACKEND_KIND_NULL:
>          chr = qemu_chr_open_null(NULL);
>          break;
> 

Apart from the naming question (see my review of 8/11),

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

[Qemu-devel] [PATCH 08/11] chardev: add serial chardev support to chardev-add (qmp)

[Gerd Hoffmann]

Add support for serial chardevs.  Windup qemu_chr_open_win() on
Windows, alias to 'tty' on Linux.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi-schema.json |    3 ++-
 qemu-char.c      |   17 +++++++++++++++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 7e5c8c2..d833385 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3036,7 +3036,8 @@
 { 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
                                    'out' : 'ChardevFileSource' } }
 
-{ 'enum': 'ChardevPortKind', 'data': [ 'tty' ] }
+{ 'enum': 'ChardevPortKind', 'data': [ 'tty',
+                                       'serial' ] }
 
 { 'type': 'ChardevPort', 'data': { 'device' : 'ChardevFileSource',
                                    'type'   : 'ChardevPortKind'} }
diff --git a/qemu-char.c b/qemu-char.c
index 764321b..4232fea 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1672,9 +1672,8 @@ static int win_chr_poll(void *opaque)
     return 0;
 }
 
-static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
+static CharDriverState *qemu_chr_open_win_path(const char *filename)
 {
-    const char *filename = qemu_opt_get(opts, "path");
     CharDriverState *chr;
     WinCharState *s;
 
@@ -1693,6 +1692,11 @@ static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
     return chr;
 }
 
+static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
+{
+    return qemu_chr_open_win_path(qemu_opt_get(opts, "path"));
+}
+
 static int win_chr_pipe_poll(void *opaque)
 {
     CharDriverState *chr = opaque;
@@ -2771,6 +2775,7 @@ static const struct {
 #endif
 #ifdef HAVE_CHARDEV_TTY
     { .name = "tty",       .open = qemu_chr_open_tty },
+    { .name = "serial",    .open = qemu_chr_open_tty },
     { .name = "pty",       .open = qemu_chr_open_pty },
 #endif
 #ifdef HAVE_CHARDEV_PARPORT
@@ -2970,7 +2975,14 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
 
 static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
 {
+    if (port->device->kind != CHARDEV_FILE_SOURCE_KIND_PATH) {
+        error_setg(errp, "only file paths supported");
+        return NULL;
+    }
+
     switch (port->type) {
+    case CHARDEV_PORT_KIND_SERIAL:
+        return qemu_chr_open_win_path(port->device->path);
     default:
         error_setg(errp, "unknown chardev port (%d)", port->type);
         return NULL;
@@ -3033,6 +3045,7 @@ static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
     switch (port->type) {
 #ifdef HAVE_CHARDEV_TTY
     case CHARDEV_PORT_KIND_TTY:
+    case CHARDEV_PORT_KIND_SERIAL:
         flags = O_RDWR | O_NONBLOCK;
         fd = qmp_chardev_open_file_source(port->device, flags, errp);
         if (error_is_set(errp)) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 08/11] chardev: add serial chardev support to chardev-add (qmp)

[Paolo Bonzini]

Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
> Add support for serial chardevs.  Windup qemu_chr_open_win() on
> Windows, alias to 'tty' on Linux.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi-schema.json |    3 ++-
>  qemu-char.c      |   17 +++++++++++++++--
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7e5c8c2..d833385 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3036,7 +3036,8 @@
>  { 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
>                                     'out' : 'ChardevFileSource' } }
>  
> -{ 'enum': 'ChardevPortKind', 'data': [ 'tty' ] }
> +{ 'enum': 'ChardevPortKind', 'data': [ 'tty',
> +                                       'serial' ] }

Can we just use one name, either tty or serial?  And for the QemuOpts
version, alias them too.

Paolo

>  
>  { 'type': 'ChardevPort', 'data': { 'device' : 'ChardevFileSource',
>                                     'type'   : 'ChardevPortKind'} }
> diff --git a/qemu-char.c b/qemu-char.c
> index 764321b..4232fea 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1672,9 +1672,8 @@ static int win_chr_poll(void *opaque)
>      return 0;
>  }
>  
> -static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
> +static CharDriverState *qemu_chr_open_win_path(const char *filename)
>  {
> -    const char *filename = qemu_opt_get(opts, "path");
>      CharDriverState *chr;
>      WinCharState *s;
>  
> @@ -1693,6 +1692,11 @@ static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
>      return chr;
>  }
>  
> +static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
> +{
> +    return qemu_chr_open_win_path(qemu_opt_get(opts, "path"));
> +}
> +
>  static int win_chr_pipe_poll(void *opaque)
>  {
>      CharDriverState *chr = opaque;
> @@ -2771,6 +2775,7 @@ static const struct {
>  #endif
>  #ifdef HAVE_CHARDEV_TTY
>      { .name = "tty",       .open = qemu_chr_open_tty },
> +    { .name = "serial",    .open = qemu_chr_open_tty },
>      { .name = "pty",       .open = qemu_chr_open_pty },
>  #endif
>  #ifdef HAVE_CHARDEV_PARPORT
> @@ -2970,7 +2975,14 @@ static CharDriverState *qmp_chardev_open_file(ChardevFile *file, Error **errp)
>  
>  static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
>  {
> +    if (port->device->kind != CHARDEV_FILE_SOURCE_KIND_PATH) {
> +        error_setg(errp, "only file paths supported");
> +        return NULL;
> +    }
> +
>      switch (port->type) {
> +    case CHARDEV_PORT_KIND_SERIAL:
> +        return qemu_chr_open_win_path(port->device->path);
>      default:
>          error_setg(errp, "unknown chardev port (%d)", port->type);
>          return NULL;
> @@ -3033,6 +3045,7 @@ static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
>      switch (port->type) {
>  #ifdef HAVE_CHARDEV_TTY
>      case CHARDEV_PORT_KIND_TTY:
> +    case CHARDEV_PORT_KIND_SERIAL:
>          flags = O_RDWR | O_NONBLOCK;
>          fd = qmp_chardev_open_file_source(port->device, flags, errp);
>          if (error_is_set(errp)) {
> 

Re: [Qemu-devel] [PATCH 08/11] chardev: add serial chardev support to chardev-add (qmp)

[Gerd Hoffmann]

  Hi,

>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 7e5c8c2..d833385 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3036,7 +3036,8 @@
>>  { 'type': 'ChardevFile', 'data': { '*in' : 'ChardevFileSource',
>>                                     'out' : 'ChardevFileSource' } }
>>  
>> -{ 'enum': 'ChardevPortKind', 'data': [ 'tty' ] }
>> +{ 'enum': 'ChardevPortKind', 'data': [ 'tty',
>> +                                       'serial' ] }
> 
> Can we just use one name, either tty or serial?

Both are in use today, 'serial' on windows and 'tty' on linux.

'tty' on windows doesn't make sense.

'serial' on linux does, but it actually is a subset of 'tty' and needs
no special care, thats why I simply aliased it for convenience.

We could switch to use 'serial' only for qmp, but I think that will be
confusing as the old 'tty' name will have to stay for -chardev.

We could just not do the alias thing and continue to use the 'serial' on
windows + 'tty' on linux (posix) scheme.

> And for the QemuOpts
> version, alias them too.

Did that ...

>>  #ifdef HAVE_CHARDEV_TTY
>>      { .name = "tty",       .open = qemu_chr_open_tty },
>> +    { .name = "serial",    .open = qemu_chr_open_tty },
>>      { .name = "pty",       .open = qemu_chr_open_pty },
>>  #endif

... here.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 08/11] chardev: add serial chardev support to chardev-add (qmp)

[Gerd Hoffmann]

  Hi,

>> Can we just use one name, either tty or serial?

Ah, after reading the parport patch comment I think I understand what
you suggest:

 (1) Standardize on 'serial' + 'parallel' on the new qmp interface.
 (2) Support these names on -chardev too.
 (3) Alias the old names to the new ones for -chardev for backward
     compatibility.

correct?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 08/11] chardev: add serial chardev support to chardev-add (qmp)

[Paolo Bonzini]

Il 10/01/2013 13:37, Gerd Hoffmann ha scritto:
>>> >> Can we just use one name, either tty or serial?
> Ah, after reading the parport patch comment I think I understand what
> you suggest:
> 
>  (1) Standardize on 'serial' + 'parallel' on the new qmp interface.

Or tty + parallel, as you see more fit.

>  (2) Support these names on -chardev too.
>  (3) Alias the old names to the new ones for -chardev for backward
>      compatibility.

Yes, exactly.

Paolo

[Qemu-devel] [PATCH 09/11] chardev: add parport chardev support to chardev-add (qmp)

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi-schema.json |    3 ++-
 qemu-char.c      |   43 +++++++++++++++++++++++++++----------------
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index d833385..0922823 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3037,7 +3037,8 @@
                                    'out' : 'ChardevFileSource' } }
 
 { 'enum': 'ChardevPortKind', 'data': [ 'tty',
-                                       'serial' ] }
+                                       'serial',
+                                       'parport' ] }
 
 { 'type': 'ChardevPort', 'data': { 'device' : 'ChardevFileSource',
                                    'type'   : 'ChardevPortKind'} }
diff --git a/qemu-char.c b/qemu-char.c
index 4232fea..5cbf44e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1367,17 +1367,10 @@ static void pp_close(CharDriverState *chr)
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
-static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
+static CharDriverState *qemu_chr_open_pp_fd(int fd)
 {
-    const char *filename = qemu_opt_get(opts, "path");
     CharDriverState *chr;
     ParallelCharDriver *drv;
-    int fd;
-
-    TFR(fd = qemu_open(filename, O_RDWR));
-    if (fd < 0) {
-        return NULL;
-    }
 
     if (ioctl(fd, PPCLAIM) < 0) {
         close(fd);
@@ -1441,16 +1434,9 @@ static int pp_ioctl(CharDriverState *chr, int cmd, void *arg)
     return 0;
 }
 
-static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
+static CharDriverState *qemu_chr_open_pp_fd(int fd)
 {
-    const char *filename = qemu_opt_get(opts, "path");
     CharDriverState *chr;
-    int fd;
-
-    fd = qemu_open(filename, O_RDWR);
-    if (fd < 0) {
-        return NULL;
-    }
 
     chr = g_malloc0(sizeof(CharDriverState));
     chr->opaque = (void *)(intptr_t)fd;
@@ -2750,6 +2736,22 @@ fail:
     return NULL;
 }
 
+#ifdef HAVE_CHARDEV_PARPORT
+
+static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
+{
+    const char *filename = qemu_opt_get(opts, "path");
+    int fd;
+
+    fd = qemu_open(filename, O_RDWR);
+    if (fd < 0) {
+        return NULL;
+    }
+    return qemu_chr_open_pp_fd(fd);
+}
+
+#endif
+
 static const struct {
     const char *name;
     CharDriverState *(*open)(QemuOpts *opts);
@@ -3053,6 +3055,15 @@ static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
         }
         return qemu_chr_open_tty_fd(fd);
 #endif
+#ifdef HAVE_CHARDEV_PARPORT
+    case CHARDEV_PORT_KIND_PARPORT:
+        flags = O_RDWR;
+        fd = qmp_chardev_open_file_source(port->device, flags, errp);
+        if (error_is_set(errp)) {
+            return NULL;
+        }
+        return qemu_chr_open_pp_fd(fd);
+#endif
     default:
         error_setg(errp, "unknown chardev port (%d)", port->type);
         return NULL;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 09/11] chardev: add parport chardev support to chardev-add (qmp)

[Paolo Bonzini]

Il 07/01/2013 14:55, Gerd Hoffmann ha scritto:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi-schema.json |    3 ++-
>  qemu-char.c      |   43 +++++++++++++++++++++++++++----------------
>  2 files changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index d833385..0922823 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3037,7 +3037,8 @@
>                                     'out' : 'ChardevFileSource' } }
>  
>  { 'enum': 'ChardevPortKind', 'data': [ 'tty',
> -                                       'serial' ] }
> +                                       'serial',
> +                                       'parport' ] }

I'd say either ['tty', 'parport'] or ['serial', 'parallel'].  In the
latter case, parallel could be an alias of parport also in the QemuOpts
case.

Apart from this,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

>  
>  { 'type': 'ChardevPort', 'data': { 'device' : 'ChardevFileSource',
>                                     'type'   : 'ChardevPortKind'} }
> diff --git a/qemu-char.c b/qemu-char.c
> index 4232fea..5cbf44e 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1367,17 +1367,10 @@ static void pp_close(CharDriverState *chr)
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>  }
>  
> -static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
> +static CharDriverState *qemu_chr_open_pp_fd(int fd)
>  {
> -    const char *filename = qemu_opt_get(opts, "path");
>      CharDriverState *chr;
>      ParallelCharDriver *drv;
> -    int fd;
> -
> -    TFR(fd = qemu_open(filename, O_RDWR));
> -    if (fd < 0) {
> -        return NULL;
> -    }
>  
>      if (ioctl(fd, PPCLAIM) < 0) {
>          close(fd);
> @@ -1441,16 +1434,9 @@ static int pp_ioctl(CharDriverState *chr, int cmd, void *arg)
>      return 0;
>  }
>  
> -static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
> +static CharDriverState *qemu_chr_open_pp_fd(int fd)
>  {
> -    const char *filename = qemu_opt_get(opts, "path");
>      CharDriverState *chr;
> -    int fd;
> -
> -    fd = qemu_open(filename, O_RDWR);
> -    if (fd < 0) {
> -        return NULL;
> -    }
>  
>      chr = g_malloc0(sizeof(CharDriverState));
>      chr->opaque = (void *)(intptr_t)fd;
> @@ -2750,6 +2736,22 @@ fail:
>      return NULL;
>  }
>  
> +#ifdef HAVE_CHARDEV_PARPORT
> +
> +static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
> +{
> +    const char *filename = qemu_opt_get(opts, "path");
> +    int fd;
> +
> +    fd = qemu_open(filename, O_RDWR);
> +    if (fd < 0) {
> +        return NULL;
> +    }
> +    return qemu_chr_open_pp_fd(fd);
> +}
> +
> +#endif
> +
>  static const struct {
>      const char *name;
>      CharDriverState *(*open)(QemuOpts *opts);
> @@ -3053,6 +3055,15 @@ static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
>          }
>          return qemu_chr_open_tty_fd(fd);
>  #endif
> +#ifdef HAVE_CHARDEV_PARPORT
> +    case CHARDEV_PORT_KIND_PARPORT:
> +        flags = O_RDWR;
> +        fd = qmp_chardev_open_file_source(port->device, flags, errp);
> +        if (error_is_set(errp)) {
> +            return NULL;
> +        }
> +        return qemu_chr_open_pp_fd(fd);
> +#endif
>      default:
>          error_setg(errp, "unknown chardev port (%d)", port->type);
>          return NULL;
> 

[Qemu-devel] [PATCH 10/11] chardev: add socket chardev support to chardev-add (qmp)

[Gerd Hoffmann]

qemu_chr_open_socket is splitted into two functions.  All initialization
after creating the socket file handler is splitted away into the new
qemu_chr_open_socket_fd function.

chr->filename doesn't get filled from QemuOpts any more.  Qemu gathers
the information using getsockname and getnameinfo instead.  This way it
will also work correctly for file handles passed via file descriptor
passing.

Finally qmp_chardev_open_socket() is the actual qmp hotplug
implementation which basically just calls socket_listen or
socket_connect and the new qemu_chr_open_socket_fd function.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi-schema.json |   13 +++-
 qemu-char.c      |  168 ++++++++++++++++++++++++++++++++++++-----------------
 2 files changed, 124 insertions(+), 57 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 0922823..39e0ab4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3043,11 +3043,18 @@
 { 'type': 'ChardevPort', 'data': { 'device' : 'ChardevFileSource',
                                    'type'   : 'ChardevPortKind'} }
 
+{ 'type': 'ChardevSocket', 'data': { 'addr'    : 'SocketAddress',
+                                     '*server' : 'bool',
+                                     '*wait'   : 'bool',
+                                     '*delay'  : 'bool',
+                                     '*telnet' : 'bool' } }
+
 { 'type': 'ChardevDummy', 'data': { } }
 
-{ 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
-                                       'port' : 'ChardevPort',
-                                       'null' : 'ChardevDummy' } }
+{ 'union': 'ChardevBackend', 'data': { 'file'   : 'ChardevFile',
+                                       'port'   : 'ChardevPort',
+                                       'socket' : 'ChardevSocket',
+                                       'null'   : 'ChardevDummy' } }
 
 { 'command': 'chardev-add', 'data': {'id'      : 'str',
                                      'backend' : 'ChardevBackend' } }
diff --git a/qemu-char.c b/qemu-char.c
index 5cbf44e..01d65e2 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2438,10 +2438,88 @@ static void tcp_chr_close(CharDriverState *chr)
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
-static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
+static CharDriverState *qemu_chr_open_socket_fd(int fd, int do_nodelay,
+                                                int is_listen, int is_telnet,
+                                                int is_waitconnect,
+                                                Error **errp)
 {
     CharDriverState *chr = NULL;
     TCPCharDriver *s = NULL;
+    char host[NI_MAXHOST], serv[NI_MAXSERV];
+    const char *left = "", *right = "";
+    struct sockaddr_storage ss;
+    socklen_t ss_len = sizeof(ss);
+
+    memset(&ss, 0, ss_len);
+    if (getsockname(fd, (struct sockaddr *) &ss, &ss_len) != 0) {
+        error_setg(errp, "getsockname: %s", strerror(errno));
+        return NULL;
+    }
+
+    chr = g_malloc0(sizeof(CharDriverState));
+    s = g_malloc0(sizeof(TCPCharDriver));
+
+    s->connected = 0;
+    s->fd = -1;
+    s->listen_fd = -1;
+    s->msgfd = -1;
+
+    chr->filename = g_malloc(256);
+    switch (ss.ss_family) {
+#ifndef _WIN32
+    case AF_UNIX:
+        s->is_unix = 1;
+        snprintf(chr->filename, 256, "unix:%s%s",
+                 ((struct sockaddr_un *)(&ss))->sun_path,
+                 is_listen ? ",server" : "");
+        break;
+#endif
+    case AF_INET6:
+        left  = "[";
+        right = "]";
+        /* fall through */
+    case AF_INET:
+        s->do_nodelay = do_nodelay;
+        getnameinfo((struct sockaddr *) &ss, ss_len, host, sizeof(host),
+                    serv, sizeof(serv), NI_NUMERICHOST | NI_NUMERICSERV);
+        snprintf(chr->filename, 256, "%s:%s:%s%s%s%s",
+                 is_telnet ? "telnet" : "tcp",
+                 left, host, right, serv,
+                 is_listen ? ",server" : "");
+        break;
+    }
+
+    chr->opaque = s;
+    chr->chr_write = tcp_chr_write;
+    chr->chr_close = tcp_chr_close;
+    chr->get_msgfd = tcp_get_msgfd;
+    chr->chr_add_client = tcp_chr_add_client;
+
+    if (is_listen) {
+        s->listen_fd = fd;
+        qemu_set_fd_handler2(s->listen_fd, NULL, tcp_chr_accept, NULL, chr);
+        if (is_telnet) {
+            s->do_telnetopt = 1;
+        }
+    } else {
+        s->connected = 1;
+        s->fd = fd;
+        socket_set_nodelay(fd);
+        tcp_chr_connect(chr);
+    }
+
+    if (is_listen && is_waitconnect) {
+        printf("QEMU waiting for connection on: %s\n",
+               chr->filename);
+        tcp_chr_accept(chr);
+        socket_set_nonblock(s->listen_fd);
+    }
+    return chr;
+}
+
+static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
+{
+    CharDriverState *chr = NULL;
     Error *local_err = NULL;
     int fd = -1;
     int is_listen;
@@ -2458,10 +2536,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     if (!is_listen)
         is_waitconnect = 0;
 
-    chr = g_malloc0(sizeof(CharDriverState));
-    s = g_malloc0(sizeof(TCPCharDriver));
-
-    if (is_unix) {
+     if (is_unix) {
         if (is_listen) {
             fd = unix_listen_opts(opts, &local_err);
         } else {
@@ -2481,56 +2556,14 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     if (!is_waitconnect)
         socket_set_nonblock(fd);
 
-    s->connected = 0;
-    s->fd = -1;
-    s->listen_fd = -1;
-    s->msgfd = -1;
-    s->is_unix = is_unix;
-    s->do_nodelay = do_nodelay && !is_unix;
-
-    chr->opaque = s;
-    chr->chr_write = tcp_chr_write;
-    chr->chr_close = tcp_chr_close;
-    chr->get_msgfd = tcp_get_msgfd;
-    chr->chr_add_client = tcp_chr_add_client;
-
-    if (is_listen) {
-        s->listen_fd = fd;
-        qemu_set_fd_handler2(s->listen_fd, NULL, tcp_chr_accept, NULL, chr);
-        if (is_telnet)
-            s->do_telnetopt = 1;
-
-    } else {
-        s->connected = 1;
-        s->fd = fd;
-        socket_set_nodelay(fd);
-        tcp_chr_connect(chr);
-    }
-
-    /* for "info chardev" monitor command */
-    chr->filename = g_malloc(256);
-    if (is_unix) {
-        snprintf(chr->filename, 256, "unix:%s%s",
-                 qemu_opt_get(opts, "path"),
-                 qemu_opt_get_bool(opts, "server", 0) ? ",server" : "");
-    } else if (is_telnet) {
-        snprintf(chr->filename, 256, "telnet:%s:%s%s",
-                 qemu_opt_get(opts, "host"), qemu_opt_get(opts, "port"),
-                 qemu_opt_get_bool(opts, "server", 0) ? ",server" : "");
-    } else {
-        snprintf(chr->filename, 256, "tcp:%s:%s%s",
-                 qemu_opt_get(opts, "host"), qemu_opt_get(opts, "port"),
-                 qemu_opt_get_bool(opts, "server", 0) ? ",server" : "");
-    }
-
-    if (is_listen && is_waitconnect) {
-        printf("QEMU waiting for connection on: %s\n",
-               chr->filename);
-        tcp_chr_accept(chr);
-        socket_set_nonblock(s->listen_fd);
+    chr = qemu_chr_open_socket_fd(fd, do_nodelay, is_listen, is_telnet,
+                                  is_waitconnect, &local_err);
+    if (error_is_set(&local_err)) {
+        goto fail;
     }
     return chr;
 
+
  fail:
     if (local_err) {
         qerror_report_err(local_err);
@@ -2539,8 +2572,10 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     if (fd >= 0) {
         closesocket(fd);
     }
-    g_free(s);
-    g_free(chr);
+    if (chr) {
+        g_free(chr->opaque);
+        g_free(chr);
+    }
     return NULL;
 }
 
@@ -3072,6 +3107,28 @@ static CharDriverState *qmp_chardev_open_port(ChardevPort *port, Error **errp)
 
 #endif /* WIN32 */
 
+static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
+                                                Error **errp)
+{
+    SocketAddress *addr = sock->addr;
+    bool do_nodelay     = sock->has_delay  ? !sock->delay : true;
+    bool is_listen      = sock->has_server ? sock->server : true;
+    bool is_telnet      = sock->has_telnet ? sock->telnet : false;
+    bool is_waitconnect = sock->has_wait   ? sock->wait   : false;
+    int fd;
+
+    if (is_listen) {
+        fd = socket_listen(addr, errp);
+    } else {
+        fd = socket_connect(addr, errp, NULL, NULL);
+    }
+    if (error_is_set(errp)) {
+        return NULL;
+    }
+    return qemu_chr_open_socket_fd(fd, do_nodelay, is_listen,
+                                   is_telnet, is_waitconnect, errp);
+}
+
 void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
 {
     CharDriverState *chr;
@@ -3083,6 +3140,9 @@ void qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp)
     case CHARDEV_BACKEND_KIND_PORT:
         chr = qmp_chardev_open_port(backend->port, errp);
         break;
+    case CHARDEV_BACKEND_KIND_SOCKET:
+        chr = qmp_chardev_open_socket(backend->socket, errp);
+        break;
     case CHARDEV_BACKEND_KIND_NULL:
         chr = qemu_chr_open_null(NULL);
         break;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 10/11] chardev: add socket chardev support to chardev-add (qmp)

[Eric Blake]

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

On 01/07/2013 06:55 AM, Gerd Hoffmann wrote:
> qemu_chr_open_socket is splitted into two functions.  All initialization

s/splitted/split/

> after creating the socket file handler is splitted away into the new

and again

> qemu_chr_open_socket_fd function.
> 
> chr->filename doesn't get filled from QemuOpts any more.  Qemu gathers
> the information using getsockname and getnameinfo instead.  This way it
> will also work correctly for file handles passed via file descriptor
> passing.
> 
> Finally qmp_chardev_open_socket() is the actual qmp hotplug
> implementation which basically just calls socket_listen or
> socket_connect and the new qemu_chr_open_socket_fd function.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi-schema.json |   13 +++-
>  qemu-char.c      |  168 ++++++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 124 insertions(+), 57 deletions(-)
> 
> +++ b/qemu-char.c
> @@ -2438,10 +2438,88 @@ static void tcp_chr_close(CharDriverState *chr)
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>  }
>  
> -static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
> +static CharDriverState *qemu_chr_open_socket_fd(int fd, int do_nodelay,
> +                                                int is_listen, int is_telnet,
> +                                                int is_waitconnect,

These three parameters sound like they might be better as 'bool' instead
of 'int'.

> @@ -2458,10 +2536,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>      if (!is_listen)
>          is_waitconnect = 0;
>  
> -    chr = g_malloc0(sizeof(CharDriverState));
> -    s = g_malloc0(sizeof(TCPCharDriver));
> -
> -    if (is_unix) {
> +     if (is_unix) {

Spurious re-indentation?

>  
> +static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
> +                                                Error **errp)
> +{
> +    SocketAddress *addr = sock->addr;
> +    bool do_nodelay     = sock->has_delay  ? !sock->delay : true;
> +    bool is_listen      = sock->has_server ? sock->server : true;
> +    bool is_telnet      = sock->has_telnet ? sock->telnet : false;
> +    bool is_waitconnect = sock->has_wait   ? sock->wait   : false;

Especially since you are using bool here.

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

Re: [Qemu-devel] [PATCH 10/11] chardev: add socket chardev support to chardev-add (qmp)

[Gerd Hoffmann]

  Hi,

>> -static CharDriverState *qemu_chr_open_socket(QemuOpts *opts) 
>> +static CharDriverState *qemu_chr_open_socket_fd(int fd, int
>> do_nodelay, +                                                int
>> is_listen, int is_telnet, +
>> int is_waitconnect,
> 
> These three parameters sound like they might be better as 'bool'
> instead of 'int'.

Just continuing to do what the existing code did.

>> @@ -2458,10 +2536,7 @@ static CharDriverState
>> *qemu_chr_open_socket(QemuOpts *opts) if (!is_listen) 
>> is_waitconnect = 0;
>> 
>> -    chr = g_malloc0(sizeof(CharDriverState)); -    s =
>> g_malloc0(sizeof(TCPCharDriver)); - -    if (is_unix) { +     if
>> (is_unix) {
> 
> Spurious re-indentation?

Seems so, I'll check.

cheers,
  Gerd

[Qemu-devel] [PATCH 11/11] chardev: add pty chardev support to chardev-add (qmp)

[Gerd Hoffmann]

The ptsname is returned directly, so there is no need to
use query-chardev to figure the pty device path.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qapi-schema.json |    7 ++++++-
 qemu-char.c      |   24 +++++++++++++++++++++---
 2 files changed, 27 insertions(+), 4 deletions(-)

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

Re: [Qemu-devel] [PATCH 11/11] chardev: add pty chardev support to chardev-add (qmp)

[Eric Blake]

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

On 01/07/2013 06:55 AM, Gerd Hoffmann wrote:
> The ptsname is returned directly, so there is no need to
> use query-chardev to figure the pty device path.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qapi-schema.json |    7 ++++++-
>  qemu-char.c      |   24 +++++++++++++++++++++---
>  2 files changed, 27 insertions(+), 4 deletions(-)

It would be nice to document an example with the return type in
qmp-commands.hx.  In fact, it might be worth declaring just:

{ 'union': 'ChardevReturn', 'data': { 'nodata' : 'ChardevDummy' } }

earlier in patch 4/11, so that we are consistently documenting the final
API all along, rather than changing the return type and having to
revisit earlier examples as part of this patch [but see below about an
alternate proposal to leave earlier examples untouched].

In the long run, it won't matter as long as the series is applied as a
whole - but if someone backports just part of the series, then we want
to avoid the case where the backport has different return semantics than
upstream, because they didn't pick up the change in return type from
this patch.

> +++ b/qapi-schema.json
> @@ -3054,10 +3054,15 @@
>  { 'union': 'ChardevBackend', 'data': { 'file'   : 'ChardevFile',
>                                         'port'   : 'ChardevPort',
>                                         'socket' : 'ChardevSocket',
> +                                       'pty'    : 'ChardevDummy',
>                                         'null'   : 'ChardevDummy' } }
>  
> +{ 'union': 'ChardevReturn', 'data': { 'pty'    : 'str',
> +                                      'nodata' : 'ChardevDummy' } }
> +
>  { 'command': 'chardev-add', 'data': {'id'      : 'str',
> -                                     'backend' : 'ChardevBackend' } }
> +                                     'backend' : 'ChardevBackend' },
> +  'returns' : 'ChardevReturn' }

If I'm reading it correctly, this means my earlier example for 4/11 becomes:

-> { "execute" : "chardev-add",
     "arguments" : { "id" : "foo",
                     "backend" : { "type" : "null" } } }
<- { "return": { "type" : "nodata"} }

or maybe the more verbose:

<- { "return": { "type" : "nodata", "data" : {} } }

and for this patch, we add:

-> { "execute" : "chardev-add",
     "arguments" : { "id" : "foo",
                     "backend" : { "type" : "pty", "data" : { } } }
<- { "return": { "type" : "pty", "data" : "/dev/pty0" } }

It also raises the question of whether unions even work with raw types,
or whether you have to always go through a struct, in which case you
should have used the 'String' wrapper instead of 'str', looking like:

{ 'union': 'ChardevReturn', 'data': { 'pty'    : 'String',
                                      'nodata' : 'ChardevDummy' } }
...
<- { "return": { "type" : "pty", "data" : { "str" : "/dev/pty0" } } }





But do we really need it to be that complex?  Why not just use a type
with an optional member, instead of going through a union:

{ 'type' : 'ChardevReturn', 'data': { '*pty' : 'str' } }

so that adding a null device returns the same as it always did:

{ "return": {} }

and so that adding a pty looks nicer:

{ "return": { "pty" : "/dev/pty0" } }


Libvirt will be able to cope either way, but I'd prefer a less complex
solution.

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

Re: [Qemu-devel] [PATCH 11/11] chardev: add pty chardev support to chardev-add (qmp)

[Gerd Hoffmann]

  Hi,

> But do we really need it to be that complex?  Why not just use a
> type with an optional member, instead of going through a union:
> 
> { 'type' : 'ChardevReturn', 'data': { '*pty' : 'str' } }

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

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 11/11] chardev: add pty chardev support to chardev-add (qmp)

[Paolo Bonzini]

Il 09/01/2013 18:37, Eric Blake ha scritto:
> It also raises the question of whether unions even work with raw types,
> or whether you have to always go through a struct, in which case you
> should have used the 'String' wrapper instead of 'str', looking like:
> 
> { 'union': 'ChardevReturn', 'data': { 'pty'    : 'String',
>                                       'nodata' : 'ChardevDummy' } }
> ...
> <- { "return": { "type" : "pty", "data" : { "str" : "/dev/pty0" } } }

They do work with raw types.

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

Paolo