Re: [Qemu-devel] [PATCH v2 13/14] char: make chr_fe_deinit() optionaly delete backend

[Philippe Mathieu-Daudé]

Hi Marc-André,

The new boolean argument bothered me at first but I couldn't find nicer 
way (that's why I kept this patch review so long).


On 05/29/2017 05:45 AM, Marc-André Lureau wrote:

This simplifies removing a backend for a frontend user (no need to
retrive the associated driver and seperate delete call etc).


retrieve, separate



NB: many frontends have questionable handling of ending a chardev. They
should probably delete the backend to prevent broken reusage.

Signed-off-by: Marc-André Lureau 


Reviewed-by: Philippe Mathieu-Daudé 


---
 include/chardev/char-fe.h|  6 --
 backends/rng-egd.c   |  2 +-
 chardev/char-fe.c|  5 -
 chardev/char-mux.c   |  2 +-
 gdbstub.c| 15 ++-
 hw/char/serial.c |  2 +-
 hw/char/xen_console.c|  2 +-
 hw/core/qdev-properties-system.c |  2 +-
 hw/usb/ccid-card-passthru.c  |  5 +
 hw/usb/redirect.c|  4 +---
 monitor.c|  2 +-
 net/colo-compare.c   |  8 +++-
 net/filter-mirror.c  |  6 +++---
 net/vhost-user.c |  5 +
 tests/test-char.c| 22 --
 tests/vhost-user-test.c  |  4 +---
 16 files changed, 34 insertions(+), 58 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index bd82093218..2cbb262f66 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -30,12 +30,14 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error 
**errp);

 /**
  * @qemu_chr_fe_deinit:
- *
+ * @b: a CharBackend
+ * @del: if true, delete the chardev backend
+*
  * Dissociate the CharBackend from the Chardev.
  *
  * Safe to call without associated Chardev.
  */
-void qemu_chr_fe_deinit(CharBackend *b);
+void qemu_chr_fe_deinit(CharBackend *b, bool del);

 /**
  * @qemu_chr_fe_get_driver:
diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index ad3e1e5edf..e7ce2cac80 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -145,7 +145,7 @@ static void rng_egd_finalize(Object *obj)
 {
 RngEgd *s = RNG_EGD(obj);

-qemu_chr_fe_deinit(>chr);
+qemu_chr_fe_deinit(>chr, false);
 g_free(s->chr_name);
 }

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 341221d029..3f90f0567c 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -211,7 +211,7 @@ unavailable:
 return false;
 }

-void qemu_chr_fe_deinit(CharBackend *b)
+void qemu_chr_fe_deinit(CharBackend *b, bool del)
 {
 assert(b);

@@ -224,6 +224,9 @@ void qemu_chr_fe_deinit(CharBackend *b)
 MuxChardev *d = MUX_CHARDEV(b->chr);
 d->backends[b->tag] = NULL;
 }
+if (del) {
+object_unparent(OBJECT(b->chr));
+}
 b->chr = NULL;
 }
 }
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 106c682e7f..08570b915e 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -266,7 +266,7 @@ static void char_mux_finalize(Object *obj)
 be->chr = NULL;
 }
 }
-qemu_chr_fe_deinit(>chr);
+qemu_chr_fe_deinit(>chr, false);
 }

 void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
diff --git a/gdbstub.c b/gdbstub.c
index 4251d23898..ec4e4b25be 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1678,9 +1678,6 @@ void gdb_exit(CPUArchState *env, int code)
 {
   GDBState *s;
   char buf[4];
-#ifndef CONFIG_USER_ONLY
-  Chardev *chr;
-#endif

   s = gdbserver_state;
   if (!s) {
@@ -1690,19 +1687,13 @@ void gdb_exit(CPUArchState *env, int code)
   if (gdbserver_fd < 0 || s->fd < 0) {
   return;
   }
-#else
-  chr = qemu_chr_fe_get_driver(>chr);
-  if (!chr) {
-  return;
-  }
 #endif

   snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
   put_packet(s, buf);

 #ifndef CONFIG_USER_ONLY
-  qemu_chr_fe_deinit(>chr);
-  object_unparent(OBJECT(chr));
+  qemu_chr_fe_deinit(>chr, true);
 #endif
 }

@@ -2002,9 +1993,7 @@ int gdbserver_start(const char *device)
NULL, _abort);
 monitor_init(mon_chr, 0);
 } else {
-if (qemu_chr_fe_get_driver(>chr)) {
-object_unparent(OBJECT(qemu_chr_fe_get_driver(>chr)));
-}
+qemu_chr_fe_deinit(>chr, true);
 mon_chr = s->mon_chr;
 memset(s, 0, sizeof(GDBState));
 s->mon_chr = mon_chr;
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 23e5fe9d18..e1f12507bf 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -905,7 +905,7 @@ void serial_realize_core(SerialState *s, Error **errp)

 void serial_exit_core(SerialState *s)
 {
-qemu_chr_fe_deinit(>chr);
+qemu_chr_fe_deinit(>chr, false);

 timer_del(s->modem_status_poll);
 timer_free(s->modem_status_poll);
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index cb849c2e3e..f9af8cadf4 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -261,7 +261,7 @@ 

Re: [Qemu-devel] [PATCH v2 13/14] char: make chr_fe_deinit() optionaly delete backend

[Marc-André Lureau]

ping, only patch left in the series without review

thanks

On Mon, May 29, 2017 at 12:56 PM Marc-André Lureau <
marcandre.lur...@redhat.com> wrote:

> This simplifies removing a backend for a frontend user (no need to
> retrive the associated driver and seperate delete call etc).
>
> NB: many frontends have questionable handling of ending a chardev. They
> should probably delete the backend to prevent broken reusage.
>
> Signed-off-by: Marc-André Lureau 
> ---
>  include/chardev/char-fe.h|  6 --
>  backends/rng-egd.c   |  2 +-
>  chardev/char-fe.c|  5 -
>  chardev/char-mux.c   |  2 +-
>  gdbstub.c| 15 ++-
>  hw/char/serial.c |  2 +-
>  hw/char/xen_console.c|  2 +-
>  hw/core/qdev-properties-system.c |  2 +-
>  hw/usb/ccid-card-passthru.c  |  5 +
>  hw/usb/redirect.c|  4 +---
>  monitor.c|  2 +-
>  net/colo-compare.c   |  8 +++-
>  net/filter-mirror.c  |  6 +++---
>  net/vhost-user.c |  5 +
>  tests/test-char.c| 22 --
>  tests/vhost-user-test.c  |  4 +---
>  16 files changed, 34 insertions(+), 58 deletions(-)
>
> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> index bd82093218..2cbb262f66 100644
> --- a/include/chardev/char-fe.h
> +++ b/include/chardev/char-fe.h
> @@ -30,12 +30,14 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s,
> Error **errp);
>
>  /**
>   * @qemu_chr_fe_deinit:
> - *
> + * @b: a CharBackend
> + * @del: if true, delete the chardev backend
> +*
>   * Dissociate the CharBackend from the Chardev.
>   *
>   * Safe to call without associated Chardev.
>   */
> -void qemu_chr_fe_deinit(CharBackend *b);
> +void qemu_chr_fe_deinit(CharBackend *b, bool del);
>
>  /**
>   * @qemu_chr_fe_get_driver:
> diff --git a/backends/rng-egd.c b/backends/rng-egd.c
> index ad3e1e5edf..e7ce2cac80 100644
> --- a/backends/rng-egd.c
> +++ b/backends/rng-egd.c
> @@ -145,7 +145,7 @@ static void rng_egd_finalize(Object *obj)
>  {
>  RngEgd *s = RNG_EGD(obj);
>
> -qemu_chr_fe_deinit(>chr);
> +qemu_chr_fe_deinit(>chr, false);
>  g_free(s->chr_name);
>  }
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index 341221d029..3f90f0567c 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -211,7 +211,7 @@ unavailable:
>  return false;
>  }
>
> -void qemu_chr_fe_deinit(CharBackend *b)
> +void qemu_chr_fe_deinit(CharBackend *b, bool del)
>  {
>  assert(b);
>
> @@ -224,6 +224,9 @@ void qemu_chr_fe_deinit(CharBackend *b)
>  MuxChardev *d = MUX_CHARDEV(b->chr);
>  d->backends[b->tag] = NULL;
>  }
> +if (del) {
> +object_unparent(OBJECT(b->chr));
> +}
>  b->chr = NULL;
>  }
>  }
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index 106c682e7f..08570b915e 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -266,7 +266,7 @@ static void char_mux_finalize(Object *obj)
>  be->chr = NULL;
>  }
>  }
> -qemu_chr_fe_deinit(>chr);
> +qemu_chr_fe_deinit(>chr, false);
>  }
>
>  void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
> diff --git a/gdbstub.c b/gdbstub.c
> index 4251d23898..ec4e4b25be 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1678,9 +1678,6 @@ void gdb_exit(CPUArchState *env, int code)
>  {
>GDBState *s;
>char buf[4];
> -#ifndef CONFIG_USER_ONLY
> -  Chardev *chr;
> -#endif
>
>s = gdbserver_state;
>if (!s) {
> @@ -1690,19 +1687,13 @@ void gdb_exit(CPUArchState *env, int code)
>if (gdbserver_fd < 0 || s->fd < 0) {
>return;
>}
> -#else
> -  chr = qemu_chr_fe_get_driver(>chr);
> -  if (!chr) {
> -  return;
> -  }
>  #endif
>
>snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
>put_packet(s, buf);
>
>  #ifndef CONFIG_USER_ONLY
> -  qemu_chr_fe_deinit(>chr);
> -  object_unparent(OBJECT(chr));
> +  qemu_chr_fe_deinit(>chr, true);
>  #endif
>  }
>
> @@ -2002,9 +1993,7 @@ int gdbserver_start(const char *device)
> NULL, _abort);
>  monitor_init(mon_chr, 0);
>  } else {
> -if (qemu_chr_fe_get_driver(>chr)) {
> -object_unparent(OBJECT(qemu_chr_fe_get_driver(>chr)));
> -}
> +qemu_chr_fe_deinit(>chr, true);
>  mon_chr = s->mon_chr;
>  memset(s, 0, sizeof(GDBState));
>  s->mon_chr = mon_chr;
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 23e5fe9d18..e1f12507bf 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -905,7 +905,7 @@ void serial_realize_core(SerialState *s, Error **errp)
>
>  void serial_exit_core(SerialState *s)
>  {
> -qemu_chr_fe_deinit(>chr);
> +qemu_chr_fe_deinit(>chr, false);
>
>  timer_del(s->modem_status_poll);
>  timer_free(s->modem_status_poll);

[Qemu-devel] [PATCH v2 13/14] char: make chr_fe_deinit() optionaly delete backend

[Marc-André Lureau]

This simplifies removing a backend for a frontend user (no need to
retrive the associated driver and seperate delete call etc).

NB: many frontends have questionable handling of ending a chardev. They
should probably delete the backend to prevent broken reusage.

Signed-off-by: Marc-André Lureau 
---
 include/chardev/char-fe.h|  6 --
 backends/rng-egd.c   |  2 +-
 chardev/char-fe.c|  5 -
 chardev/char-mux.c   |  2 +-
 gdbstub.c| 15 ++-
 hw/char/serial.c |  2 +-
 hw/char/xen_console.c|  2 +-
 hw/core/qdev-properties-system.c |  2 +-
 hw/usb/ccid-card-passthru.c  |  5 +
 hw/usb/redirect.c|  4 +---
 monitor.c|  2 +-
 net/colo-compare.c   |  8 +++-
 net/filter-mirror.c  |  6 +++---
 net/vhost-user.c |  5 +
 tests/test-char.c| 22 --
 tests/vhost-user-test.c  |  4 +---
 16 files changed, 34 insertions(+), 58 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index bd82093218..2cbb262f66 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -30,12 +30,14 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error 
**errp);
 
 /**
  * @qemu_chr_fe_deinit:
- *
+ * @b: a CharBackend
+ * @del: if true, delete the chardev backend
+*
  * Dissociate the CharBackend from the Chardev.
  *
  * Safe to call without associated Chardev.
  */
-void qemu_chr_fe_deinit(CharBackend *b);
+void qemu_chr_fe_deinit(CharBackend *b, bool del);
 
 /**
  * @qemu_chr_fe_get_driver:
diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index ad3e1e5edf..e7ce2cac80 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -145,7 +145,7 @@ static void rng_egd_finalize(Object *obj)
 {
 RngEgd *s = RNG_EGD(obj);
 
-qemu_chr_fe_deinit(>chr);
+qemu_chr_fe_deinit(>chr, false);
 g_free(s->chr_name);
 }
 
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 341221d029..3f90f0567c 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -211,7 +211,7 @@ unavailable:
 return false;
 }
 
-void qemu_chr_fe_deinit(CharBackend *b)
+void qemu_chr_fe_deinit(CharBackend *b, bool del)
 {
 assert(b);
 
@@ -224,6 +224,9 @@ void qemu_chr_fe_deinit(CharBackend *b)
 MuxChardev *d = MUX_CHARDEV(b->chr);
 d->backends[b->tag] = NULL;
 }
+if (del) {
+object_unparent(OBJECT(b->chr));
+}
 b->chr = NULL;
 }
 }
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 106c682e7f..08570b915e 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -266,7 +266,7 @@ static void char_mux_finalize(Object *obj)
 be->chr = NULL;
 }
 }
-qemu_chr_fe_deinit(>chr);
+qemu_chr_fe_deinit(>chr, false);
 }
 
 void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
diff --git a/gdbstub.c b/gdbstub.c
index 4251d23898..ec4e4b25be 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1678,9 +1678,6 @@ void gdb_exit(CPUArchState *env, int code)
 {
   GDBState *s;
   char buf[4];
-#ifndef CONFIG_USER_ONLY
-  Chardev *chr;
-#endif
 
   s = gdbserver_state;
   if (!s) {
@@ -1690,19 +1687,13 @@ void gdb_exit(CPUArchState *env, int code)
   if (gdbserver_fd < 0 || s->fd < 0) {
   return;
   }
-#else
-  chr = qemu_chr_fe_get_driver(>chr);
-  if (!chr) {
-  return;
-  }
 #endif
 
   snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
   put_packet(s, buf);
 
 #ifndef CONFIG_USER_ONLY
-  qemu_chr_fe_deinit(>chr);
-  object_unparent(OBJECT(chr));
+  qemu_chr_fe_deinit(>chr, true);
 #endif
 }
 
@@ -2002,9 +1993,7 @@ int gdbserver_start(const char *device)
NULL, _abort);
 monitor_init(mon_chr, 0);
 } else {
-if (qemu_chr_fe_get_driver(>chr)) {
-object_unparent(OBJECT(qemu_chr_fe_get_driver(>chr)));
-}
+qemu_chr_fe_deinit(>chr, true);
 mon_chr = s->mon_chr;
 memset(s, 0, sizeof(GDBState));
 s->mon_chr = mon_chr;
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 23e5fe9d18..e1f12507bf 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -905,7 +905,7 @@ void serial_realize_core(SerialState *s, Error **errp)
 
 void serial_exit_core(SerialState *s)
 {
-qemu_chr_fe_deinit(>chr);
+qemu_chr_fe_deinit(>chr, false);
 
 timer_del(s->modem_status_poll);
 timer_free(s->modem_status_poll);
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index cb849c2e3e..f9af8cadf4 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -261,7 +261,7 @@ static void con_disconnect(struct XenDevice *xendev)
 {
 struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
 
-qemu_chr_fe_deinit(>chr);
+qemu_chr_fe_deinit(>chr, false);
 xen_pv_unbind_evtchn(>xendev);
 
 if