All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/9] chardevice hotswap
@ 2017-05-19 12:47 Anton Nefedov
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 1/9] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
                   ` (8 more replies)
  0 siblings, 9 replies; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov

This serie is a v2 of the February submit
http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg01989.html

The interface is changed as requested and the changes are slightly reworked
and split into separate patches.

========

The patchset adds support of the character device change without
a frontend device removal.
Yet isa-serial and virtio-serial frontends are supported.

The feature can be helpful for e.g. Windows debug allowing to
establish connection to a live VM from VM with WinDbg.

Anton Nefedov (9):
  char: move QemuOpts->ChardevBackend translation to a separate func
  char: add backend hotswap handler
  char: chardevice hotswap
  hmp: add hmp analogue for qmp-chardev-change
  char: forbid direct chardevice access for hotswap devices
  virtio-console: chardev hotswap support
  serial: move TIOCM update to a separate function
  serial: chardev hotswap support
  char: avoid chardevice direct access

 backends/rng-egd.c          |   2 +-
 chardev/char-mux.c          |   1 +
 chardev/char.c              | 235 +++++++++++++++++++++++++++++++++++++-------
 gdbstub.c                   |   4 +-
 hmp-commands.hx             |  16 +++
 hmp.c                       |  34 +++++++
 hmp.h                       |   1 +
 hw/arm/pxa2xx.c             |   3 +-
 hw/arm/strongarm.c          |   4 +-
 hw/char/bcm2835_aux.c       |   2 +-
 hw/char/cadence_uart.c      |   4 +-
 hw/char/debugcon.c          |   4 +-
 hw/char/digic-uart.c        |   2 +-
 hw/char/escc.c              |   8 +-
 hw/char/etraxfs_ser.c       |   2 +-
 hw/char/exynos4210_uart.c   |   4 +-
 hw/char/grlib_apbuart.c     |   4 +-
 hw/char/imx_serial.c        |   2 +-
 hw/char/ipoctal232.c        |   4 +-
 hw/char/lm32_juart.c        |   2 +-
 hw/char/lm32_uart.c         |   2 +-
 hw/char/mcf_uart.c          |   2 +-
 hw/char/milkymist-uart.c    |   2 +-
 hw/char/parallel.c          |   2 +-
 hw/char/pl011.c             |   2 +-
 hw/char/sclpconsole-lm.c    |   4 +-
 hw/char/sclpconsole.c       |   4 +-
 hw/char/serial.c            |  63 +++++++++---
 hw/char/sh_serial.c         |   4 +-
 hw/char/spapr_vty.c         |   4 +-
 hw/char/stm32f2xx_usart.c   |   3 +-
 hw/char/terminal3270.c      |   4 +-
 hw/char/virtio-console.c    |  35 ++++++-
 hw/char/xen_console.c       |   4 +-
 hw/char/xilinx_uartlite.c   |   2 +-
 hw/ipmi/ipmi_bmc_extern.c   |   4 +-
 hw/mips/boston.c            |   2 +-
 hw/mips/mips_malta.c        |   2 +-
 hw/misc/ivshmem.c           |   6 +-
 hw/usb/ccid-card-passthru.c |   6 +-
 hw/usb/dev-serial.c         |   7 +-
 hw/usb/redirect.c           |   7 +-
 include/sysemu/char.h       |  44 +++++++++
 monitor.c                   |   4 +-
 net/colo-compare.c          |  14 +--
 net/filter-mirror.c         |   8 +-
 net/slirp.c                 |   2 +-
 net/vhost-user.c            |   7 +-
 qapi-schema.json            |  40 ++++++++
 qtest.c                     |   2 +-
 tests/test-char.c           |  14 ++-
 tests/vhost-user-test.c     |   2 +-
 52 files changed, 506 insertions(+), 140 deletions(-)

-- 
2.7.4

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

* [Qemu-devel] [PATCH v2 1/9] char: move QemuOpts->ChardevBackend translation to a separate func
  2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
@ 2017-05-19 12:47 ` Anton Nefedov
  2017-05-25 14:29   ` Marc-André Lureau
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 2/9] char: add backend hotswap handler Anton Nefedov
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 chardev/char.c | 72 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 26 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 4e24dc3..684cccd 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -854,17 +854,14 @@ help_string_append(const char *name, void *opaque)
     g_string_append_printf(str, "\n%s", name);
 }
 
-Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
-                                Error **errp)
+static ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
+                                           Error **errp)
 {
     Error *local_err = NULL;
     const ChardevClass *cc;
-    Chardev *chr;
     int i;
     ChardevBackend *backend = NULL;
     const char *name = qemu_opt_get(opts, "backend");
-    const char *id = qemu_opts_id(opts);
-    char *bid = NULL;
 
     if (name == NULL) {
         error_setg(errp, "chardev: \"%s\" missing backend",
@@ -872,21 +869,6 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
         return NULL;
     }
 
-    if (is_help_option(name)) {
-        GString *str = g_string_new("");
-
-        chardev_name_foreach(help_string_append, str);
-
-        error_report("Available chardev backend types: %s", str->str);
-        g_string_free(str, true);
-        exit(0);
-    }
-
-    if (id == NULL) {
-        error_setg(errp, "chardev: no id specified");
-        return NULL;
-    }
-
     for (i = 0; i < ARRAY_SIZE(chardev_alias_table); i++) {
         if (g_strcmp0(chardev_alias_table[i].alias, name) == 0) {
             name = chardev_alias_table[i].typename;
@@ -902,16 +884,12 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
     backend = g_new0(ChardevBackend, 1);
     backend->type = CHARDEV_BACKEND_KIND_NULL;
 
-    if (qemu_opt_get_bool(opts, "mux", 0)) {
-        bid = g_strdup_printf("%s-base", id);
-    }
-
-    chr = NULL;
     if (cc->parse) {
         cc->parse(opts, backend, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            goto out;
+            qapi_free_ChardevBackend(backend);
+            return NULL;
         }
     } else {
         ChardevCommon *ccom = g_new0(ChardevCommon, 1);
@@ -919,6 +897,48 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
         backend->u.null.data = ccom; /* Any ChardevCommon member would work */
     }
 
+    return backend;
+}
+
+Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
+                                Error **errp)
+{
+    const ChardevClass *cc;
+    Chardev *chr = NULL;
+    ChardevBackend *backend = NULL;
+    const char *name = qemu_opt_get(opts, "backend");
+    const char *id = qemu_opts_id(opts);
+    char *bid = NULL;
+
+    if (name && is_help_option(name)) {
+        GString *str = g_string_new("");
+
+        chardev_name_foreach(help_string_append, str);
+
+        error_report("Available chardev backend types: %s", str->str);
+        g_string_free(str, true);
+        exit(0);
+    }
+
+    if (id == NULL) {
+        error_setg(errp, "chardev: no id specified");
+        return NULL;
+    }
+
+    backend = qemu_chr_parse_opts(opts, errp);
+    if (backend == NULL) {
+        return NULL;
+    }
+
+    cc = char_get_class(name, errp);
+    if (cc == NULL) {
+        goto out;
+    }
+
+    if (qemu_opt_get_bool(opts, "mux", 0)) {
+        bid = g_strdup_printf("%s-base", id);
+    }
+
     chr = qemu_chardev_new(bid ? bid : id,
                            object_class_get_name(OBJECT_CLASS(cc)),
                            backend, errp);
-- 
2.7.4

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

* [Qemu-devel] [PATCH v2 2/9] char: add backend hotswap handler
  2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 1/9] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
@ 2017-05-19 12:47 ` Anton Nefedov
  2017-05-25 14:32   ` Marc-André Lureau
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 3/9] char: chardevice hotswap Anton Nefedov
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov

Frontends should have an interface to setup the handler of a backend change.
The interface will be used in the next commits

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 backends/rng-egd.c          |  2 +-
 chardev/char-mux.c          |  1 +
 chardev/char.c              |  4 +++-
 gdbstub.c                   |  2 +-
 hw/arm/pxa2xx.c             |  3 ++-
 hw/arm/strongarm.c          |  2 +-
 hw/char/bcm2835_aux.c       |  2 +-
 hw/char/cadence_uart.c      |  2 +-
 hw/char/debugcon.c          |  2 +-
 hw/char/digic-uart.c        |  2 +-
 hw/char/escc.c              |  2 +-
 hw/char/etraxfs_ser.c       |  2 +-
 hw/char/exynos4210_uart.c   |  2 +-
 hw/char/grlib_apbuart.c     |  2 +-
 hw/char/imx_serial.c        |  2 +-
 hw/char/ipoctal232.c        |  2 +-
 hw/char/lm32_juart.c        |  2 +-
 hw/char/lm32_uart.c         |  2 +-
 hw/char/mcf_uart.c          |  2 +-
 hw/char/milkymist-uart.c    |  2 +-
 hw/char/pl011.c             |  2 +-
 hw/char/sclpconsole-lm.c    |  2 +-
 hw/char/sclpconsole.c       |  2 +-
 hw/char/serial.c            |  2 +-
 hw/char/sh_serial.c         |  2 +-
 hw/char/spapr_vty.c         |  2 +-
 hw/char/stm32f2xx_usart.c   |  3 ++-
 hw/char/terminal3270.c      |  2 +-
 hw/char/virtio-console.c    |  4 ++--
 hw/char/xen_console.c       |  2 +-
 hw/char/xilinx_uartlite.c   |  2 +-
 hw/ipmi/ipmi_bmc_extern.c   |  2 +-
 hw/mips/boston.c            |  2 +-
 hw/mips/mips_malta.c        |  2 +-
 hw/misc/ivshmem.c           |  2 +-
 hw/usb/ccid-card-passthru.c |  2 +-
 hw/usb/dev-serial.c         |  2 +-
 hw/usb/redirect.c           |  2 +-
 include/sysemu/char.h       |  5 +++++
 monitor.c                   |  4 ++--
 net/colo-compare.c          | 14 ++++++++------
 net/filter-mirror.c         |  6 +++---
 net/slirp.c                 |  2 +-
 net/vhost-user.c            |  7 ++++---
 qtest.c                     |  2 +-
 tests/test-char.c           | 14 ++++++++++----
 tests/vhost-user-test.c     |  2 +-
 47 files changed, 78 insertions(+), 59 deletions(-)

diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 380b19a..0b0e945 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -106,7 +106,7 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
 
     /* FIXME we should resubmit pending requests when the CDS reconnects. */
     qemu_chr_fe_set_handlers(&s->chr, rng_egd_chr_can_read,
-                             rng_egd_chr_read, NULL, s, NULL, true);
+                             rng_egd_chr_read, NULL, NULL, s, NULL, true);
 }
 
 static void rng_egd_set_chardev(Object *obj, const char *value, Error **errp)
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 37d42c6..5849ea5 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -278,6 +278,7 @@ void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
                              mux_chr_can_read,
                              mux_chr_read,
                              mux_chr_event,
+                             NULL,
                              chr,
                              context, true);
 }
diff --git a/chardev/char.c b/chardev/char.c
index 684cccd..ae60950 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -522,7 +522,7 @@ void qemu_chr_fe_deinit(CharBackend *b)
     assert(b);
 
     if (b->chr) {
-        qemu_chr_fe_set_handlers(b, NULL, NULL, NULL, NULL, NULL, true);
+        qemu_chr_fe_set_handlers(b, NULL, NULL, NULL, NULL, NULL, NULL, true);
         if (b->chr->be == b) {
             b->chr->be = NULL;
         }
@@ -538,6 +538,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               IOCanReadHandler *fd_can_read,
                               IOReadHandler *fd_read,
                               IOEventHandler *fd_event,
+                              BackendChangeHandler *be_change,
                               void *opaque,
                               GMainContext *context,
                               bool set_open)
@@ -561,6 +562,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
     b->chr_can_read = fd_can_read;
     b->chr_read = fd_read;
     b->chr_event = fd_event;
+    b->chr_be_change = be_change;
     b->opaque = opaque;
     if (cc->chr_update_read_handler) {
         cc->chr_update_read_handler(s, context);
diff --git a/gdbstub.c b/gdbstub.c
index 86eed4f..1ac0489 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2013,7 +2013,7 @@ int gdbserver_start(const char *device)
     if (chr) {
         qemu_chr_fe_init(&s->chr, chr, &error_abort);
         qemu_chr_fe_set_handlers(&s->chr, gdb_chr_can_receive, gdb_chr_receive,
-                                 gdb_chr_event, NULL, NULL, true);
+                                 gdb_chr_event, NULL, NULL, NULL, true);
     }
     s->state = chr ? RS_IDLE : RS_INACTIVE;
     s->mon_chr = mon_chr;
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index eea551d..3e51882 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1970,7 +1970,8 @@ static void pxa2xx_fir_realize(DeviceState *dev, Error **errp)
     PXA2xxFIrState *s = PXA2XX_FIR(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, pxa2xx_fir_is_empty,
-                             pxa2xx_fir_rx, pxa2xx_fir_event, s, NULL, true);
+                             pxa2xx_fir_rx, pxa2xx_fir_event, NULL, s, NULL,
+                             true);
 }
 
 static bool pxa2xx_fir_vmstate_validate(void *opaque, int version_id)
diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index 3311cc3..bec093d 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -1246,7 +1246,7 @@ static void strongarm_uart_realize(DeviceState *dev, Error **errp)
                              strongarm_uart_can_receive,
                              strongarm_uart_receive,
                              strongarm_uart_event,
-                             s, NULL, true);
+                             NULL, s, NULL, true);
 }
 
 static void strongarm_uart_reset(DeviceState *dev)
diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c
index 4d46ad6..370dc7e 100644
--- a/hw/char/bcm2835_aux.c
+++ b/hw/char/bcm2835_aux.c
@@ -279,7 +279,7 @@ static void bcm2835_aux_realize(DeviceState *dev, Error **errp)
     BCM2835AuxState *s = BCM2835_AUX(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, bcm2835_aux_can_receive,
-                             bcm2835_aux_receive, NULL, s, NULL, true);
+                             bcm2835_aux_receive, NULL, NULL, s, NULL, true);
 }
 
 static Property bcm2835_aux_props[] = {
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 4dcee57..71867b3 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -484,7 +484,7 @@ static void cadence_uart_realize(DeviceState *dev, Error **errp)
                                           fifo_trigger_update, s);
 
     qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
-                             uart_event, s, NULL, true);
+                             uart_event, NULL, s, NULL, true);
 }
 
 static void cadence_uart_init(Object *obj)
diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
index 80dce07..6d95297 100644
--- a/hw/char/debugcon.c
+++ b/hw/char/debugcon.c
@@ -92,7 +92,7 @@ static void debugcon_realize_core(DebugconState *s, Error **errp)
         return;
     }
 
-    qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL, s, NULL, true);
+    qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL, NULL, s, NULL, true);
 }
 
 static void debugcon_isa_realizefn(DeviceState *dev, Error **errp)
diff --git a/hw/char/digic-uart.c b/hw/char/digic-uart.c
index 029f5bb..2d373dc 100644
--- a/hw/char/digic-uart.c
+++ b/hw/char/digic-uart.c
@@ -146,7 +146,7 @@ static void digic_uart_realize(DeviceState *dev, Error **errp)
     DigicUartState *s = DIGIC_UART(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx,
-                             uart_event, s, NULL, true);
+                             uart_event, NULL, s, NULL, true);
 }
 
 static void digic_uart_init(Object *obj)
diff --git a/hw/char/escc.c b/hw/char/escc.c
index 9228091..aa882b6 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -1015,7 +1015,7 @@ static void escc_realize(DeviceState *dev, Error **errp)
         if (qemu_chr_fe_get_driver(&s->chn[i].chr)) {
             s->chn[i].clock = s->frequency / 2;
             qemu_chr_fe_set_handlers(&s->chn[i].chr, serial_can_receive,
-                                     serial_receive1, serial_event,
+                                     serial_receive1, serial_event, NULL,
                                      &s->chn[i], NULL, true);
         }
     }
diff --git a/hw/char/etraxfs_ser.c b/hw/char/etraxfs_ser.c
index 5438387..4abd382 100644
--- a/hw/char/etraxfs_ser.c
+++ b/hw/char/etraxfs_ser.c
@@ -233,7 +233,7 @@ static void etraxfs_ser_realize(DeviceState *dev, Error **errp)
 
     qemu_chr_fe_set_handlers(&s->chr,
                              serial_can_receive, serial_receive,
-                             serial_event, s, NULL, true);
+                             serial_event, NULL, s, NULL, true);
 }
 
 static void etraxfs_ser_class_init(ObjectClass *klass, void *data)
diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index bff706a..7ef4ea5 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -644,7 +644,7 @@ static void exynos4210_uart_realize(DeviceState *dev, Error **errp)
 
     qemu_chr_fe_set_handlers(&s->chr, exynos4210_uart_can_receive,
                              exynos4210_uart_receive, exynos4210_uart_event,
-                             s, NULL, true);
+                             NULL, s, NULL, true);
 }
 
 static Property exynos4210_uart_properties[] = {
diff --git a/hw/char/grlib_apbuart.c b/hw/char/grlib_apbuart.c
index db686e6..610a317 100644
--- a/hw/char/grlib_apbuart.c
+++ b/hw/char/grlib_apbuart.c
@@ -247,7 +247,7 @@ static int grlib_apbuart_init(SysBusDevice *dev)
                              grlib_apbuart_can_receive,
                              grlib_apbuart_receive,
                              grlib_apbuart_event,
-                             uart, NULL, true);
+                             NULL, uart, NULL, true);
 
     sysbus_init_irq(dev, &uart->irq);
 
diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
index 52e67f8..b66396f 100644
--- a/hw/char/imx_serial.c
+++ b/hw/char/imx_serial.c
@@ -316,7 +316,7 @@ static void imx_serial_realize(DeviceState *dev, Error **errp)
     DPRINTF("char dev for uart: %p\n", qemu_chr_fe_get_driver(&s->chr));
 
     qemu_chr_fe_set_handlers(&s->chr, imx_can_receive, imx_receive,
-                             imx_event, s, NULL, true);
+                             imx_event, NULL, s, NULL, true);
 }
 
 static void imx_serial_init(Object *obj)
diff --git a/hw/char/ipoctal232.c b/hw/char/ipoctal232.c
index 93929c2..734e42c 100644
--- a/hw/char/ipoctal232.c
+++ b/hw/char/ipoctal232.c
@@ -545,7 +545,7 @@ static void ipoctal_realize(DeviceState *dev, Error **errp)
         if (qemu_chr_fe_get_driver(&ch->dev)) {
             qemu_chr_fe_set_handlers(&ch->dev, hostdev_can_receive,
                                      hostdev_receive, hostdev_event,
-                                     ch, NULL, true);
+                                     NULL, ch, NULL, true);
             DPRINTF("Redirecting channel %u to %s\n", i, ch->dev->label);
         } else {
             DPRINTF("Could not redirect channel %u, no chardev set\n", i);
diff --git a/hw/char/lm32_juart.c b/hw/char/lm32_juart.c
index f8c1e0d..b3a5351 100644
--- a/hw/char/lm32_juart.c
+++ b/hw/char/lm32_juart.c
@@ -119,7 +119,7 @@ static void lm32_juart_realize(DeviceState *dev, Error **errp)
     LM32JuartState *s = LM32_JUART(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, juart_can_rx, juart_rx,
-                             juart_event, s, NULL, true);
+                             juart_event, NULL, s, NULL, true);
 }
 
 static const VMStateDescription vmstate_lm32_juart = {
diff --git a/hw/char/lm32_uart.c b/hw/char/lm32_uart.c
index 7f3597c..3a6dbf8 100644
--- a/hw/char/lm32_uart.c
+++ b/hw/char/lm32_uart.c
@@ -266,7 +266,7 @@ static void lm32_uart_realize(DeviceState *dev, Error **errp)
     LM32UartState *s = LM32_UART(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx,
-                             uart_event, s, NULL, true);
+                             uart_event, NULL, s, NULL, true);
 }
 
 static const VMStateDescription vmstate_lm32_uart = {
diff --git a/hw/char/mcf_uart.c b/hw/char/mcf_uart.c
index e69672f..7f3cd5a 100644
--- a/hw/char/mcf_uart.c
+++ b/hw/char/mcf_uart.c
@@ -305,7 +305,7 @@ static void mcf_uart_realize(DeviceState *dev, Error **errp)
     mcf_uart_state *s = MCF_UART(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, mcf_uart_can_receive, mcf_uart_receive,
-                             mcf_uart_event, s, NULL, true);
+                             mcf_uart_event, NULL, s, NULL, true);
 }
 
 static Property mcf_uart_properties[] = {
diff --git a/hw/char/milkymist-uart.c b/hw/char/milkymist-uart.c
index ae8e2f3..523959a 100644
--- a/hw/char/milkymist-uart.c
+++ b/hw/char/milkymist-uart.c
@@ -199,7 +199,7 @@ static void milkymist_uart_realize(DeviceState *dev, Error **errp)
     MilkymistUartState *s = MILKYMIST_UART(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx,
-                             uart_event, s, NULL, true);
+                             uart_event, NULL, s, NULL, true);
 }
 
 static void milkymist_uart_init(Object *obj)
diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 24ea973..c38f60a 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -329,7 +329,7 @@ static void pl011_realize(DeviceState *dev, Error **errp)
     PL011State *s = PL011(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
-                             pl011_event, s, NULL, true);
+                             pl011_event, NULL, s, NULL, true);
 }
 
 static void pl011_class_init(ObjectClass *oc, void *data)
diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
index 07d6ebd..ed1e2c5 100644
--- a/hw/char/sclpconsole-lm.c
+++ b/hw/char/sclpconsole-lm.c
@@ -313,7 +313,7 @@ static int console_init(SCLPEvent *event)
     console_available = true;
 
     qemu_chr_fe_set_handlers(&scon->chr, chr_can_read,
-                             chr_read, NULL, scon, NULL, true);
+                             chr_read, NULL, NULL, scon, NULL, true);
 
     return 0;
 }
diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
index b78f240..9a65010 100644
--- a/hw/char/sclpconsole.c
+++ b/hw/char/sclpconsole.c
@@ -228,7 +228,7 @@ static int console_init(SCLPEvent *event)
     }
     console_available = true;
     qemu_chr_fe_set_handlers(&scon->chr, chr_can_read,
-                             chr_read, NULL, scon, NULL, true);
+                             chr_read, NULL, NULL, scon, NULL, true);
 
     return 0;
 }
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 03d890c..d8d34d0 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -897,7 +897,7 @@ void serial_realize_core(SerialState *s, Error **errp)
     qemu_register_reset(serial_reset, s);
 
     qemu_chr_fe_set_handlers(&s->chr, serial_can_receive1, serial_receive1,
-                             serial_event, s, NULL, true);
+                             serial_event, NULL, s, NULL, true);
     fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
     fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
     serial_reset(s);
diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index 303eb0a..c352337 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -400,7 +400,7 @@ void sh_serial_init(MemoryRegion *sysmem,
         qemu_chr_fe_init(&s->chr, chr, &error_abort);
         qemu_chr_fe_set_handlers(&s->chr, sh_serial_can_receive1,
                                  sh_serial_receive1,
-                                 sh_serial_event, s, NULL, true);
+                                 sh_serial_event, NULL, s, NULL, true);
     }
 
     s->eri = eri_source;
diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index e30c8da..9cdc0e0 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -84,7 +84,7 @@ static void spapr_vty_realize(VIOsPAPRDevice *sdev, Error **errp)
     }
 
     qemu_chr_fe_set_handlers(&dev->chardev, vty_can_receive,
-                             vty_receive, NULL, dev, NULL, true);
+                             vty_receive, NULL, NULL, dev, NULL, true);
 }
 
 /* Forward declaration */
diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
index 59872e6..268e435 100644
--- a/hw/char/stm32f2xx_usart.c
+++ b/hw/char/stm32f2xx_usart.c
@@ -207,7 +207,8 @@ static void stm32f2xx_usart_realize(DeviceState *dev, Error **errp)
     STM32F2XXUsartState *s = STM32F2XX_USART(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, stm32f2xx_usart_can_receive,
-                             stm32f2xx_usart_receive, NULL, s, NULL, true);
+                             stm32f2xx_usart_receive, NULL, NULL,
+                             s, NULL, true);
 }
 
 static void stm32f2xx_usart_class_init(ObjectClass *klass, void *data)
diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index b2dda01..943a0f3 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -179,7 +179,7 @@ static void terminal_init(EmulatedCcw3270Device *dev, Error **errp)
     }
     terminal_available = true;
     qemu_chr_fe_set_handlers(&t->chr, terminal_can_read,
-                             terminal_read, chr_event, t, NULL, true);
+                             terminal_read, chr_event, NULL, t, NULL, true);
 }
 
 static int read_payload_3270(EmulatedCcw3270Device *dev, uint32_t cda,
diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 798d9b6..cf7331d 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -188,11 +188,11 @@ static void virtconsole_realize(DeviceState *dev, Error **errp)
          */
         if (k->is_console) {
             qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
-                                     NULL, vcon, NULL, true);
+                                     NULL, NULL, vcon, NULL, true);
             virtio_serial_open(port);
         } else {
             qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
-                                     chr_event, vcon, NULL, false);
+                                     chr_event, NULL, vcon, NULL, false);
         }
     }
 }
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index c01f410..cb7975f 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -246,7 +246,7 @@ static int con_initialise(struct XenDevice *xendev)
 
     xen_be_bind_evtchn(&con->xendev);
     qemu_chr_fe_set_handlers(&con->chr, xencons_can_receive,
-                             xencons_receive, NULL, con, NULL, true);
+                             xencons_receive, NULL, NULL, con, NULL, true);
 
     xen_pv_printf(xendev, 1,
                   "ring mfn %d, remote port %d, local port %d, limit %zd\n",
diff --git a/hw/char/xilinx_uartlite.c b/hw/char/xilinx_uartlite.c
index 37d313b..2568302 100644
--- a/hw/char/xilinx_uartlite.c
+++ b/hw/char/xilinx_uartlite.c
@@ -212,7 +212,7 @@ static void xilinx_uartlite_realize(DeviceState *dev, Error **errp)
     XilinxUARTLite *s = XILINX_UARTLITE(dev);
 
     qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx,
-                             uart_event, s, NULL, true);
+                             uart_event, NULL, s, NULL, true);
 }
 
 static void xilinx_uartlite_init(Object *obj)
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index e8e3d25..6f2339d 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -453,7 +453,7 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp)
     }
 
     qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive,
-                             chr_event, ibe, NULL, true);
+                             chr_event, NULL, ibe, NULL, true);
 }
 
 static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id)
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 83f7b82..a57c860 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -533,7 +533,7 @@ static void boston_mach_init(MachineState *machine)
     chr = qemu_chr_new("lcd", "vc:320x240");
     qemu_chr_fe_init(&s->lcd_display, chr, NULL);
     qemu_chr_fe_set_handlers(&s->lcd_display, NULL, NULL,
-                             boston_lcd_event, s, NULL, true);
+                             boston_lcd_event, NULL, s, NULL, true);
 
     ahci = pci_create_simple_multifunction(&PCI_BRIDGE(&pcie2->root)->sec_bus,
                                            PCI_DEVFN(0, 0),
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index 5dd177e..96dce76 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -571,7 +571,7 @@ static MaltaFPGAState *malta_fpga_init(MemoryRegion *address_space,
     chr = qemu_chr_new("fpga", "vc:320x200");
     qemu_chr_fe_init(&s->display, chr, NULL);
     qemu_chr_fe_set_handlers(&s->display, NULL, NULL,
-                             malta_fgpa_display_event, s, NULL, true);
+                             malta_fgpa_display_event, NULL, s, NULL, true);
 
     s->uart = serial_mm_init(address_space, base + 0x900, 3, uart_irq,
                              230400, uart_chr, DEVICE_NATIVE_ENDIAN);
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 475e36a..e2dece8 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -896,7 +896,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
         }
 
         qemu_chr_fe_set_handlers(&s->server_chr, ivshmem_can_receive,
-                                 ivshmem_read, NULL, s, NULL, true);
+                                 ivshmem_read, NULL, NULL, s, NULL, true);
 
         if (ivshmem_setup_interrupts(s, errp) < 0) {
             error_prepend(errp, "Failed to initialize interrupts: ");
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index a41b0d6..9ace5ac 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -351,7 +351,7 @@ static int passthru_initfn(CCIDCardState *base)
         qemu_chr_fe_set_handlers(&card->cs,
             ccid_card_vscard_can_read,
             ccid_card_vscard_read,
-            ccid_card_vscard_event, card, NULL, true);
+            ccid_card_vscard_event, NULL, card, NULL, true);
         ccid_card_vscard_send_init(card);
     } else {
         error_report("missing chardev");
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 83a4f0e..e6b2c7c 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -501,7 +501,7 @@ static void usb_serial_realize(USBDevice *dev, Error **errp)
     }
 
     qemu_chr_fe_set_handlers(&s->cs, usb_serial_can_read, usb_serial_read,
-                             usb_serial_event, s, NULL, true);
+                             usb_serial_event, NULL, s, NULL, true);
     usb_serial_handle_reset(dev);
 
     if (chr->be_open && !dev->attached) {
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index ad5ef78..1e9bf69 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1399,7 +1399,7 @@ static void usbredir_realize(USBDevice *udev, Error **errp)
     /* Let the backend know we are ready */
     qemu_chr_fe_set_handlers(&dev->cs, usbredir_chardev_can_read,
                              usbredir_chardev_read, usbredir_chardev_event,
-                             dev, NULL, true);
+                             NULL, dev, NULL, true);
 
     dev->vmstate =
         qemu_add_vm_change_state_handler(usbredir_vm_state_change, dev);
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index fffc0f4..9f8df07 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -64,6 +64,7 @@ struct ParallelIOArg {
 #define CHR_TIOCM_RTS	0x004
 
 typedef void IOEventHandler(void *opaque, int event);
+typedef int BackendChangeHandler(void *opaque);
 
 typedef enum {
     /* Whether the chardev peer is able to close and
@@ -87,6 +88,7 @@ typedef struct CharBackend {
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
+    BackendChangeHandler *chr_be_change;
     void *opaque;
     int tag;
     int fe_open;
@@ -399,6 +401,8 @@ void qemu_chr_fe_deinit(CharBackend *b);
  *               receive
  * @fd_read: callback to receive data from char
  * @fd_event: event callback
+ * @be_change: backend change callback; passing NULL means hot backend change
+ *             is not supported and will not be attempted
  * @opaque: an opaque pointer for the callbacks
  * @context: a main loop context or NULL for the default
  * @set_open: whether to call qemu_chr_fe_set_open() implicitely when
@@ -413,6 +417,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               IOCanReadHandler *fd_can_read,
                               IOReadHandler *fd_read,
                               IOEventHandler *fd_event,
+                              BackendChangeHandler *be_change,
                               void *opaque,
                               GMainContext *context,
                               bool set_open);
diff --git a/monitor.c b/monitor.c
index afbacfe..9057ad3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4088,12 +4088,12 @@ void monitor_init(Chardev *chr, int flags)
 
     if (monitor_is_qmp(mon)) {
         qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
-                                 monitor_qmp_event, mon, NULL, true);
+                                 monitor_qmp_event, NULL, mon, NULL, true);
         qemu_chr_fe_set_echo(&mon->chr, true);
         json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
     } else {
         qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
-                                 monitor_event, mon, NULL, true);
+                                 monitor_event, NULL, mon, NULL, true);
     }
 
     qemu_mutex_lock(&monitor_lock);
diff --git a/net/colo-compare.c b/net/colo-compare.c
index 4ab80b1..f672f4f 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -532,7 +532,7 @@ static void compare_pri_chr_in(void *opaque, const uint8_t *buf, int size)
 
     ret = net_fill_rstate(&s->pri_rs, buf, size);
     if (ret == -1) {
-        qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL,
+        qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL, NULL,
                                  NULL, NULL, true);
         error_report("colo-compare primary_in error");
     }
@@ -549,7 +549,7 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
 
     ret = net_fill_rstate(&s->sec_rs, buf, size);
     if (ret == -1) {
-        qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL,
+        qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL, NULL,
                                  NULL, NULL, true);
         error_report("colo-compare secondary_in error");
     }
@@ -577,9 +577,11 @@ static void *colo_compare_thread(void *opaque)
     s->worker_context = g_main_context_new();
 
     qemu_chr_fe_set_handlers(&s->chr_pri_in, compare_chr_can_read,
-                          compare_pri_chr_in, NULL, s, s->worker_context, true);
+                             compare_pri_chr_in, NULL, NULL,
+                             s, s->worker_context, true);
     qemu_chr_fe_set_handlers(&s->chr_sec_in, compare_chr_can_read,
-                          compare_sec_chr_in, NULL, s, s->worker_context, true);
+                             compare_sec_chr_in, NULL, NULL,
+                             s, s->worker_context, true);
 
     s->compare_loop = g_main_loop_new(s->worker_context, FALSE);
 
@@ -790,9 +792,9 @@ static void colo_compare_finalize(Object *obj)
 {
     CompareState *s = COLO_COMPARE(obj);
 
-    qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL, NULL,
+    qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL, NULL, NULL,
                              s->worker_context, true);
-    qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL, NULL,
+    qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL, NULL, NULL,
                              s->worker_context, true);
     qemu_chr_fe_deinit(&s->chr_out);
 
diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index 72fa7c2..06321d7 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -112,7 +112,7 @@ static void redirector_chr_read(void *opaque, const uint8_t *buf, int size)
 
     if (ret == -1) {
         qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
-                                 NULL, NULL, true);
+                                 NULL, NULL, NULL, true);
     }
 }
 
@@ -124,7 +124,7 @@ static void redirector_chr_event(void *opaque, int event)
     switch (event) {
     case CHR_EVENT_CLOSED:
         qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
-                                 NULL, NULL, true);
+                                 NULL, NULL, NULL, true);
         break;
     default:
         break;
@@ -251,7 +251,7 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
 
         qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
                                  redirector_chr_read, redirector_chr_event,
-                                 nf, NULL, true);
+                                 NULL, nf, NULL, true);
     }
 
     if (s->outdev) {
diff --git a/net/slirp.c b/net/slirp.c
index c705a60..6cbae5a 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -778,7 +778,7 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
         fwd->slirp = s->slirp;
 
         qemu_chr_fe_set_handlers(&fwd->hd, guestfwd_can_read, guestfwd_read,
-                                 NULL, fwd, NULL, true);
+                                 NULL, NULL, fwd, NULL, true);
     }
     return 0;
 
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 00a0c1c..d090311 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -214,7 +214,7 @@ static void chr_closed_bh(void *opaque)
     vhost_user_stop(queues, ncs);
 
     qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, net_vhost_user_event,
-                             opaque, NULL, true);
+                             NULL, opaque, NULL, true);
 
     if (err) {
         error_report_err(err);
@@ -260,7 +260,7 @@ static void net_vhost_user_event(void *opaque, int event)
 
             g_source_remove(s->watch);
             s->watch = 0;
-            qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL,
+            qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL, NULL,
                                      NULL, NULL, false);
 
             aio_bh_schedule_oneshot(ctx, chr_closed_bh, opaque);
@@ -308,7 +308,8 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
             return -1;
         }
         qemu_chr_fe_set_handlers(&s->chr, NULL, NULL,
-                                 net_vhost_user_event, nc0->name, NULL, true);
+                                 net_vhost_user_event, NULL, nc0->name, NULL,
+                                 true);
     } while (!s->started);
 
     assert(s->vhost_net);
diff --git a/qtest.c b/qtest.c
index 5aa6636..b6e9780 100644
--- a/qtest.c
+++ b/qtest.c
@@ -691,7 +691,7 @@ void qtest_init(const char *qtest_chrdev, const char *qtest_log, Error **errp)
 
     qemu_chr_fe_init(&qtest_chr, chr, errp);
     qemu_chr_fe_set_handlers(&qtest_chr, qtest_can_read, qtest_read,
-                             qtest_event, &qtest_chr, NULL, true);
+                             qtest_event, NULL, &qtest_chr, NULL, true);
     qemu_chr_fe_set_echo(&qtest_chr, true);
 
     inbuf = g_string_new("");
diff --git a/tests/test-char.c b/tests/test-char.c
index 124d0c5..f3b377f 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -182,6 +182,7 @@ static void char_mux_test(void)
                              fe_can_read,
                              fe_read,
                              fe_event,
+                             NULL,
                              &h1,
                              NULL, true);
 
@@ -190,6 +191,7 @@ static void char_mux_test(void)
                              fe_can_read,
                              fe_read,
                              fe_event,
+                             NULL,
                              &h2,
                              NULL, true);
     qemu_chr_fe_take_focus(&chr_be2);
@@ -213,7 +215,8 @@ static void char_mux_test(void)
     h1.read_count = 0;
 
     /* remove first handler */
-    qemu_chr_fe_set_handlers(&chr_be1, NULL, NULL, NULL, NULL, NULL, true);
+    qemu_chr_fe_set_handlers(&chr_be1, NULL, NULL, NULL, NULL,
+                             NULL, NULL, true);
     qemu_chr_be_write(base, (void *)"hello", 6);
     g_assert_cmpint(h1.read_count, ==, 0);
     g_assert_cmpint(h2.read_count, ==, 0);
@@ -312,13 +315,13 @@ static void char_socket_test(void)
 
     qemu_chr_fe_init(&be, chr, &error_abort);
     qemu_chr_fe_set_handlers(&be, socket_can_read, socket_read,
-                             NULL, &d, NULL, true);
+                             NULL, NULL, &d, NULL, true);
 
     chr_client = qemu_chr_new("client", tmp);
     qemu_chr_fe_init(&client_be, chr_client, &error_abort);
     qemu_chr_fe_set_handlers(&client_be, socket_can_read_hello,
                              socket_read_hello,
-                             NULL, &d, NULL, true);
+                             NULL, NULL, &d, NULL, true);
     g_free(tmp);
 
     d.conn_expected = true;
@@ -388,6 +391,7 @@ static void char_pipe_test(void)
                              fe_can_read,
                              fe_read,
                              fe_event,
+                             NULL,
                              &fe,
                              NULL, true);
 
@@ -437,7 +441,7 @@ static void char_udp_test(void)
     d.chr = chr;
     qemu_chr_fe_init(&be, chr, &error_abort);
     qemu_chr_fe_set_handlers(&be, socket_can_read_hello, socket_read_hello,
-                             NULL, &d, NULL, true);
+                             NULL, NULL, &d, NULL, true);
     ret = qemu_chr_write_all(chr, (uint8_t *)"hello", 5);
     g_assert_cmpint(ret, ==, 5);
 
@@ -503,6 +507,7 @@ static void char_file_test(void)
                                  fe_can_read,
                                  fe_read,
                                  fe_event,
+                                 NULL,
                                  &fe, NULL, true);
 
         main_loop();
@@ -558,6 +563,7 @@ static void char_null_test(void)
                              fe_can_read,
                              fe_read,
                              fe_event,
+                             NULL,
                              NULL, NULL, true);
 
     ret = qemu_chr_fe_write(&be, (void *)"buf", 4);
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 9095af2..718c08a 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -464,7 +464,7 @@ static void test_server_create_chr(TestServer *server, const gchar *opt)
 
     qemu_chr_fe_init(&server->chr, chr, &error_abort);
     qemu_chr_fe_set_handlers(&server->chr, chr_can_read, chr_read,
-                             chr_event, server, NULL, true);
+                             chr_event, NULL, server, NULL, true);
 }
 
 static void test_server_listen(TestServer *server)
-- 
2.7.4

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

* [Qemu-devel] [PATCH v2 3/9] char: chardevice hotswap
  2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 1/9] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 2/9] char: add backend hotswap handler Anton Nefedov
@ 2017-05-19 12:47 ` Anton Nefedov
  2017-05-25 14:29   ` Marc-André Lureau
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 4/9] hmp: add hmp analogue for qmp-chardev-change Anton Nefedov
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov

This patch adds a possibility to change a char device without a frontend
removal.

1. Ideally, it would have to happen transparently to a frontend, i.e.
frontend would continue its regular operation.
However, backends are not stateless and are set up by the frontends
via qemu_chr_fe_<> functions, and it's not (generally) possible to replay
that setup entirely in a backend code, as different chardevs respond
to the setup calls differently, so do frontends work differently basing
on those setup responses.
Moreover, some frontend can generally get and save the backend pointer
(qemu_chr_fe_get_driver()), and it will become invalid after backend change.

So, a frontend which would like to support chardev hotswap has to register
a "backend change" handler, and redo its backend setup there.

2. Write path can be used by multiple threads and thus protected with
chr_write_lock.
So hotswap also has to be protected so write functions won't access
a backend being replaced.

3. Hotswap function can be called from e.g. a read handler of a monitor
socket. This can cause troubles so it's safer to defer execution to
a bottom-half (however, it means we cannot return some of the errors
synchronously - but most of them we can)

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 chardev/char.c        | 147 ++++++++++++++++++++++++++++++++++++++++++++++----
 include/sysemu/char.h |  10 ++++
 qapi-schema.json      |  40 ++++++++++++++
 3 files changed, 187 insertions(+), 10 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index ae60950..bac5e1c 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -132,12 +132,16 @@ static bool qemu_chr_replay(Chardev *chr)
 
 int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
 {
-    Chardev *s = be->chr;
+    Chardev *s;
     ChardevClass *cc;
     int ret;
 
+    qemu_mutex_lock(&be->chr_lock);
+    s = be->chr;
+
     if (!s) {
-        return 0;
+        ret = 0;
+        goto end;
     }
 
     if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_PLAY) {
@@ -145,7 +149,7 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
         replay_char_write_event_load(&ret, &offset);
         assert(offset <= len);
         qemu_chr_fe_write_buffer(s, buf, offset, &offset);
-        return ret;
+        goto end;
     }
 
     cc = CHARDEV_GET_CLASS(s);
@@ -161,7 +165,9 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
     if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_RECORD) {
         replay_char_write_event_save(ret, ret < 0 ? 0 : ret);
     }
-    
+
+end:
+    qemu_mutex_unlock(&be->chr_lock);
     return ret;
 }
 
@@ -191,13 +197,16 @@ int qemu_chr_write_all(Chardev *s, const uint8_t *buf, int len)
 
 int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len)
 {
-    Chardev *s = be->chr;
+    Chardev *s;
+    int ret;
 
-    if (!s) {
-        return 0;
-    }
+    qemu_mutex_lock(&be->chr_lock);
 
-    return qemu_chr_write_all(s, buf, len);
+    s = be->chr;
+    ret = s ? qemu_chr_write_all(s, buf, len) : 0;
+
+    qemu_mutex_unlock(&be->chr_lock);
+    return ret;
 }
 
 int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
@@ -478,7 +487,7 @@ Chardev *qemu_chr_fe_get_driver(CharBackend *be)
     return be->chr;
 }
 
-bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
+static bool fe_connect(CharBackend *b, Chardev *s, Error **errp)
 {
     int tag = 0;
 
@@ -507,6 +516,17 @@ unavailable:
     return false;
 }
 
+bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
+{
+    if (!fe_connect(b, s, errp)) {
+        return false;
+    }
+
+    qemu_mutex_init(&b->chr_lock);
+    b->hotswap_bh = NULL;
+    return true;
+}
+
 static bool qemu_chr_is_busy(Chardev *s)
 {
     if (CHARDEV_IS_MUX(s)) {
@@ -531,6 +551,10 @@ void qemu_chr_fe_deinit(CharBackend *b)
             d->backends[b->tag] = NULL;
         }
         b->chr = NULL;
+        qemu_mutex_destroy(&b->chr_lock);
+        if (b->hotswap_bh) {
+            qemu_bh_delete(b->hotswap_bh);
+        }
     }
 }
 
@@ -1308,6 +1332,109 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     return ret;
 }
 
+static void chardev_change_bh(void *opaque)
+{
+    Chardev *chr_new = opaque;
+    const char *id = chr_new->label;
+    Chardev *chr = qemu_chr_find(id);
+    CharBackend *be = chr->be;
+    bool closed_sent = false;
+
+    if (!be) {
+        /* disconnected since we checked: ok, less work for us */
+        goto end;
+    }
+
+    if (chr->be_open && !chr_new->be_open) {
+        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+        closed_sent = true;
+    }
+
+    qemu_mutex_lock(&be->chr_lock);
+    chr->be = NULL;
+    fe_connect(be, chr_new, &error_abort);
+
+    if (be->chr_be_change(be->opaque) < 0) {
+        error_report("Chardev '%s' change failed", id);
+        fe_connect(be, chr, &error_abort);
+        qemu_mutex_unlock(&be->chr_lock);
+        if (closed_sent) {
+            qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+        }
+        object_unref(OBJECT(chr_new));
+        return;
+    }
+    qemu_mutex_unlock(&be->chr_lock);
+
+end:
+    object_unparent(OBJECT(chr));
+    object_property_add_child(get_chardevs_root(), id, OBJECT(chr_new),
+                              &error_abort);
+    object_unref(OBJECT(chr_new));
+}
+
+ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
+                                  Error **errp)
+{
+    const ChardevClass *cc;
+    Chardev *chr, *chr_new;
+    ChardevReturn *ret;
+
+    chr = qemu_chr_find(id);
+    if (!chr) {
+        error_setg(errp, "Chardev '%s' does not exist", id);
+        return NULL;
+    }
+
+    if (CHARDEV_IS_MUX(chr)) {
+        error_setg(errp, "Mux device hotswap not supported yet");
+        return NULL;
+    }
+
+    if (qemu_chr_replay(chr)) {
+        error_setg(errp,
+            "Chardev '%s' cannot be changed in record/replay mode", id);
+        return NULL;
+    }
+
+    if (!chr->be) {
+        /* easy case */
+        object_unparent(OBJECT(chr));
+        return qmp_chardev_add(id, backend, errp);
+    }
+
+    if (!chr->be->chr_be_change) {
+        error_setg(errp, "Chardev user does not support chardev hotswap");
+        return NULL;
+    }
+
+    cc = char_get_class(ChardevBackendKind_lookup[backend->type], errp);
+    if (!cc) {
+        return NULL;
+    }
+
+    chr_new = qemu_chardev_new(NULL, object_class_get_name(OBJECT_CLASS(cc)),
+                               backend, errp);
+    chr_new->label = g_strdup(id);
+    if (!chr_new) {
+        return NULL;
+    }
+
+    if (chr->be->hotswap_bh) {
+        qemu_bh_delete(chr->be->hotswap_bh);
+    }
+    chr->be->hotswap_bh = qemu_bh_new(chardev_change_bh, chr_new);
+    qemu_bh_schedule(chr->be->hotswap_bh);
+
+    ret = g_new0(ChardevReturn, 1);
+    if (CHARDEV_IS_PTY(chr_new)) {
+        ret->pty = g_strdup(chr_new->filename + 4);
+        ret->has_pty = true;
+    }
+
+    return ret;
+}
+
 void qmp_chardev_remove(const char *id, Error **errp)
 {
     Chardev *chr;
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 9f8df07..68c7876 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -92,6 +92,8 @@ typedef struct CharBackend {
     void *opaque;
     int tag;
     int fe_open;
+    QemuMutex chr_lock;
+    QEMUBH *hotswap_bh;
 } CharBackend;
 
 struct Chardev {
@@ -141,6 +143,14 @@ void qemu_chr_parse_common(QemuOpts *opts, ChardevCommon *backend);
  */
 Chardev *qemu_chr_new(const char *label, const char *filename);
 
+/**
+ * @qemu_chr_change:
+ *
+ * Change an existing character backend
+ *
+ * @opts the new backend options
+ */
+void qemu_chr_change(QemuOpts *opts, Error **errp);
 
 /**
  * @qemu_chr_fe_disconnect:
diff --git a/qapi-schema.json b/qapi-schema.json
index 80603cf..bf72166 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5075,6 +5075,46 @@
   'returns': 'ChardevReturn' }
 
 ##
+# @chardev-change:
+#
+# Change a character device backend
+#
+# @id: the chardev's ID, must exist
+# @backend: new backend type and parameters
+#
+# Returns: ChardevReturn.
+#
+# Since: 2.10
+#
+# Example:
+#
+# -> { "execute" : "chardev-change",
+#      "arguments" : { "id" : "baz",
+#                      "backend" : { "type" : "pty", "data" : {} } } }
+# <- { "return": { "pty" : "/dev/pty/42" } }
+#
+# -> {"execute" : "chardev-change",
+#     "arguments" : {
+#         "id" : "charchannel2",
+#         "backend" : {
+#             "type" : "socket",
+#             "data" : {
+#                 "addr" : {
+#                     "type" : "unix" ,
+#                     "data" : {
+#                         "path" : "/tmp/charchannel2.socket"
+#                     }
+#                  },
+#                  "server" : true,
+#                  "wait" : false }}}}
+# <- {"return": {}}
+#
+##
+{ 'command': 'chardev-change', 'data': {'id'      : 'str',
+                                        'backend' : 'ChardevBackend' },
+  'returns': 'ChardevReturn' }
+
+##
 # @chardev-remove:
 #
 # Remove a character device backend
-- 
2.7.4

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

* [Qemu-devel] [PATCH v2 4/9] hmp: add hmp analogue for qmp-chardev-change
  2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
                   ` (2 preceding siblings ...)
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 3/9] char: chardevice hotswap Anton Nefedov
@ 2017-05-19 12:47 ` Anton Nefedov
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 5/9] char: forbid direct chardevice access for hotswap devices Anton Nefedov
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel
  Cc: den, pbonzini, marcandre.lureau, Anton Nefedov, Dr . David Alan Gilbert

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 chardev/char.c        |  4 ++--
 hmp-commands.hx       | 16 ++++++++++++++++
 hmp.c                 | 34 ++++++++++++++++++++++++++++++++++
 hmp.h                 |  1 +
 include/sysemu/char.h | 12 ++++++++++++
 5 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index bac5e1c..0483f19 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -880,8 +880,8 @@ help_string_append(const char *name, void *opaque)
     g_string_append_printf(str, "\n%s", name);
 }
 
-static ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
-                                           Error **errp)
+ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
+                                    Error **errp)
 {
     Error *local_err = NULL;
     const ChardevClass *cc;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 0aca984..0f2a059 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1727,6 +1727,22 @@ chardev_add accepts the same parameters as the -chardev command line switch.
 ETEXI
 
     {
+        .name       = "chardev-change",
+        .args_type  = "id:s,args:s",
+        .params     = "id args",
+        .help       = "change chardev",
+        .cmd        = hmp_chardev_change,
+    },
+
+STEXI
+@item chardev-change args
+@findex chardev-change
+chardev_change accepts existing chardev @var{id} and then the same arguments
+as the -chardev command line switch (except for "id").
+
+ETEXI
+
+    {
         .name       = "chardev-remove",
         .args_type  = "id:s",
         .params     = "id",
diff --git a/hmp.c b/hmp.c
index 3dceaf8..f7d0b38 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2209,6 +2209,40 @@ void hmp_chardev_add(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+void hmp_chardev_change(Monitor *mon, const QDict *qdict)
+{
+    const char *args = qdict_get_str(qdict, "args");
+    const char *id;
+    Error *err = NULL;
+    ChardevBackend *backend = NULL;
+    ChardevReturn *ret = NULL;
+    QemuOpts *opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"), args,
+                                             true);
+    if (!opts) {
+        error_setg(&err, "Parsing chardev args failed");
+        goto end;
+    }
+
+    id = qdict_get_str(qdict, "id");
+    if (qemu_opts_id(opts)) {
+        error_setg(&err, "Unexpected 'id' parameter");
+        goto end;
+    }
+
+    backend = qemu_chr_parse_opts(opts, &err);
+    if (!backend) {
+        goto end;
+    }
+
+    ret = qmp_chardev_change(id, backend, &err);
+
+end:
+    qapi_free_ChardevReturn(ret);
+    qapi_free_ChardevBackend(backend);
+    qemu_opts_del(opts);
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
 {
     Error *local_err = NULL;
diff --git a/hmp.h b/hmp.h
index d8b94ce..23e035c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -102,6 +102,7 @@ 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_change(Monitor *mon, const QDict *qdict);
 void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
 void hmp_qemu_io(Monitor *mon, const QDict *qdict);
 void hmp_cpu_add(Monitor *mon, const QDict *qdict);
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 68c7876..92ae57e 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -132,6 +132,18 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
 void qemu_chr_parse_common(QemuOpts *opts, ChardevCommon *backend);
 
 /**
+ * @qemu_chr_parse_opts:
+ *
+ * Parse the options to the ChardevBackend struct.
+ *
+ * @opts
+ *
+ * Returns: a new backend
+ */
+ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
+                                    Error **errp);
+
+/**
  * @qemu_chr_new:
  *
  * Create a new character backend from a URI.
-- 
2.7.4

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

* [Qemu-devel] [PATCH v2 5/9] char: forbid direct chardevice access for hotswap devices
  2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
                   ` (3 preceding siblings ...)
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 4/9] hmp: add hmp analogue for qmp-chardev-change Anton Nefedov
@ 2017-05-19 12:47 ` Anton Nefedov
  2017-05-25 14:31   ` Marc-André Lureau
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 6/9] virtio-console: chardev hotswap support Anton Nefedov
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov

qemu_chr_fe_get_driver() is unsafe, frontends with hotswap support
should not access CharDriver ptr directly as CharDriver might change.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 chardev/char.c        |  7 +++++++
 include/sysemu/char.h | 10 ++++++++++
 2 files changed, 17 insertions(+)

diff --git a/chardev/char.c b/chardev/char.c
index 0483f19..36d6f36 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -484,9 +484,16 @@ static Notifier muxes_realize_notify = {
 
 Chardev *qemu_chr_fe_get_driver(CharBackend *be)
 {
+    /* this is unsafe for the users that support chardev hotswap */
+    assert(be->chr_be_change == NULL);
     return be->chr;
 }
 
+bool qemu_chr_fe_backend_connected(CharBackend *be)
+{
+    return !!be->chr;
+}
+
 static bool fe_connect(CharBackend *b, Chardev *s, Error **errp)
 {
     int tag = 0;
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 92ae57e..fa21535 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -404,10 +404,20 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp);
  *
  * Returns the driver associated with a CharBackend or NULL if no
  * associated Chardev.
+ * Note: avoid this function as the driver should never be accessed directly,
+ *       especially by the frontends that support chardevice hotswap.
+ *       Consider qemu_chr_fe_backend_connected() to check for driver existence
  */
 Chardev *qemu_chr_fe_get_driver(CharBackend *be);
 
 /**
+ * @qemu_chr_fe_backend_connected:
+ *
+ * Returns true if there is a chardevice associated with @be.
+ */
+bool qemu_chr_fe_backend_connected(CharBackend *be);
+
+/**
  * @qemu_chr_fe_deinit:
  *
  * Dissociate the CharBackend from the Chardev.
-- 
2.7.4

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

* [Qemu-devel] [PATCH v2 6/9] virtio-console: chardev hotswap support
  2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
                   ` (4 preceding siblings ...)
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 5/9] char: forbid direct chardevice access for hotswap devices Anton Nefedov
@ 2017-05-19 12:47 ` Anton Nefedov
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 7/9] serial: move TIOCM update to a separate function Anton Nefedov
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov, Amit Shah

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
CC: Amit Shah <amit@kernel.org>
---
 hw/char/virtio-console.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index cf7331d..bd74669 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -49,7 +49,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port,
     VirtConsole *vcon = VIRTIO_CONSOLE(port);
     ssize_t ret;
 
-    if (!qemu_chr_fe_get_driver(&vcon->chr)) {
+    if (!qemu_chr_fe_backend_connected(&vcon->chr)) {
         /* If there's no backend, we can just say we consumed all data. */
         return len;
     }
@@ -163,12 +163,35 @@ static void chr_event(void *opaque, int event)
     }
 }
 
+static int chr_be_change(void *opaque)
+{
+    VirtConsole *vcon = opaque;
+    VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(vcon);
+    VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+
+    if (k->is_console) {
+        qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
+                                 NULL, chr_be_change, vcon, NULL, true);
+    } else {
+        qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
+                                 chr_event, chr_be_change, vcon, NULL, false);
+    }
+
+    if (vcon->watch) {
+        g_source_remove(vcon->watch);
+        vcon->watch = qemu_chr_fe_add_watch(&vcon->chr,
+                                            G_IO_OUT | G_IO_HUP,
+                                            chr_write_unblocked, vcon);
+    }
+
+    return 0;
+}
+
 static void virtconsole_realize(DeviceState *dev, Error **errp)
 {
     VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
     VirtConsole *vcon = VIRTIO_CONSOLE(dev);
     VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(dev);
-    Chardev *chr = qemu_chr_fe_get_driver(&vcon->chr);
 
     if (port->id == 0 && !k->is_console) {
         error_setg(errp, "Port number 0 on virtio-serial devices reserved "
@@ -176,7 +199,7 @@ static void virtconsole_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (chr) {
+    if (qemu_chr_fe_backend_connected(&vcon->chr)) {
         /*
          * For consoles we don't block guest data transfer just
          * because nothing is connected - we'll just let it go
@@ -188,11 +211,13 @@ static void virtconsole_realize(DeviceState *dev, Error **errp)
          */
         if (k->is_console) {
             qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
-                                     NULL, NULL, vcon, NULL, true);
+                                     NULL, chr_be_change,
+                                     vcon, NULL, true);
             virtio_serial_open(port);
         } else {
             qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
-                                     chr_event, NULL, vcon, NULL, false);
+                                     chr_event, chr_be_change,
+                                     vcon, NULL, false);
         }
     }
 }
-- 
2.7.4

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

* [Qemu-devel] [PATCH v2 7/9] serial: move TIOCM update to a separate function
  2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
                   ` (5 preceding siblings ...)
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 6/9] virtio-console: chardev hotswap support Anton Nefedov
@ 2017-05-19 12:47 ` Anton Nefedov
  2017-05-25 14:36   ` Marc-André Lureau
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 8/9] serial: chardev hotswap support Anton Nefedov
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 9/9] char: avoid chardevice direct access Anton Nefedov
  8 siblings, 1 reply; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel
  Cc: den, pbonzini, marcandre.lureau, Anton Nefedov, Michael S . Tsirkin

will be used by the following patch

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index d8d34d0..1e6bdeb 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -312,6 +312,24 @@ static void serial_write_fcr(SerialState *s, uint8_t val)
     }
 }
 
+static void serial_update_tiocm(SerialState *s)
+{
+    int flags;
+
+    qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_GET_TIOCM, &flags);
+
+    flags &= ~(CHR_TIOCM_RTS | CHR_TIOCM_DTR);
+
+    if (s->mcr & UART_MCR_RTS) {
+        flags |= CHR_TIOCM_RTS;
+    }
+    if (s->mcr & UART_MCR_DTR) {
+        flags |= CHR_TIOCM_DTR;
+    }
+
+    qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
+}
+
 static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
                                 unsigned size)
 {
@@ -426,24 +444,13 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
         break;
     case 4:
         {
-            int flags;
             int old_mcr = s->mcr;
             s->mcr = val & 0x1f;
             if (val & UART_MCR_LOOP)
                 break;
 
             if (s->poll_msl >= 0 && old_mcr != s->mcr) {
-
-                qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_GET_TIOCM, &flags);
-
-                flags &= ~(CHR_TIOCM_RTS | CHR_TIOCM_DTR);
-
-                if (val & UART_MCR_RTS)
-                    flags |= CHR_TIOCM_RTS;
-                if (val & UART_MCR_DTR)
-                    flags |= CHR_TIOCM_DTR;
-
-                qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
+                serial_update_tiocm(s);
                 /* Update the modem status after a one-character-send wait-time, since there may be a response
                    from the device/computer at the other end of the serial line */
                 timer_mod(s->modem_status_poll, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->char_transmit_time);
-- 
2.7.4

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

* [Qemu-devel] [PATCH v2 8/9] serial: chardev hotswap support
  2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
                   ` (6 preceding siblings ...)
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 7/9] serial: move TIOCM update to a separate function Anton Nefedov
@ 2017-05-19 12:47 ` Anton Nefedov
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 9/9] char: avoid chardevice direct access Anton Nefedov
  8 siblings, 0 replies; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel
  Cc: den, pbonzini, marcandre.lureau, Anton Nefedov, Michael S . Tsirkin

for a backend change, a number of ioctls has to be replayed to sync
the current setup of a frontend to a backend tty. This is hopefully
enough so we don't have to track, store and replay the whole original
control byte sequence.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
CC: Michael S. Tsirkin <mst@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 1e6bdeb..ed01637 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -891,9 +891,37 @@ static void serial_reset(void *opaque)
     s->msr &= ~UART_MSR_ANY_DELTA;
 }
 
+static int serial_be_change(void *opaque)
+{
+    SerialState *s = opaque;
+
+    qemu_chr_fe_set_handlers(&s->chr, serial_can_receive1, serial_receive1,
+                             serial_event, serial_be_change, s, NULL, true);
+
+    serial_update_parameters(s);
+
+    qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_BREAK,
+                      &s->last_break_enable);
+
+    s->poll_msl = (s->ier & UART_IER_MSI) ? 1 : 0;
+    serial_update_msl(s);
+
+    if (s->poll_msl >= 0 && !(s->mcr & UART_MCR_LOOP)) {
+        serial_update_tiocm(s);
+    }
+
+    if (s->watch_tag > 0) {
+        g_source_remove(s->watch_tag);
+        s->watch_tag = qemu_chr_fe_add_watch(&s->chr, G_IO_OUT | G_IO_HUP,
+                                             serial_watch_cb, s);
+    }
+
+    return 0;
+}
+
 void serial_realize_core(SerialState *s, Error **errp)
 {
-    if (!qemu_chr_fe_get_driver(&s->chr)) {
+    if (!qemu_chr_fe_backend_connected(&s->chr)) {
         error_setg(errp, "Can't create serial device, empty char device");
         return;
     }
@@ -904,7 +932,7 @@ void serial_realize_core(SerialState *s, Error **errp)
     qemu_register_reset(serial_reset, s);
 
     qemu_chr_fe_set_handlers(&s->chr, serial_can_receive1, serial_receive1,
-                             serial_event, NULL, s, NULL, true);
+                             serial_event, serial_be_change, s, NULL, true);
     fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
     fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
     serial_reset(s);
-- 
2.7.4

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

* [Qemu-devel] [PATCH v2 9/9] char: avoid chardevice direct access
  2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
                   ` (7 preceding siblings ...)
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 8/9] serial: chardev hotswap support Anton Nefedov
@ 2017-05-19 12:47 ` Anton Nefedov
  2017-05-25 14:32   ` Marc-André Lureau
  8 siblings, 1 reply; 17+ messages in thread
From: Anton Nefedov @ 2017-05-19 12:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov

frontends should avoid accessing CharDriver struct where possible

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 chardev/char.c              | 5 +++++
 gdbstub.c                   | 2 +-
 hw/arm/strongarm.c          | 2 +-
 hw/char/cadence_uart.c      | 2 +-
 hw/char/debugcon.c          | 2 +-
 hw/char/escc.c              | 6 +++---
 hw/char/exynos4210_uart.c   | 2 +-
 hw/char/grlib_apbuart.c     | 2 +-
 hw/char/ipoctal232.c        | 2 +-
 hw/char/parallel.c          | 2 +-
 hw/char/sclpconsole-lm.c    | 2 +-
 hw/char/sclpconsole.c       | 2 +-
 hw/char/sh_serial.c         | 2 +-
 hw/char/spapr_vty.c         | 2 +-
 hw/char/terminal3270.c      | 2 +-
 hw/char/xen_console.c       | 2 +-
 hw/ipmi/ipmi_bmc_extern.c   | 2 +-
 hw/misc/ivshmem.c           | 4 ++--
 hw/usb/ccid-card-passthru.c | 4 ++--
 hw/usb/dev-serial.c         | 5 ++---
 hw/usb/redirect.c           | 5 ++---
 include/sysemu/char.h       | 7 +++++++
 net/filter-mirror.c         | 2 +-
 23 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/chardev/char.c b/chardev/char.c
index 36d6f36..e43d840 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -494,6 +494,11 @@ bool qemu_chr_fe_backend_connected(CharBackend *be)
     return !!be->chr;
 }
 
+bool qemu_chr_fe_backend_open(CharBackend *be)
+{
+    return be->chr && be->chr->be_open;
+}
+
 static bool fe_connect(CharBackend *b, Chardev *s, Error **errp)
 {
     int tag = 0;
diff --git a/gdbstub.c b/gdbstub.c
index 1ac0489..68cbe8a 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2001,7 +2001,7 @@ int gdbserver_start(const char *device)
                                    NULL, &error_abort);
         monitor_init(mon_chr, 0);
     } else {
-        if (qemu_chr_fe_get_driver(&s->chr)) {
+        if (qemu_chr_fe_backend_connected(&s->chr)) {
             object_unparent(OBJECT(qemu_chr_fe_get_driver(&s->chr)));
         }
         mon_chr = s->mon_chr;
diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index bec093d..9d7cf21 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -1105,7 +1105,7 @@ static void strongarm_uart_tx(void *opaque)
 
     if (s->utcr3 & UTCR3_LBM) /* loopback */ {
         strongarm_uart_receive(s, &s->tx_fifo[s->tx_start], 1);
-    } else if (qemu_chr_fe_get_driver(&s->chr)) {
+    } else if (qemu_chr_fe_backend_connected(&s->chr)) {
         /* XXX this blocks entire thread. Rewrite to use
          * qemu_chr_fe_write and background I/O callbacks */
         qemu_chr_fe_write_all(&s->chr, &s->tx_fifo[s->tx_start], 1);
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 71867b3..19636c0 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -278,7 +278,7 @@ static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
     int ret;
 
     /* instant drain the fifo when there's no back-end */
-    if (!qemu_chr_fe_get_driver(&s->chr)) {
+    if (!qemu_chr_fe_backend_connected(&s->chr)) {
         s->tx_count = 0;
         return FALSE;
     }
diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
index 6d95297..bd0d4f0 100644
--- a/hw/char/debugcon.c
+++ b/hw/char/debugcon.c
@@ -87,7 +87,7 @@ static const MemoryRegionOps debugcon_ops = {
 
 static void debugcon_realize_core(DebugconState *s, Error **errp)
 {
-    if (!qemu_chr_fe_get_driver(&s->chr)) {
+    if (!qemu_chr_fe_backend_connected(&s->chr)) {
         error_setg(errp, "Can't create debugcon device, empty char device");
         return;
     }
diff --git a/hw/char/escc.c b/hw/char/escc.c
index aa882b6..dbbeb4a 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -416,7 +416,7 @@ static void escc_update_parameters(ChannelState *s)
     int speed, parity, data_bits, stop_bits;
     QEMUSerialSetParams ssp;
 
-    if (!qemu_chr_fe_get_driver(&s->chr) || s->type != ser)
+    if (!qemu_chr_fe_backend_connected(&s->chr) || s->type != ser)
         return;
 
     if (s->wregs[W_TXCTRL1] & TXCTRL1_PAREN) {
@@ -556,7 +556,7 @@ static void escc_mem_write(void *opaque, hwaddr addr,
         trace_escc_mem_writeb_data(CHN_C(s), val);
         s->tx = val;
         if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
-            if (qemu_chr_fe_get_driver(&s->chr)) {
+            if (qemu_chr_fe_backend_connected(&s->chr)) {
                 /* XXX this blocks entire thread. Rewrite to use
                  * qemu_chr_fe_write and background I/O callbacks */
                 qemu_chr_fe_write_all(&s->chr, &s->tx, 1);
@@ -1012,7 +1012,7 @@ static void escc_realize(DeviceState *dev, Error **errp)
                           ESCC_SIZE << s->it_shift);
 
     for (i = 0; i < 2; i++) {
-        if (qemu_chr_fe_get_driver(&s->chn[i].chr)) {
+        if (qemu_chr_fe_backend_connected(&s->chn[i].chr)) {
             s->chn[i].clock = s->frequency / 2;
             qemu_chr_fe_set_handlers(&s->chn[i].chr, serial_can_receive,
                                      serial_receive1, serial_event, NULL,
diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 7ef4ea5..2b0576c 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -379,7 +379,7 @@ static void exynos4210_uart_write(void *opaque, hwaddr offset,
         break;
 
     case UTXH:
-        if (qemu_chr_fe_get_driver(&s->chr)) {
+        if (qemu_chr_fe_backend_connected(&s->chr)) {
             s->reg[I_(UTRSTAT)] &= ~(UTRSTAT_TRANSMITTER_EMPTY |
                     UTRSTAT_Tx_BUFFER_EMPTY);
             ch = (uint8_t)val;
diff --git a/hw/char/grlib_apbuart.c b/hw/char/grlib_apbuart.c
index 610a317..1cb9026 100644
--- a/hw/char/grlib_apbuart.c
+++ b/hw/char/grlib_apbuart.c
@@ -201,7 +201,7 @@ static void grlib_apbuart_write(void *opaque, hwaddr addr,
     case DATA_OFFSET:
     case DATA_OFFSET + 3:       /* When only one byte write */
         /* Transmit when character device available and transmitter enabled */
-        if (qemu_chr_fe_get_driver(&uart->chr) &&
+        if (qemu_chr_fe_backend_connected(&uart->chr) &&
             (uart->control & UART_TRANSMIT_ENABLE)) {
             c = value & 0xFF;
             /* XXX this blocks entire thread. Rewrite to use
diff --git a/hw/char/ipoctal232.c b/hw/char/ipoctal232.c
index 734e42c..b5ada43 100644
--- a/hw/char/ipoctal232.c
+++ b/hw/char/ipoctal232.c
@@ -542,7 +542,7 @@ static void ipoctal_realize(DeviceState *dev, Error **errp)
         ch->ipoctal = s;
 
         /* Redirect IP-Octal channels to host character devices */
-        if (qemu_chr_fe_get_driver(&ch->dev)) {
+        if (qemu_chr_fe_backend_connected(&ch->dev)) {
             qemu_chr_fe_set_handlers(&ch->dev, hostdev_can_receive,
                                      hostdev_receive, hostdev_event,
                                      NULL, ch, NULL, true);
diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index c71a4a0..b3ed117 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -512,7 +512,7 @@ static void parallel_isa_realizefn(DeviceState *dev, Error **errp)
     int base;
     uint8_t dummy;
 
-    if (!qemu_chr_fe_get_driver(&s->chr)) {
+    if (!qemu_chr_fe_backend_connected(&s->chr)) {
         error_setg(errp, "Can't create parallel device, empty char device");
         return;
     }
diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
index ed1e2c5..ff54d19 100644
--- a/hw/char/sclpconsole-lm.c
+++ b/hw/char/sclpconsole-lm.c
@@ -195,7 +195,7 @@ static int write_console_data(SCLPEvent *event, const uint8_t *buf, int len)
 {
     SCLPConsoleLM *scon = SCLPLM_CONSOLE(event);
 
-    if (!qemu_chr_fe_get_driver(&scon->chr)) {
+    if (!qemu_chr_fe_backend_connected(&scon->chr)) {
         /* If there's no backend, we can just say we consumed all data. */
         return len;
     }
diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
index 9a65010..7e23d09 100644
--- a/hw/char/sclpconsole.c
+++ b/hw/char/sclpconsole.c
@@ -163,7 +163,7 @@ static ssize_t write_console_data(SCLPEvent *event, const uint8_t *buf,
 {
     SCLPConsole *scon = SCLP_CONSOLE(event);
 
-    if (!qemu_chr_fe_get_driver(&scon->chr)) {
+    if (!qemu_chr_fe_backend_connected(&scon->chr)) {
         /* If there's no backend, we can just say we consumed all data. */
         return len;
     }
diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index c352337..868773f 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -110,7 +110,7 @@ static void sh_serial_write(void *opaque, hwaddr offs,
         }
         return;
     case 0x0c: /* FTDR / TDR */
-        if (qemu_chr_fe_get_driver(&s->chr)) {
+        if (qemu_chr_fe_backend_connected(&s->chr)) {
             ch = val;
             /* XXX this blocks entire thread. Rewrite to use
              * qemu_chr_fe_write and background I/O callbacks */
diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index 9cdc0e0..6d1ccff 100644
--- a/hw/char/spapr_vty.c
+++ b/hw/char/spapr_vty.c
@@ -78,7 +78,7 @@ static void spapr_vty_realize(VIOsPAPRDevice *sdev, Error **errp)
 {
     VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
 
-    if (!qemu_chr_fe_get_driver(&dev->chardev)) {
+    if (!qemu_chr_fe_backend_connected(&dev->chardev)) {
         error_setg(errp, "chardev property not set");
         return;
     }
diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index 943a0f3..62803e5 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -239,7 +239,7 @@ static int write_payload_3270(EmulatedCcw3270Device *dev, uint8_t cmd,
             return 0;
         }
     }
-    if (!qemu_chr_fe_get_driver(&t->chr)) {
+    if (!qemu_chr_fe_backend_connected(&t->chr)) {
         /* We just say we consumed all data if there's no backend. */
         return count;
     }
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index cb7975f..b066176 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -150,7 +150,7 @@ static void xencons_send(struct XenConsole *con)
     ssize_t len, size;
 
     size = con->buffer.size - con->buffer.consumed;
-    if (qemu_chr_fe_get_driver(&con->chr)) {
+    if (qemu_chr_fe_backend_connected(&con->chr)) {
         len = qemu_chr_fe_write(&con->chr,
                                 con->buffer.data + con->buffer.consumed,
                                 size);
diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index 6f2339d..0ca9e92 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -447,7 +447,7 @@ static void ipmi_bmc_extern_realize(DeviceState *dev, Error **errp)
 {
     IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev);
 
-    if (!qemu_chr_fe_get_driver(&ibe->chr)) {
+    if (!qemu_chr_fe_backend_connected(&ibe->chr)) {
         error_setg(errp, "IPMI external bmc requires chardev attribute");
         return;
     }
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index e2dece8..a1be4bb 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -1130,7 +1130,7 @@ static void ivshmem_doorbell_realize(PCIDevice *dev, Error **errp)
 {
     IVShmemState *s = IVSHMEM_COMMON(dev);
 
-    if (!qemu_chr_fe_get_driver(&s->server_chr)) {
+    if (!qemu_chr_fe_backend_connected(&s->server_chr)) {
         error_setg(errp, "You must specify a 'chardev'");
         return;
     }
@@ -1259,7 +1259,7 @@ static void ivshmem_realize(PCIDevice *dev, Error **errp)
                      " or ivshmem-doorbell instead");
     }
 
-    if (!!qemu_chr_fe_get_driver(&s->server_chr) + !!s->shmobj != 1) {
+    if (qemu_chr_fe_backend_connected(&s->server_chr) + !!s->shmobj != 1) {
         error_setg(errp, "You must specify either 'shm' or 'chardev'");
         return;
     }
diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
index 9ace5ac..c0f8acd 100644
--- a/hw/usb/ccid-card-passthru.c
+++ b/hw/usb/ccid-card-passthru.c
@@ -325,7 +325,7 @@ static void passthru_apdu_from_guest(
 {
     PassthruState *card = PASSTHRU_CCID_CARD(base);
 
-    if (!qemu_chr_fe_get_driver(&card->cs)) {
+    if (!qemu_chr_fe_backend_connected(&card->cs)) {
         printf("ccid-passthru: no chardev, discarding apdu length %d\n", len);
         return;
     }
@@ -346,7 +346,7 @@ static int passthru_initfn(CCIDCardState *base)
 
     card->vscard_in_pos = 0;
     card->vscard_in_hdr = 0;
-    if (qemu_chr_fe_get_driver(&card->cs)) {
+    if (qemu_chr_fe_backend_connected(&card->cs)) {
         DPRINTF(card, D_INFO, "initing chardev\n");
         qemu_chr_fe_set_handlers(&card->cs,
             ccid_card_vscard_can_read,
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index e6b2c7c..7aa7290 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -483,13 +483,12 @@ static void usb_serial_realize(USBDevice *dev, Error **errp)
 {
     USBSerialState *s = USB_SERIAL_DEV(dev);
     Error *local_err = NULL;
-    Chardev *chr = qemu_chr_fe_get_driver(&s->cs);
 
     usb_desc_create_serial(dev);
     usb_desc_init(dev);
     dev->auto_attach = 0;
 
-    if (!chr) {
+    if (!qemu_chr_fe_backend_connected(&s->cs)) {
         error_setg(errp, "Property chardev is required");
         return;
     }
@@ -504,7 +503,7 @@ static void usb_serial_realize(USBDevice *dev, Error **errp)
                              usb_serial_event, NULL, s, NULL, true);
     usb_serial_handle_reset(dev);
 
-    if (chr->be_open && !dev->attached) {
+    if (qemu_chr_fe_backend_open(&s->cs) && !dev->attached) {
         usb_device_attach(dev, &error_abort);
     }
 }
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 1e9bf69..6992d92 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -273,10 +273,9 @@ static gboolean usbredir_write_unblocked(GIOChannel *chan, GIOCondition cond,
 static int usbredir_write(void *priv, uint8_t *data, int count)
 {
     USBRedirDevice *dev = priv;
-    Chardev *chr = qemu_chr_fe_get_driver(&dev->cs);
     int r;
 
-    if (!chr->be_open) {
+    if (!qemu_chr_fe_backend_open(&dev->cs)) {
         return 0;
     }
 
@@ -1366,7 +1365,7 @@ static void usbredir_realize(USBDevice *udev, Error **errp)
     USBRedirDevice *dev = USB_REDIRECT(udev);
     int i;
 
-    if (!qemu_chr_fe_get_driver(&dev->cs)) {
+    if (!qemu_chr_fe_backend_connected(&dev->cs)) {
         error_setg(errp, QERR_MISSING_PARAMETER, "chardev");
         return;
     }
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index fa21535..e1a0e52 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -418,6 +418,13 @@ Chardev *qemu_chr_fe_get_driver(CharBackend *be);
 bool qemu_chr_fe_backend_connected(CharBackend *be);
 
 /**
+ * @qemu_chr_fe_backend_open:
+ *
+ * Returns true if chardevice associated with @be is open.
+ */
+bool qemu_chr_fe_backend_open(CharBackend *be);
+
+/**
  * @qemu_chr_fe_deinit:
  *
  * Dissociate the CharBackend from the Chardev.
diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index 06321d7..72a6077 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -163,7 +163,7 @@ static ssize_t filter_redirector_receive_iov(NetFilterState *nf,
     MirrorState *s = FILTER_REDIRECTOR(nf);
     int ret;
 
-    if (qemu_chr_fe_get_driver(&s->chr_out)) {
+    if (qemu_chr_fe_backend_connected(&s->chr_out)) {
         ret = filter_mirror_send(&s->chr_out, iov, iovcnt);
         if (ret) {
             error_report("filter_mirror_send failed(%s)", strerror(-ret));
-- 
2.7.4

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

* Re: [Qemu-devel] [PATCH v2 3/9] char: chardevice hotswap
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 3/9] char: chardevice hotswap Anton Nefedov
@ 2017-05-25 14:29   ` Marc-André Lureau
  2017-05-25 17:44     ` Anton Nefedov
  0 siblings, 1 reply; 17+ messages in thread
From: Marc-André Lureau @ 2017-05-25 14:29 UTC (permalink / raw)
  To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den

Hi

On Fri, May 19, 2017 at 4:51 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:

> This patch adds a possibility to change a char device without a frontend
> removal.
>
> 1. Ideally, it would have to happen transparently to a frontend, i.e.
> frontend would continue its regular operation.
> However, backends are not stateless and are set up by the frontends
> via qemu_chr_fe_<> functions, and it's not (generally) possible to replay
> that setup entirely in a backend code, as different chardevs respond
> to the setup calls differently, so do frontends work differently basing
> on those setup responses.
> Moreover, some frontend can generally get and save the backend pointer
> (qemu_chr_fe_get_driver()), and it will become invalid after backend
> change.
>
> So, a frontend which would like to support chardev hotswap has to register
> a "backend change" handler, and redo its backend setup there.
>
> 2. Write path can be used by multiple threads and thus protected with
> chr_write_lock.
> So hotswap also has to be protected so write functions won't access
> a backend being replaced.
>
>
Why not use the chr_write_lock then? (rename it chr_lock?)


> 3. Hotswap function can be called from e.g. a read handler of a monitor
> socket. This can cause troubles so it's safer to defer execution to
> a bottom-half (however, it means we cannot return some of the errors
> synchronously - but most of them we can)
>

What kind of troubles?

>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  chardev/char.c        | 147
> ++++++++++++++++++++++++++++++++++++++++++++++----
>  include/sysemu/char.h |  10 ++++
>  qapi-schema.json      |  40 ++++++++++++++
>  3 files changed, 187 insertions(+), 10 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index ae60950..bac5e1c 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -132,12 +132,16 @@ static bool qemu_chr_replay(Chardev *chr)
>
>  int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
>  {
> -    Chardev *s = be->chr;
> +    Chardev *s;
>      ChardevClass *cc;
>      int ret;
>
> +    qemu_mutex_lock(&be->chr_lock);
> +    s = be->chr;
> +
>      if (!s) {
> -        return 0;
> +        ret = 0;
> +        goto end;
>      }
>
>      if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_PLAY) {
> @@ -145,7 +149,7 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t
> *buf, int len)
>          replay_char_write_event_load(&ret, &offset);
>          assert(offset <= len);
>          qemu_chr_fe_write_buffer(s, buf, offset, &offset);
> -        return ret;
> +        goto end;
>      }
>
>      cc = CHARDEV_GET_CLASS(s);
> @@ -161,7 +165,9 @@ int qemu_chr_fe_write(CharBackend *be, const uint8_t
> *buf, int len)
>      if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_RECORD) {
>          replay_char_write_event_save(ret, ret < 0 ? 0 : ret);
>      }
> -
> +
> +end:
> +    qemu_mutex_unlock(&be->chr_lock);
>      return ret;
>  }
>
> @@ -191,13 +197,16 @@ int qemu_chr_write_all(Chardev *s, const uint8_t
> *buf, int len)
>
>  int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len)
>  {
> -    Chardev *s = be->chr;
> +    Chardev *s;
> +    int ret;
>
> -    if (!s) {
> -        return 0;
> -    }
> +    qemu_mutex_lock(&be->chr_lock);
>
> -    return qemu_chr_write_all(s, buf, len);
> +    s = be->chr;
> +    ret = s ? qemu_chr_write_all(s, buf, len) : 0;
> +
> +    qemu_mutex_unlock(&be->chr_lock);
> +    return ret;
>  }
>
>  int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
> @@ -478,7 +487,7 @@ Chardev *qemu_chr_fe_get_driver(CharBackend *be)
>      return be->chr;
>  }
>
> -bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
> +static bool fe_connect(CharBackend *b, Chardev *s, Error **errp)
>  {
>

I would rather keep a qemu_chr prefix for consistency


>      int tag = 0;
>
> @@ -507,6 +516,17 @@ unavailable:
>      return false;
>  }
>
> +bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
> +{
> +    if (!fe_connect(b, s, errp)) {
> +        return false;
> +    }
> +
> +    qemu_mutex_init(&b->chr_lock);
> +    b->hotswap_bh = NULL;
> +    return true;
> +}
> +
>  static bool qemu_chr_is_busy(Chardev *s)
>  {
>      if (CHARDEV_IS_MUX(s)) {
> @@ -531,6 +551,10 @@ void qemu_chr_fe_deinit(CharBackend *b)
>              d->backends[b->tag] = NULL;
>          }
>          b->chr = NULL;
> +        qemu_mutex_destroy(&b->chr_lock);
> +        if (b->hotswap_bh) {
> +            qemu_bh_delete(b->hotswap_bh);
>

Could there be a chr_new leak here?


> +        }
>      }
>  }
>
> @@ -1308,6 +1332,109 @@ ChardevReturn *qmp_chardev_add(const char *id,
> ChardevBackend *backend,
>      return ret;
>  }
>
> +static void chardev_change_bh(void *opaque)
> +{
> +    Chardev *chr_new = opaque;
> +    const char *id = chr_new->label;
> +    Chardev *chr = qemu_chr_find(id);
>

Could chr be gone in the meantime? potentially


> +    CharBackend *be = chr->be;
> +    bool closed_sent = false;
> +
> +    if (!be) {
> +        /* disconnected since we checked: ok, less work for us */
> +        goto end;
> +    }
> +
> +    if (chr->be_open && !chr_new->be_open) {
> +        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> +        closed_sent = true;
> +    }
> +
> +    qemu_mutex_lock(&be->chr_lock);
> +    chr->be = NULL;
> +    fe_connect(be, chr_new, &error_abort);
> +
> +    if (be->chr_be_change(be->opaque) < 0) {
> +        error_report("Chardev '%s' change failed", id);
> +        fe_connect(be, chr, &error_abort);
> +        qemu_mutex_unlock(&be->chr_lock);
> +        if (closed_sent) {
> +            qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> +        }
> +        object_unref(OBJECT(chr_new));
> +        return;
> +    }
> +    qemu_mutex_unlock(&be->chr_lock);
> +
> +end:
> +    object_unparent(OBJECT(chr));
> +    object_property_add_child(get_chardevs_root(), id, OBJECT(chr_new),
> +                              &error_abort);
> +    object_unref(OBJECT(chr_new));
> +}
> +
> +ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> +                                  Error **errp)
> +{
> +    const ChardevClass *cc;
> +    Chardev *chr, *chr_new;
> +    ChardevReturn *ret;
> +
> +    chr = qemu_chr_find(id);
> +    if (!chr) {
> +        error_setg(errp, "Chardev '%s' does not exist", id);
> +        return NULL;
> +    }
> +
> +    if (CHARDEV_IS_MUX(chr)) {
> +        error_setg(errp, "Mux device hotswap not supported yet");
> +        return NULL;
> +    }
> +
> +    if (qemu_chr_replay(chr)) {
> +        error_setg(errp,
> +            "Chardev '%s' cannot be changed in record/replay mode", id);
> +        return NULL;
> +    }
> +
> +    if (!chr->be) {
> +        /* easy case */
> +        object_unparent(OBJECT(chr));
> +        return qmp_chardev_add(id, backend, errp);
> +    }
> +
> +    if (!chr->be->chr_be_change) {
> +        error_setg(errp, "Chardev user does not support chardev hotswap");
> +        return NULL;
> +    }
> +
> +    cc = char_get_class(ChardevBackendKind_lookup[backend->type], errp);
> +    if (!cc) {
> +        return NULL;
> +    }
> +
> +    chr_new = qemu_chardev_new(NULL,
> object_class_get_name(OBJECT_CLASS(cc)),
> +                               backend, errp);
> +    chr_new->label = g_strdup(id);
>

move it below the check for !chr_new


> +    if (!chr_new) {
> +        return NULL;
> +    }
> +
> +    if (chr->be->hotswap_bh) {
> +        qemu_bh_delete(chr->be->hotswap_bh);
> +    }
>

Is it necessary to delete and recreate the bh? Could there be a chr_new
leak if the bh didn't run? It's probably best to deny backend change while
one is going on.


> +    chr->be->hotswap_bh = qemu_bh_new(chardev_change_bh, chr_new);
> +    qemu_bh_schedule(chr->be->hotswap_bh);
> +
> +    ret = g_new0(ChardevReturn, 1);
> +    if (CHARDEV_IS_PTY(chr_new)) {
> +        ret->pty = g_strdup(chr_new->filename + 4);
> +        ret->has_pty = true;
> +    }
> +
> +    return ret;
>

ah, potentially a case where qmp-async would be better..

+}
> +
>  void qmp_chardev_remove(const char *id, Error **errp)
>  {
>      Chardev *chr;
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 9f8df07..68c7876 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -92,6 +92,8 @@ typedef struct CharBackend {
>      void *opaque;
>      int tag;
>      int fe_open;
> +    QemuMutex chr_lock;
> +    QEMUBH *hotswap_bh;
>  } CharBackend;
>
>  struct Chardev {
> @@ -141,6 +143,14 @@ void qemu_chr_parse_common(QemuOpts *opts,
> ChardevCommon *backend);
>   */
>  Chardev *qemu_chr_new(const char *label, const char *filename);
>
> +/**
> + * @qemu_chr_change:
> + *
> + * Change an existing character backend
> + *
> + * @opts the new backend options
> + */
> +void qemu_chr_change(QemuOpts *opts, Error **errp);
>
>
This patch needs some test-char.c tests.


>  /**
>   * @qemu_chr_fe_disconnect:
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 80603cf..bf72166 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -5075,6 +5075,46 @@
>    'returns': 'ChardevReturn' }
>
>  ##
> +# @chardev-change:
> +#
> +# Change a character device backend
> +#
> +# @id: the chardev's ID, must exist
> +# @backend: new backend type and parameters
> +#
> +# Returns: ChardevReturn.
> +#
> +# Since: 2.10
> +#
> +# Example:
> +#
> +# -> { "execute" : "chardev-change",
> +#      "arguments" : { "id" : "baz",
> +#                      "backend" : { "type" : "pty", "data" : {} } } }
> +# <- { "return": { "pty" : "/dev/pty/42" } }
> +#
> +# -> {"execute" : "chardev-change",
> +#     "arguments" : {
> +#         "id" : "charchannel2",
> +#         "backend" : {
> +#             "type" : "socket",
> +#             "data" : {
> +#                 "addr" : {
> +#                     "type" : "unix" ,
> +#                     "data" : {
> +#                         "path" : "/tmp/charchannel2.socket"
> +#                     }
> +#                  },
> +#                  "server" : true,
> +#                  "wait" : false }}}}
> +# <- {"return": {}}
> +#
> +##
> +{ 'command': 'chardev-change', 'data': {'id'      : 'str',
> +                                        'backend' : 'ChardevBackend' },
> +  'returns': 'ChardevReturn' }
> +
> +##
>  # @chardev-remove:
>  #
>  # Remove a character device backend
> --
> 2.7.4
>
>
> --
Marc-André Lureau

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

* Re: [Qemu-devel] [PATCH v2 1/9] char: move QemuOpts->ChardevBackend translation to a separate func
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 1/9] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
@ 2017-05-25 14:29   ` Marc-André Lureau
  0 siblings, 0 replies; 17+ messages in thread
From: Marc-André Lureau @ 2017-05-25 14:29 UTC (permalink / raw)
  To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den

Hi

On Fri, May 19, 2017 at 4:51 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:

> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  chardev/char.c | 72
> +++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 46 insertions(+), 26 deletions(-)
>

Motivation is the upcoming patch for hmp_chardev_change()


>
> diff --git a/chardev/char.c b/chardev/char.c
> index 4e24dc3..684cccd 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -854,17 +854,14 @@ help_string_append(const char *name, void *opaque)
>      g_string_append_printf(str, "\n%s", name);
>  }
>
> -Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
> -                                Error **errp)
> +static ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> +                                           Error **errp)
>

while at it, make it a single line?


>  {
>      Error *local_err = NULL;
>      const ChardevClass *cc;
> -    Chardev *chr;
>      int i;
>      ChardevBackend *backend = NULL;
>      const char *name = qemu_opt_get(opts, "backend");
> -    const char *id = qemu_opts_id(opts);
> -    char *bid = NULL;
>
>      if (name == NULL) {
>          error_setg(errp, "chardev: \"%s\" missing backend",
> @@ -872,21 +869,6 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
>          return NULL;
>      }
>
> -    if (is_help_option(name)) {
> -        GString *str = g_string_new("");
> -
> -        chardev_name_foreach(help_string_append, str);
> -
> -        error_report("Available chardev backend types: %s", str->str);
> -        g_string_free(str, true);
> -        exit(0);
> -    }
> -
> -    if (id == NULL) {
> -        error_setg(errp, "chardev: no id specified");
> -        return NULL;
> -    }
> -
>      for (i = 0; i < ARRAY_SIZE(chardev_alias_table); i++) {
>          if (g_strcmp0(chardev_alias_table[i].alias, name) == 0) {
>              name = chardev_alias_table[i].typename;
> @@ -902,16 +884,12 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
>      backend = g_new0(ChardevBackend, 1);
>      backend->type = CHARDEV_BACKEND_KIND_NULL;
>
> -    if (qemu_opt_get_bool(opts, "mux", 0)) {
> -        bid = g_strdup_printf("%s-base", id);
> -    }
> -
> -    chr = NULL;
>      if (cc->parse) {
>          cc->parse(opts, backend, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
> -            goto out;
> +            qapi_free_ChardevBackend(backend);
> +            return NULL;
>          }
>      } else {
>          ChardevCommon *ccom = g_new0(ChardevCommon, 1);
> @@ -919,6 +897,48 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
>          backend->u.null.data = ccom; /* Any ChardevCommon member would
> work */
>      }
>
> +    return backend;
> +}
> +
> +Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
> +                                Error **errp)
>

same here


> +{
> +    const ChardevClass *cc;
> +    Chardev *chr = NULL;
> +    ChardevBackend *backend = NULL;
> +    const char *name = qemu_opt_get(opts, "backend");
> +    const char *id = qemu_opts_id(opts);
> +    char *bid = NULL;
> +
> +    if (name && is_help_option(name)) {
> +        GString *str = g_string_new("");
> +
> +        chardev_name_foreach(help_string_append, str);
> +
> +        error_report("Available chardev backend types: %s", str->str);
> +        g_string_free(str, true);
> +        exit(0);
> +    }
> +
> +    if (id == NULL) {
> +        error_setg(errp, "chardev: no id specified");
> +        return NULL;
> +    }
> +
> +    backend = qemu_chr_parse_opts(opts, errp);
> +    if (backend == NULL) {
> +        return NULL;
> +    }
> +
> +    cc = char_get_class(name, errp);
> +    if (cc == NULL) {
> +        goto out;
> +    }
> +
> +    if (qemu_opt_get_bool(opts, "mux", 0)) {
> +        bid = g_strdup_printf("%s-base", id);
> +    }
> +
>      chr = qemu_chardev_new(bid ? bid : id,
>                             object_class_get_name(OBJECT_CLASS(cc)),
>                             backend, errp);
> --
> 2.7.4
>
>
> --
Marc-André Lureau

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

* Re: [Qemu-devel] [PATCH v2 5/9] char: forbid direct chardevice access for hotswap devices
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 5/9] char: forbid direct chardevice access for hotswap devices Anton Nefedov
@ 2017-05-25 14:31   ` Marc-André Lureau
  0 siblings, 0 replies; 17+ messages in thread
From: Marc-André Lureau @ 2017-05-25 14:31 UTC (permalink / raw)
  To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den

On Fri, May 19, 2017 at 4:49 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:

> qemu_chr_fe_get_driver() is unsafe, frontends with hotswap support
> should not access CharDriver ptr directly as CharDriver might change.
>

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>

I would move this patch up in the series, otherwise looks good


> ---
>  chardev/char.c        |  7 +++++++
>  include/sysemu/char.h | 10 ++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 0483f19..36d6f36 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -484,9 +484,16 @@ static Notifier muxes_realize_notify = {
>
>  Chardev *qemu_chr_fe_get_driver(CharBackend *be)
>  {
> +    /* this is unsafe for the users that support chardev hotswap */
> +    assert(be->chr_be_change == NULL);
>      return be->chr;
>  }
>
> +bool qemu_chr_fe_backend_connected(CharBackend *be)
> +{
> +    return !!be->chr;
> +}
> +
>  static bool fe_connect(CharBackend *b, Chardev *s, Error **errp)
>  {
>      int tag = 0;
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 92ae57e..fa21535 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -404,10 +404,20 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s,
> Error **errp);
>   *
>   * Returns the driver associated with a CharBackend or NULL if no
>   * associated Chardev.
> + * Note: avoid this function as the driver should never be accessed
> directly,
> + *       especially by the frontends that support chardevice hotswap.
> + *       Consider qemu_chr_fe_backend_connected() to check for driver
> existence
>   */
>  Chardev *qemu_chr_fe_get_driver(CharBackend *be);
>
>  /**
> + * @qemu_chr_fe_backend_connected:
> + *
> + * Returns true if there is a chardevice associated with @be.
> + */
> +bool qemu_chr_fe_backend_connected(CharBackend *be);
> +
> +/**
>   * @qemu_chr_fe_deinit:
>   *
>   * Dissociate the CharBackend from the Chardev.
> --
> 2.7.4
>
>
> --
Marc-André Lureau

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

* Re: [Qemu-devel] [PATCH v2 2/9] char: add backend hotswap handler
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 2/9] char: add backend hotswap handler Anton Nefedov
@ 2017-05-25 14:32   ` Marc-André Lureau
  0 siblings, 0 replies; 17+ messages in thread
From: Marc-André Lureau @ 2017-05-25 14:32 UTC (permalink / raw)
  To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den

On Fri, May 19, 2017 at 4:51 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:

> Frontends should have an interface to setup the handler of a backend
> change.
> The interface will be used in the next commits
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



> ---
>  backends/rng-egd.c          |  2 +-
>  chardev/char-mux.c          |  1 +
>  chardev/char.c              |  4 +++-
>  gdbstub.c                   |  2 +-
>  hw/arm/pxa2xx.c             |  3 ++-
>  hw/arm/strongarm.c          |  2 +-
>  hw/char/bcm2835_aux.c       |  2 +-
>  hw/char/cadence_uart.c      |  2 +-
>  hw/char/debugcon.c          |  2 +-
>  hw/char/digic-uart.c        |  2 +-
>  hw/char/escc.c              |  2 +-
>  hw/char/etraxfs_ser.c       |  2 +-
>  hw/char/exynos4210_uart.c   |  2 +-
>  hw/char/grlib_apbuart.c     |  2 +-
>  hw/char/imx_serial.c        |  2 +-
>  hw/char/ipoctal232.c        |  2 +-
>  hw/char/lm32_juart.c        |  2 +-
>  hw/char/lm32_uart.c         |  2 +-
>  hw/char/mcf_uart.c          |  2 +-
>  hw/char/milkymist-uart.c    |  2 +-
>  hw/char/pl011.c             |  2 +-
>  hw/char/sclpconsole-lm.c    |  2 +-
>  hw/char/sclpconsole.c       |  2 +-
>  hw/char/serial.c            |  2 +-
>  hw/char/sh_serial.c         |  2 +-
>  hw/char/spapr_vty.c         |  2 +-
>  hw/char/stm32f2xx_usart.c   |  3 ++-
>  hw/char/terminal3270.c      |  2 +-
>  hw/char/virtio-console.c    |  4 ++--
>  hw/char/xen_console.c       |  2 +-
>  hw/char/xilinx_uartlite.c   |  2 +-
>  hw/ipmi/ipmi_bmc_extern.c   |  2 +-
>  hw/mips/boston.c            |  2 +-
>  hw/mips/mips_malta.c        |  2 +-
>  hw/misc/ivshmem.c           |  2 +-
>  hw/usb/ccid-card-passthru.c |  2 +-
>  hw/usb/dev-serial.c         |  2 +-
>  hw/usb/redirect.c           |  2 +-
>  include/sysemu/char.h       |  5 +++++
>  monitor.c                   |  4 ++--
>  net/colo-compare.c          | 14 ++++++++------
>  net/filter-mirror.c         |  6 +++---
>  net/slirp.c                 |  2 +-
>  net/vhost-user.c            |  7 ++++---
>  qtest.c                     |  2 +-
>  tests/test-char.c           | 14 ++++++++++----
>  tests/vhost-user-test.c     |  2 +-
>  47 files changed, 78 insertions(+), 59 deletions(-)
>
> diff --git a/backends/rng-egd.c b/backends/rng-egd.c
> index 380b19a..0b0e945 100644
> --- a/backends/rng-egd.c
> +++ b/backends/rng-egd.c
> @@ -106,7 +106,7 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
>
>      /* FIXME we should resubmit pending requests when the CDS reconnects.
> */
>      qemu_chr_fe_set_handlers(&s->chr, rng_egd_chr_can_read,
> -                             rng_egd_chr_read, NULL, s, NULL, true);
> +                             rng_egd_chr_read, NULL, NULL, s, NULL, true);
>  }
>
>  static void rng_egd_set_chardev(Object *obj, const char *value, Error
> **errp)
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index 37d42c6..5849ea5 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -278,6 +278,7 @@ void mux_chr_set_handlers(Chardev *chr, GMainContext
> *context)
>                               mux_chr_can_read,
>                               mux_chr_read,
>                               mux_chr_event,
> +                             NULL,
>                               chr,
>                               context, true);
>  }
> diff --git a/chardev/char.c b/chardev/char.c
> index 684cccd..ae60950 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -522,7 +522,7 @@ void qemu_chr_fe_deinit(CharBackend *b)
>      assert(b);
>
>      if (b->chr) {
> -        qemu_chr_fe_set_handlers(b, NULL, NULL, NULL, NULL, NULL, true);
> +        qemu_chr_fe_set_handlers(b, NULL, NULL, NULL, NULL, NULL, NULL,
> true);
>          if (b->chr->be == b) {
>              b->chr->be = NULL;
>          }
> @@ -538,6 +538,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>                                IOCanReadHandler *fd_can_read,
>                                IOReadHandler *fd_read,
>                                IOEventHandler *fd_event,
> +                              BackendChangeHandler *be_change,
>                                void *opaque,
>                                GMainContext *context,
>                                bool set_open)
> @@ -561,6 +562,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>      b->chr_can_read = fd_can_read;
>      b->chr_read = fd_read;
>      b->chr_event = fd_event;
> +    b->chr_be_change = be_change;
>      b->opaque = opaque;
>      if (cc->chr_update_read_handler) {
>          cc->chr_update_read_handler(s, context);
> diff --git a/gdbstub.c b/gdbstub.c
> index 86eed4f..1ac0489 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2013,7 +2013,7 @@ int gdbserver_start(const char *device)
>      if (chr) {
>          qemu_chr_fe_init(&s->chr, chr, &error_abort);
>          qemu_chr_fe_set_handlers(&s->chr, gdb_chr_can_receive,
> gdb_chr_receive,
> -                                 gdb_chr_event, NULL, NULL, true);
> +                                 gdb_chr_event, NULL, NULL, NULL, true);
>      }
>      s->state = chr ? RS_IDLE : RS_INACTIVE;
>      s->mon_chr = mon_chr;
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index eea551d..3e51882 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -1970,7 +1970,8 @@ static void pxa2xx_fir_realize(DeviceState *dev,
> Error **errp)
>      PXA2xxFIrState *s = PXA2XX_FIR(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, pxa2xx_fir_is_empty,
> -                             pxa2xx_fir_rx, pxa2xx_fir_event, s, NULL,
> true);
> +                             pxa2xx_fir_rx, pxa2xx_fir_event, NULL, s,
> NULL,
> +                             true);
>  }
>
>  static bool pxa2xx_fir_vmstate_validate(void *opaque, int version_id)
> diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
> index 3311cc3..bec093d 100644
> --- a/hw/arm/strongarm.c
> +++ b/hw/arm/strongarm.c
> @@ -1246,7 +1246,7 @@ static void strongarm_uart_realize(DeviceState *dev,
> Error **errp)
>                               strongarm_uart_can_receive,
>                               strongarm_uart_receive,
>                               strongarm_uart_event,
> -                             s, NULL, true);
> +                             NULL, s, NULL, true);
>  }
>
>  static void strongarm_uart_reset(DeviceState *dev)
> diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c
> index 4d46ad6..370dc7e 100644
> --- a/hw/char/bcm2835_aux.c
> +++ b/hw/char/bcm2835_aux.c
> @@ -279,7 +279,7 @@ static void bcm2835_aux_realize(DeviceState *dev,
> Error **errp)
>      BCM2835AuxState *s = BCM2835_AUX(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, bcm2835_aux_can_receive,
> -                             bcm2835_aux_receive, NULL, s, NULL, true);
> +                             bcm2835_aux_receive, NULL, NULL, s, NULL,
> true);
>  }
>
>  static Property bcm2835_aux_props[] = {
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 4dcee57..71867b3 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -484,7 +484,7 @@ static void cadence_uart_realize(DeviceState *dev,
> Error **errp)
>                                            fifo_trigger_update, s);
>
>      qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
> -                             uart_event, s, NULL, true);
> +                             uart_event, NULL, s, NULL, true);
>  }
>
>  static void cadence_uart_init(Object *obj)
> diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
> index 80dce07..6d95297 100644
> --- a/hw/char/debugcon.c
> +++ b/hw/char/debugcon.c
> @@ -92,7 +92,7 @@ static void debugcon_realize_core(DebugconState *s,
> Error **errp)
>          return;
>      }
>
> -    qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL, s, NULL, true);
> +    qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL, NULL, s, NULL,
> true);
>  }
>
>  static void debugcon_isa_realizefn(DeviceState *dev, Error **errp)
> diff --git a/hw/char/digic-uart.c b/hw/char/digic-uart.c
> index 029f5bb..2d373dc 100644
> --- a/hw/char/digic-uart.c
> +++ b/hw/char/digic-uart.c
> @@ -146,7 +146,7 @@ static void digic_uart_realize(DeviceState *dev, Error
> **errp)
>      DigicUartState *s = DIGIC_UART(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx,
> -                             uart_event, s, NULL, true);
> +                             uart_event, NULL, s, NULL, true);
>  }
>
>  static void digic_uart_init(Object *obj)
> diff --git a/hw/char/escc.c b/hw/char/escc.c
> index 9228091..aa882b6 100644
> --- a/hw/char/escc.c
> +++ b/hw/char/escc.c
> @@ -1015,7 +1015,7 @@ static void escc_realize(DeviceState *dev, Error
> **errp)
>          if (qemu_chr_fe_get_driver(&s->chn[i].chr)) {
>              s->chn[i].clock = s->frequency / 2;
>              qemu_chr_fe_set_handlers(&s->chn[i].chr, serial_can_receive,
> -                                     serial_receive1, serial_event,
> +                                     serial_receive1, serial_event, NULL,
>                                       &s->chn[i], NULL, true);
>          }
>      }
> diff --git a/hw/char/etraxfs_ser.c b/hw/char/etraxfs_ser.c
> index 5438387..4abd382 100644
> --- a/hw/char/etraxfs_ser.c
> +++ b/hw/char/etraxfs_ser.c
> @@ -233,7 +233,7 @@ static void etraxfs_ser_realize(DeviceState *dev,
> Error **errp)
>
>      qemu_chr_fe_set_handlers(&s->chr,
>                               serial_can_receive, serial_receive,
> -                             serial_event, s, NULL, true);
> +                             serial_event, NULL, s, NULL, true);
>  }
>
>  static void etraxfs_ser_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index bff706a..7ef4ea5 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -644,7 +644,7 @@ static void exynos4210_uart_realize(DeviceState *dev,
> Error **errp)
>
>      qemu_chr_fe_set_handlers(&s->chr, exynos4210_uart_can_receive,
>                               exynos4210_uart_receive,
> exynos4210_uart_event,
> -                             s, NULL, true);
> +                             NULL, s, NULL, true);
>  }
>
>  static Property exynos4210_uart_properties[] = {
> diff --git a/hw/char/grlib_apbuart.c b/hw/char/grlib_apbuart.c
> index db686e6..610a317 100644
> --- a/hw/char/grlib_apbuart.c
> +++ b/hw/char/grlib_apbuart.c
> @@ -247,7 +247,7 @@ static int grlib_apbuart_init(SysBusDevice *dev)
>                               grlib_apbuart_can_receive,
>                               grlib_apbuart_receive,
>                               grlib_apbuart_event,
> -                             uart, NULL, true);
> +                             NULL, uart, NULL, true);
>
>      sysbus_init_irq(dev, &uart->irq);
>
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index 52e67f8..b66396f 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -316,7 +316,7 @@ static void imx_serial_realize(DeviceState *dev, Error
> **errp)
>      DPRINTF("char dev for uart: %p\n", qemu_chr_fe_get_driver(&s->chr));
>
>      qemu_chr_fe_set_handlers(&s->chr, imx_can_receive, imx_receive,
> -                             imx_event, s, NULL, true);
> +                             imx_event, NULL, s, NULL, true);
>  }
>
>  static void imx_serial_init(Object *obj)
> diff --git a/hw/char/ipoctal232.c b/hw/char/ipoctal232.c
> index 93929c2..734e42c 100644
> --- a/hw/char/ipoctal232.c
> +++ b/hw/char/ipoctal232.c
> @@ -545,7 +545,7 @@ static void ipoctal_realize(DeviceState *dev, Error
> **errp)
>          if (qemu_chr_fe_get_driver(&ch->dev)) {
>              qemu_chr_fe_set_handlers(&ch->dev, hostdev_can_receive,
>                                       hostdev_receive, hostdev_event,
> -                                     ch, NULL, true);
> +                                     NULL, ch, NULL, true);
>              DPRINTF("Redirecting channel %u to %s\n", i, ch->dev->label);
>          } else {
>              DPRINTF("Could not redirect channel %u, no chardev set\n", i);
> diff --git a/hw/char/lm32_juart.c b/hw/char/lm32_juart.c
> index f8c1e0d..b3a5351 100644
> --- a/hw/char/lm32_juart.c
> +++ b/hw/char/lm32_juart.c
> @@ -119,7 +119,7 @@ static void lm32_juart_realize(DeviceState *dev, Error
> **errp)
>      LM32JuartState *s = LM32_JUART(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, juart_can_rx, juart_rx,
> -                             juart_event, s, NULL, true);
> +                             juart_event, NULL, s, NULL, true);
>  }
>
>  static const VMStateDescription vmstate_lm32_juart = {
> diff --git a/hw/char/lm32_uart.c b/hw/char/lm32_uart.c
> index 7f3597c..3a6dbf8 100644
> --- a/hw/char/lm32_uart.c
> +++ b/hw/char/lm32_uart.c
> @@ -266,7 +266,7 @@ static void lm32_uart_realize(DeviceState *dev, Error
> **errp)
>      LM32UartState *s = LM32_UART(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx,
> -                             uart_event, s, NULL, true);
> +                             uart_event, NULL, s, NULL, true);
>  }
>
>  static const VMStateDescription vmstate_lm32_uart = {
> diff --git a/hw/char/mcf_uart.c b/hw/char/mcf_uart.c
> index e69672f..7f3cd5a 100644
> --- a/hw/char/mcf_uart.c
> +++ b/hw/char/mcf_uart.c
> @@ -305,7 +305,7 @@ static void mcf_uart_realize(DeviceState *dev, Error
> **errp)
>      mcf_uart_state *s = MCF_UART(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, mcf_uart_can_receive,
> mcf_uart_receive,
> -                             mcf_uart_event, s, NULL, true);
> +                             mcf_uart_event, NULL, s, NULL, true);
>  }
>
>  static Property mcf_uart_properties[] = {
> diff --git a/hw/char/milkymist-uart.c b/hw/char/milkymist-uart.c
> index ae8e2f3..523959a 100644
> --- a/hw/char/milkymist-uart.c
> +++ b/hw/char/milkymist-uart.c
> @@ -199,7 +199,7 @@ static void milkymist_uart_realize(DeviceState *dev,
> Error **errp)
>      MilkymistUartState *s = MILKYMIST_UART(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx,
> -                             uart_event, s, NULL, true);
> +                             uart_event, NULL, s, NULL, true);
>  }
>
>  static void milkymist_uart_init(Object *obj)
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 24ea973..c38f60a 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -329,7 +329,7 @@ static void pl011_realize(DeviceState *dev, Error
> **errp)
>      PL011State *s = PL011(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
> -                             pl011_event, s, NULL, true);
> +                             pl011_event, NULL, s, NULL, true);
>  }
>
>  static void pl011_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> index 07d6ebd..ed1e2c5 100644
> --- a/hw/char/sclpconsole-lm.c
> +++ b/hw/char/sclpconsole-lm.c
> @@ -313,7 +313,7 @@ static int console_init(SCLPEvent *event)
>      console_available = true;
>
>      qemu_chr_fe_set_handlers(&scon->chr, chr_can_read,
> -                             chr_read, NULL, scon, NULL, true);
> +                             chr_read, NULL, NULL, scon, NULL, true);
>
>      return 0;
>  }
> diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
> index b78f240..9a65010 100644
> --- a/hw/char/sclpconsole.c
> +++ b/hw/char/sclpconsole.c
> @@ -228,7 +228,7 @@ static int console_init(SCLPEvent *event)
>      }
>      console_available = true;
>      qemu_chr_fe_set_handlers(&scon->chr, chr_can_read,
> -                             chr_read, NULL, scon, NULL, true);
> +                             chr_read, NULL, NULL, scon, NULL, true);
>
>      return 0;
>  }
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 03d890c..d8d34d0 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -897,7 +897,7 @@ void serial_realize_core(SerialState *s, Error **errp)
>      qemu_register_reset(serial_reset, s);
>
>      qemu_chr_fe_set_handlers(&s->chr, serial_can_receive1,
> serial_receive1,
> -                             serial_event, s, NULL, true);
> +                             serial_event, NULL, s, NULL, true);
>      fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
>      fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
>      serial_reset(s);
> diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
> index 303eb0a..c352337 100644
> --- a/hw/char/sh_serial.c
> +++ b/hw/char/sh_serial.c
> @@ -400,7 +400,7 @@ void sh_serial_init(MemoryRegion *sysmem,
>          qemu_chr_fe_init(&s->chr, chr, &error_abort);
>          qemu_chr_fe_set_handlers(&s->chr, sh_serial_can_receive1,
>                                   sh_serial_receive1,
> -                                 sh_serial_event, s, NULL, true);
> +                                 sh_serial_event, NULL, s, NULL, true);
>      }
>
>      s->eri = eri_source;
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index e30c8da..9cdc0e0 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -84,7 +84,7 @@ static void spapr_vty_realize(VIOsPAPRDevice *sdev,
> Error **errp)
>      }
>
>      qemu_chr_fe_set_handlers(&dev->chardev, vty_can_receive,
> -                             vty_receive, NULL, dev, NULL, true);
> +                             vty_receive, NULL, NULL, dev, NULL, true);
>  }
>
>  /* Forward declaration */
> diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
> index 59872e6..268e435 100644
> --- a/hw/char/stm32f2xx_usart.c
> +++ b/hw/char/stm32f2xx_usart.c
> @@ -207,7 +207,8 @@ static void stm32f2xx_usart_realize(DeviceState *dev,
> Error **errp)
>      STM32F2XXUsartState *s = STM32F2XX_USART(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, stm32f2xx_usart_can_receive,
> -                             stm32f2xx_usart_receive, NULL, s, NULL,
> true);
> +                             stm32f2xx_usart_receive, NULL, NULL,
> +                             s, NULL, true);
>  }
>
>  static void stm32f2xx_usart_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> index b2dda01..943a0f3 100644
> --- a/hw/char/terminal3270.c
> +++ b/hw/char/terminal3270.c
> @@ -179,7 +179,7 @@ static void terminal_init(EmulatedCcw3270Device *dev,
> Error **errp)
>      }
>      terminal_available = true;
>      qemu_chr_fe_set_handlers(&t->chr, terminal_can_read,
> -                             terminal_read, chr_event, t, NULL, true);
> +                             terminal_read, chr_event, NULL, t, NULL,
> true);
>  }
>
>  static int read_payload_3270(EmulatedCcw3270Device *dev, uint32_t cda,
> diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
> index 798d9b6..cf7331d 100644
> --- a/hw/char/virtio-console.c
> +++ b/hw/char/virtio-console.c
> @@ -188,11 +188,11 @@ static void virtconsole_realize(DeviceState *dev,
> Error **errp)
>           */
>          if (k->is_console) {
>              qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
> -                                     NULL, vcon, NULL, true);
> +                                     NULL, NULL, vcon, NULL, true);
>              virtio_serial_open(port);
>          } else {
>              qemu_chr_fe_set_handlers(&vcon->chr, chr_can_read, chr_read,
> -                                     chr_event, vcon, NULL, false);
> +                                     chr_event, NULL, vcon, NULL, false);
>          }
>      }
>  }
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index c01f410..cb7975f 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -246,7 +246,7 @@ static int con_initialise(struct XenDevice *xendev)
>
>      xen_be_bind_evtchn(&con->xendev);
>      qemu_chr_fe_set_handlers(&con->chr, xencons_can_receive,
> -                             xencons_receive, NULL, con, NULL, true);
> +                             xencons_receive, NULL, NULL, con, NULL,
> true);
>
>      xen_pv_printf(xendev, 1,
>                    "ring mfn %d, remote port %d, local port %d, limit
> %zd\n",
> diff --git a/hw/char/xilinx_uartlite.c b/hw/char/xilinx_uartlite.c
> index 37d313b..2568302 100644
> --- a/hw/char/xilinx_uartlite.c
> +++ b/hw/char/xilinx_uartlite.c
> @@ -212,7 +212,7 @@ static void xilinx_uartlite_realize(DeviceState *dev,
> Error **errp)
>      XilinxUARTLite *s = XILINX_UARTLITE(dev);
>
>      qemu_chr_fe_set_handlers(&s->chr, uart_can_rx, uart_rx,
> -                             uart_event, s, NULL, true);
> +                             uart_event, NULL, s, NULL, true);
>  }
>
>  static void xilinx_uartlite_init(Object *obj)
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index e8e3d25..6f2339d 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -453,7 +453,7 @@ static void ipmi_bmc_extern_realize(DeviceState *dev,
> Error **errp)
>      }
>
>      qemu_chr_fe_set_handlers(&ibe->chr, can_receive, receive,
> -                             chr_event, ibe, NULL, true);
> +                             chr_event, NULL, ibe, NULL, true);
>  }
>
>  static int ipmi_bmc_extern_post_migrate(void *opaque, int version_id)
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index 83f7b82..a57c860 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -533,7 +533,7 @@ static void boston_mach_init(MachineState *machine)
>      chr = qemu_chr_new("lcd", "vc:320x240");
>      qemu_chr_fe_init(&s->lcd_display, chr, NULL);
>      qemu_chr_fe_set_handlers(&s->lcd_display, NULL, NULL,
> -                             boston_lcd_event, s, NULL, true);
> +                             boston_lcd_event, NULL, s, NULL, true);
>
>      ahci =
> pci_create_simple_multifunction(&PCI_BRIDGE(&pcie2->root)->sec_bus,
>                                             PCI_DEVFN(0, 0),
> diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
> index 5dd177e..96dce76 100644
> --- a/hw/mips/mips_malta.c
> +++ b/hw/mips/mips_malta.c
> @@ -571,7 +571,7 @@ static MaltaFPGAState *malta_fpga_init(MemoryRegion
> *address_space,
>      chr = qemu_chr_new("fpga", "vc:320x200");
>      qemu_chr_fe_init(&s->display, chr, NULL);
>      qemu_chr_fe_set_handlers(&s->display, NULL, NULL,
> -                             malta_fgpa_display_event, s, NULL, true);
> +                             malta_fgpa_display_event, NULL, s, NULL,
> true);
>
>      s->uart = serial_mm_init(address_space, base + 0x900, 3, uart_irq,
>                               230400, uart_chr, DEVICE_NATIVE_ENDIAN);
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 475e36a..e2dece8 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -896,7 +896,7 @@ static void ivshmem_common_realize(PCIDevice *dev,
> Error **errp)
>          }
>
>          qemu_chr_fe_set_handlers(&s->server_chr, ivshmem_can_receive,
> -                                 ivshmem_read, NULL, s, NULL, true);
> +                                 ivshmem_read, NULL, NULL, s, NULL, true);
>
>          if (ivshmem_setup_interrupts(s, errp) < 0) {
>              error_prepend(errp, "Failed to initialize interrupts: ");
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index a41b0d6..9ace5ac 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -351,7 +351,7 @@ static int passthru_initfn(CCIDCardState *base)
>          qemu_chr_fe_set_handlers(&card->cs,
>              ccid_card_vscard_can_read,
>              ccid_card_vscard_read,
> -            ccid_card_vscard_event, card, NULL, true);
> +            ccid_card_vscard_event, NULL, card, NULL, true);
>          ccid_card_vscard_send_init(card);
>      } else {
>          error_report("missing chardev");
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 83a4f0e..e6b2c7c 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -501,7 +501,7 @@ static void usb_serial_realize(USBDevice *dev, Error
> **errp)
>      }
>
>      qemu_chr_fe_set_handlers(&s->cs, usb_serial_can_read, usb_serial_read,
> -                             usb_serial_event, s, NULL, true);
> +                             usb_serial_event, NULL, s, NULL, true);
>      usb_serial_handle_reset(dev);
>
>      if (chr->be_open && !dev->attached) {
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index ad5ef78..1e9bf69 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1399,7 +1399,7 @@ static void usbredir_realize(USBDevice *udev, Error
> **errp)
>      /* Let the backend know we are ready */
>      qemu_chr_fe_set_handlers(&dev->cs, usbredir_chardev_can_read,
>                               usbredir_chardev_read,
> usbredir_chardev_event,
> -                             dev, NULL, true);
> +                             NULL, dev, NULL, true);
>
>      dev->vmstate =
>          qemu_add_vm_change_state_handler(usbredir_vm_state_change, dev);
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index fffc0f4..9f8df07 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -64,6 +64,7 @@ struct ParallelIOArg {
>  #define CHR_TIOCM_RTS  0x004
>
>  typedef void IOEventHandler(void *opaque, int event);
> +typedef int BackendChangeHandler(void *opaque);
>
>  typedef enum {
>      /* Whether the chardev peer is able to close and
> @@ -87,6 +88,7 @@ typedef struct CharBackend {
>      IOEventHandler *chr_event;
>      IOCanReadHandler *chr_can_read;
>      IOReadHandler *chr_read;
> +    BackendChangeHandler *chr_be_change;
>      void *opaque;
>      int tag;
>      int fe_open;
> @@ -399,6 +401,8 @@ void qemu_chr_fe_deinit(CharBackend *b);
>   *               receive
>   * @fd_read: callback to receive data from char
>   * @fd_event: event callback
> + * @be_change: backend change callback; passing NULL means hot backend
> change
> + *             is not supported and will not be attempted
>   * @opaque: an opaque pointer for the callbacks
>   * @context: a main loop context or NULL for the default
>   * @set_open: whether to call qemu_chr_fe_set_open() implicitely when
> @@ -413,6 +417,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>                                IOCanReadHandler *fd_can_read,
>                                IOReadHandler *fd_read,
>                                IOEventHandler *fd_event,
> +                              BackendChangeHandler *be_change,
>                                void *opaque,
>                                GMainContext *context,
>                                bool set_open);
> diff --git a/monitor.c b/monitor.c
> index afbacfe..9057ad3 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4088,12 +4088,12 @@ void monitor_init(Chardev *chr, int flags)
>
>      if (monitor_is_qmp(mon)) {
>          qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
> monitor_qmp_read,
> -                                 monitor_qmp_event, mon, NULL, true);
> +                                 monitor_qmp_event, NULL, mon, NULL,
> true);
>          qemu_chr_fe_set_echo(&mon->chr, true);
>          json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
>      } else {
>          qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
> monitor_read,
> -                                 monitor_event, mon, NULL, true);
> +                                 monitor_event, NULL, mon, NULL, true);
>      }
>
>      qemu_mutex_lock(&monitor_lock);
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 4ab80b1..f672f4f 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -532,7 +532,7 @@ static void compare_pri_chr_in(void *opaque, const
> uint8_t *buf, int size)
>
>      ret = net_fill_rstate(&s->pri_rs, buf, size);
>      if (ret == -1) {
> -        qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL,
> +        qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL, NULL,
>                                   NULL, NULL, true);
>          error_report("colo-compare primary_in error");
>      }
> @@ -549,7 +549,7 @@ static void compare_sec_chr_in(void *opaque, const
> uint8_t *buf, int size)
>
>      ret = net_fill_rstate(&s->sec_rs, buf, size);
>      if (ret == -1) {
> -        qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL,
> +        qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL, NULL,
>                                   NULL, NULL, true);
>          error_report("colo-compare secondary_in error");
>      }
> @@ -577,9 +577,11 @@ static void *colo_compare_thread(void *opaque)
>      s->worker_context = g_main_context_new();
>
>      qemu_chr_fe_set_handlers(&s->chr_pri_in, compare_chr_can_read,
> -                          compare_pri_chr_in, NULL, s, s->worker_context,
> true);
> +                             compare_pri_chr_in, NULL, NULL,
> +                             s, s->worker_context, true);
>      qemu_chr_fe_set_handlers(&s->chr_sec_in, compare_chr_can_read,
> -                          compare_sec_chr_in, NULL, s, s->worker_context,
> true);
> +                             compare_sec_chr_in, NULL, NULL,
> +                             s, s->worker_context, true);
>
>      s->compare_loop = g_main_loop_new(s->worker_context, FALSE);
>
> @@ -790,9 +792,9 @@ static void colo_compare_finalize(Object *obj)
>  {
>      CompareState *s = COLO_COMPARE(obj);
>
> -    qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL, NULL,
> +    qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL, NULL, NULL,
>                               s->worker_context, true);
> -    qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL, NULL,
> +    qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL, NULL, NULL,
>                               s->worker_context, true);
>      qemu_chr_fe_deinit(&s->chr_out);
>
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> index 72fa7c2..06321d7 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -112,7 +112,7 @@ static void redirector_chr_read(void *opaque, const
> uint8_t *buf, int size)
>
>      if (ret == -1) {
>          qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
> -                                 NULL, NULL, true);
> +                                 NULL, NULL, NULL, true);
>      }
>  }
>
> @@ -124,7 +124,7 @@ static void redirector_chr_event(void *opaque, int
> event)
>      switch (event) {
>      case CHR_EVENT_CLOSED:
>          qemu_chr_fe_set_handlers(&s->chr_in, NULL, NULL, NULL,
> -                                 NULL, NULL, true);
> +                                 NULL, NULL, NULL, true);
>          break;
>      default:
>          break;
> @@ -251,7 +251,7 @@ static void filter_redirector_setup(NetFilterState
> *nf, Error **errp)
>
>          qemu_chr_fe_set_handlers(&s->chr_in, redirector_chr_can_read,
>                                   redirector_chr_read,
> redirector_chr_event,
> -                                 nf, NULL, true);
> +                                 NULL, nf, NULL, true);
>      }
>
>      if (s->outdev) {
> diff --git a/net/slirp.c b/net/slirp.c
> index c705a60..6cbae5a 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -778,7 +778,7 @@ static int slirp_guestfwd(SlirpState *s, const char
> *config_str,
>          fwd->slirp = s->slirp;
>
>          qemu_chr_fe_set_handlers(&fwd->hd, guestfwd_can_read,
> guestfwd_read,
> -                                 NULL, fwd, NULL, true);
> +                                 NULL, NULL, fwd, NULL, true);
>      }
>      return 0;
>
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 00a0c1c..d090311 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -214,7 +214,7 @@ static void chr_closed_bh(void *opaque)
>      vhost_user_stop(queues, ncs);
>
>      qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, net_vhost_user_event,
> -                             opaque, NULL, true);
> +                             NULL, opaque, NULL, true);
>
>      if (err) {
>          error_report_err(err);
> @@ -260,7 +260,7 @@ static void net_vhost_user_event(void *opaque, int
> event)
>
>              g_source_remove(s->watch);
>              s->watch = 0;
> -            qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL,
> +            qemu_chr_fe_set_handlers(&s->chr, NULL, NULL, NULL, NULL,
>                                       NULL, NULL, false);
>
>              aio_bh_schedule_oneshot(ctx, chr_closed_bh, opaque);
> @@ -308,7 +308,8 @@ static int net_vhost_user_init(NetClientState *peer,
> const char *device,
>              return -1;
>          }
>          qemu_chr_fe_set_handlers(&s->chr, NULL, NULL,
> -                                 net_vhost_user_event, nc0->name, NULL,
> true);
> +                                 net_vhost_user_event, NULL, nc0->name,
> NULL,
> +                                 true);
>      } while (!s->started);
>
>      assert(s->vhost_net);
> diff --git a/qtest.c b/qtest.c
> index 5aa6636..b6e9780 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -691,7 +691,7 @@ void qtest_init(const char *qtest_chrdev, const char
> *qtest_log, Error **errp)
>
>      qemu_chr_fe_init(&qtest_chr, chr, errp);
>      qemu_chr_fe_set_handlers(&qtest_chr, qtest_can_read, qtest_read,
> -                             qtest_event, &qtest_chr, NULL, true);
> +                             qtest_event, NULL, &qtest_chr, NULL, true);
>      qemu_chr_fe_set_echo(&qtest_chr, true);
>
>      inbuf = g_string_new("");
> diff --git a/tests/test-char.c b/tests/test-char.c
> index 124d0c5..f3b377f 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -182,6 +182,7 @@ static void char_mux_test(void)
>                               fe_can_read,
>                               fe_read,
>                               fe_event,
> +                             NULL,
>                               &h1,
>                               NULL, true);
>
> @@ -190,6 +191,7 @@ static void char_mux_test(void)
>                               fe_can_read,
>                               fe_read,
>                               fe_event,
> +                             NULL,
>                               &h2,
>                               NULL, true);
>      qemu_chr_fe_take_focus(&chr_be2);
> @@ -213,7 +215,8 @@ static void char_mux_test(void)
>      h1.read_count = 0;
>
>      /* remove first handler */
> -    qemu_chr_fe_set_handlers(&chr_be1, NULL, NULL, NULL, NULL, NULL,
> true);
> +    qemu_chr_fe_set_handlers(&chr_be1, NULL, NULL, NULL, NULL,
> +                             NULL, NULL, true);
>      qemu_chr_be_write(base, (void *)"hello", 6);
>      g_assert_cmpint(h1.read_count, ==, 0);
>      g_assert_cmpint(h2.read_count, ==, 0);
> @@ -312,13 +315,13 @@ static void char_socket_test(void)
>
>      qemu_chr_fe_init(&be, chr, &error_abort);
>      qemu_chr_fe_set_handlers(&be, socket_can_read, socket_read,
> -                             NULL, &d, NULL, true);
> +                             NULL, NULL, &d, NULL, true);
>
>      chr_client = qemu_chr_new("client", tmp);
>      qemu_chr_fe_init(&client_be, chr_client, &error_abort);
>      qemu_chr_fe_set_handlers(&client_be, socket_can_read_hello,
>                               socket_read_hello,
> -                             NULL, &d, NULL, true);
> +                             NULL, NULL, &d, NULL, true);
>      g_free(tmp);
>
>      d.conn_expected = true;
> @@ -388,6 +391,7 @@ static void char_pipe_test(void)
>                               fe_can_read,
>                               fe_read,
>                               fe_event,
> +                             NULL,
>                               &fe,
>                               NULL, true);
>
> @@ -437,7 +441,7 @@ static void char_udp_test(void)
>      d.chr = chr;
>      qemu_chr_fe_init(&be, chr, &error_abort);
>      qemu_chr_fe_set_handlers(&be, socket_can_read_hello,
> socket_read_hello,
> -                             NULL, &d, NULL, true);
> +                             NULL, NULL, &d, NULL, true);
>      ret = qemu_chr_write_all(chr, (uint8_t *)"hello", 5);
>      g_assert_cmpint(ret, ==, 5);
>
> @@ -503,6 +507,7 @@ static void char_file_test(void)
>                                   fe_can_read,
>                                   fe_read,
>                                   fe_event,
> +                                 NULL,
>                                   &fe, NULL, true);
>
>          main_loop();
> @@ -558,6 +563,7 @@ static void char_null_test(void)
>                               fe_can_read,
>                               fe_read,
>                               fe_event,
> +                             NULL,
>                               NULL, NULL, true);
>
>      ret = qemu_chr_fe_write(&be, (void *)"buf", 4);
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index 9095af2..718c08a 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -464,7 +464,7 @@ static void test_server_create_chr(TestServer *server,
> const gchar *opt)
>
>      qemu_chr_fe_init(&server->chr, chr, &error_abort);
>      qemu_chr_fe_set_handlers(&server->chr, chr_can_read, chr_read,
> -                             chr_event, server, NULL, true);
> +                             chr_event, NULL, server, NULL, true);
>  }
>
>  static void test_server_listen(TestServer *server)
> --
> 2.7.4
>
>
> --
Marc-André Lureau

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

* Re: [Qemu-devel] [PATCH v2 9/9] char: avoid chardevice direct access
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 9/9] char: avoid chardevice direct access Anton Nefedov
@ 2017-05-25 14:32   ` Marc-André Lureau
  0 siblings, 0 replies; 17+ messages in thread
From: Marc-André Lureau @ 2017-05-25 14:32 UTC (permalink / raw)
  To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den

On Fri, May 19, 2017 at 4:53 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:

> frontends should avoid accessing CharDriver struct where possible
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>

I would move this patch up in the series, otherwise looks good


> ---
>  chardev/char.c              | 5 +++++
>  gdbstub.c                   | 2 +-
>  hw/arm/strongarm.c          | 2 +-
>  hw/char/cadence_uart.c      | 2 +-
>  hw/char/debugcon.c          | 2 +-
>  hw/char/escc.c              | 6 +++---
>  hw/char/exynos4210_uart.c   | 2 +-
>  hw/char/grlib_apbuart.c     | 2 +-
>  hw/char/ipoctal232.c        | 2 +-
>  hw/char/parallel.c          | 2 +-
>  hw/char/sclpconsole-lm.c    | 2 +-
>  hw/char/sclpconsole.c       | 2 +-
>  hw/char/sh_serial.c         | 2 +-
>  hw/char/spapr_vty.c         | 2 +-
>  hw/char/terminal3270.c      | 2 +-
>  hw/char/xen_console.c       | 2 +-
>  hw/ipmi/ipmi_bmc_extern.c   | 2 +-
>  hw/misc/ivshmem.c           | 4 ++--
>  hw/usb/ccid-card-passthru.c | 4 ++--
>  hw/usb/dev-serial.c         | 5 ++---
>  hw/usb/redirect.c           | 5 ++---
>  include/sysemu/char.h       | 7 +++++++
>  net/filter-mirror.c         | 2 +-
>  23 files changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 36d6f36..e43d840 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -494,6 +494,11 @@ bool qemu_chr_fe_backend_connected(CharBackend *be)
>      return !!be->chr;
>  }
>
> +bool qemu_chr_fe_backend_open(CharBackend *be)
> +{
> +    return be->chr && be->chr->be_open;
> +}
> +
>  static bool fe_connect(CharBackend *b, Chardev *s, Error **errp)
>  {
>      int tag = 0;
> diff --git a/gdbstub.c b/gdbstub.c
> index 1ac0489..68cbe8a 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2001,7 +2001,7 @@ int gdbserver_start(const char *device)
>                                     NULL, &error_abort);
>          monitor_init(mon_chr, 0);
>      } else {
> -        if (qemu_chr_fe_get_driver(&s->chr)) {
> +        if (qemu_chr_fe_backend_connected(&s->chr)) {
>              object_unparent(OBJECT(qemu_chr_fe_get_driver(&s->chr)));
>          }
>          mon_chr = s->mon_chr;
> diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
> index bec093d..9d7cf21 100644
> --- a/hw/arm/strongarm.c
> +++ b/hw/arm/strongarm.c
> @@ -1105,7 +1105,7 @@ static void strongarm_uart_tx(void *opaque)
>
>      if (s->utcr3 & UTCR3_LBM) /* loopback */ {
>          strongarm_uart_receive(s, &s->tx_fifo[s->tx_start], 1);
> -    } else if (qemu_chr_fe_get_driver(&s->chr)) {
> +    } else if (qemu_chr_fe_backend_connected(&s->chr)) {
>          /* XXX this blocks entire thread. Rewrite to use
>           * qemu_chr_fe_write and background I/O callbacks */
>          qemu_chr_fe_write_all(&s->chr, &s->tx_fifo[s->tx_start], 1);
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index 71867b3..19636c0 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -278,7 +278,7 @@ static gboolean cadence_uart_xmit(GIOChannel *chan,
> GIOCondition cond,
>      int ret;
>
>      /* instant drain the fifo when there's no back-end */
> -    if (!qemu_chr_fe_get_driver(&s->chr)) {
> +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
>          s->tx_count = 0;
>          return FALSE;
>      }
> diff --git a/hw/char/debugcon.c b/hw/char/debugcon.c
> index 6d95297..bd0d4f0 100644
> --- a/hw/char/debugcon.c
> +++ b/hw/char/debugcon.c
> @@ -87,7 +87,7 @@ static const MemoryRegionOps debugcon_ops = {
>
>  static void debugcon_realize_core(DebugconState *s, Error **errp)
>  {
> -    if (!qemu_chr_fe_get_driver(&s->chr)) {
> +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
>          error_setg(errp, "Can't create debugcon device, empty char
> device");
>          return;
>      }
> diff --git a/hw/char/escc.c b/hw/char/escc.c
> index aa882b6..dbbeb4a 100644
> --- a/hw/char/escc.c
> +++ b/hw/char/escc.c
> @@ -416,7 +416,7 @@ static void escc_update_parameters(ChannelState *s)
>      int speed, parity, data_bits, stop_bits;
>      QEMUSerialSetParams ssp;
>
> -    if (!qemu_chr_fe_get_driver(&s->chr) || s->type != ser)
> +    if (!qemu_chr_fe_backend_connected(&s->chr) || s->type != ser)
>          return;
>
>      if (s->wregs[W_TXCTRL1] & TXCTRL1_PAREN) {
> @@ -556,7 +556,7 @@ static void escc_mem_write(void *opaque, hwaddr addr,
>          trace_escc_mem_writeb_data(CHN_C(s), val);
>          s->tx = val;
>          if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
> -            if (qemu_chr_fe_get_driver(&s->chr)) {
> +            if (qemu_chr_fe_backend_connected(&s->chr)) {
>                  /* XXX this blocks entire thread. Rewrite to use
>                   * qemu_chr_fe_write and background I/O callbacks */
>                  qemu_chr_fe_write_all(&s->chr, &s->tx, 1);
> @@ -1012,7 +1012,7 @@ static void escc_realize(DeviceState *dev, Error
> **errp)
>                            ESCC_SIZE << s->it_shift);
>
>      for (i = 0; i < 2; i++) {
> -        if (qemu_chr_fe_get_driver(&s->chn[i].chr)) {
> +        if (qemu_chr_fe_backend_connected(&s->chn[i].chr)) {
>              s->chn[i].clock = s->frequency / 2;
>              qemu_chr_fe_set_handlers(&s->chn[i].chr, serial_can_receive,
>                                       serial_receive1, serial_event, NULL,
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index 7ef4ea5..2b0576c 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -379,7 +379,7 @@ static void exynos4210_uart_write(void *opaque, hwaddr
> offset,
>          break;
>
>      case UTXH:
> -        if (qemu_chr_fe_get_driver(&s->chr)) {
> +        if (qemu_chr_fe_backend_connected(&s->chr)) {
>              s->reg[I_(UTRSTAT)] &= ~(UTRSTAT_TRANSMITTER_EMPTY |
>                      UTRSTAT_Tx_BUFFER_EMPTY);
>              ch = (uint8_t)val;
> diff --git a/hw/char/grlib_apbuart.c b/hw/char/grlib_apbuart.c
> index 610a317..1cb9026 100644
> --- a/hw/char/grlib_apbuart.c
> +++ b/hw/char/grlib_apbuart.c
> @@ -201,7 +201,7 @@ static void grlib_apbuart_write(void *opaque, hwaddr
> addr,
>      case DATA_OFFSET:
>      case DATA_OFFSET + 3:       /* When only one byte write */
>          /* Transmit when character device available and transmitter
> enabled */
> -        if (qemu_chr_fe_get_driver(&uart->chr) &&
> +        if (qemu_chr_fe_backend_connected(&uart->chr) &&
>              (uart->control & UART_TRANSMIT_ENABLE)) {
>              c = value & 0xFF;
>              /* XXX this blocks entire thread. Rewrite to use
> diff --git a/hw/char/ipoctal232.c b/hw/char/ipoctal232.c
> index 734e42c..b5ada43 100644
> --- a/hw/char/ipoctal232.c
> +++ b/hw/char/ipoctal232.c
> @@ -542,7 +542,7 @@ static void ipoctal_realize(DeviceState *dev, Error
> **errp)
>          ch->ipoctal = s;
>
>          /* Redirect IP-Octal channels to host character devices */
> -        if (qemu_chr_fe_get_driver(&ch->dev)) {
> +        if (qemu_chr_fe_backend_connected(&ch->dev)) {
>              qemu_chr_fe_set_handlers(&ch->dev, hostdev_can_receive,
>                                       hostdev_receive, hostdev_event,
>                                       NULL, ch, NULL, true);
> diff --git a/hw/char/parallel.c b/hw/char/parallel.c
> index c71a4a0..b3ed117 100644
> --- a/hw/char/parallel.c
> +++ b/hw/char/parallel.c
> @@ -512,7 +512,7 @@ static void parallel_isa_realizefn(DeviceState *dev,
> Error **errp)
>      int base;
>      uint8_t dummy;
>
> -    if (!qemu_chr_fe_get_driver(&s->chr)) {
> +    if (!qemu_chr_fe_backend_connected(&s->chr)) {
>          error_setg(errp, "Can't create parallel device, empty char
> device");
>          return;
>      }
> diff --git a/hw/char/sclpconsole-lm.c b/hw/char/sclpconsole-lm.c
> index ed1e2c5..ff54d19 100644
> --- a/hw/char/sclpconsole-lm.c
> +++ b/hw/char/sclpconsole-lm.c
> @@ -195,7 +195,7 @@ static int write_console_data(SCLPEvent *event, const
> uint8_t *buf, int len)
>  {
>      SCLPConsoleLM *scon = SCLPLM_CONSOLE(event);
>
> -    if (!qemu_chr_fe_get_driver(&scon->chr)) {
> +    if (!qemu_chr_fe_backend_connected(&scon->chr)) {
>          /* If there's no backend, we can just say we consumed all data. */
>          return len;
>      }
> diff --git a/hw/char/sclpconsole.c b/hw/char/sclpconsole.c
> index 9a65010..7e23d09 100644
> --- a/hw/char/sclpconsole.c
> +++ b/hw/char/sclpconsole.c
> @@ -163,7 +163,7 @@ static ssize_t write_console_data(SCLPEvent *event,
> const uint8_t *buf,
>  {
>      SCLPConsole *scon = SCLP_CONSOLE(event);
>
> -    if (!qemu_chr_fe_get_driver(&scon->chr)) {
> +    if (!qemu_chr_fe_backend_connected(&scon->chr)) {
>          /* If there's no backend, we can just say we consumed all data. */
>          return len;
>      }
> diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
> index c352337..868773f 100644
> --- a/hw/char/sh_serial.c
> +++ b/hw/char/sh_serial.c
> @@ -110,7 +110,7 @@ static void sh_serial_write(void *opaque, hwaddr offs,
>          }
>          return;
>      case 0x0c: /* FTDR / TDR */
> -        if (qemu_chr_fe_get_driver(&s->chr)) {
> +        if (qemu_chr_fe_backend_connected(&s->chr)) {
>              ch = val;
>              /* XXX this blocks entire thread. Rewrite to use
>               * qemu_chr_fe_write and background I/O callbacks */
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 9cdc0e0..6d1ccff 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -78,7 +78,7 @@ static void spapr_vty_realize(VIOsPAPRDevice *sdev,
> Error **errp)
>  {
>      VIOsPAPRVTYDevice *dev = VIO_SPAPR_VTY_DEVICE(sdev);
>
> -    if (!qemu_chr_fe_get_driver(&dev->chardev)) {
> +    if (!qemu_chr_fe_backend_connected(&dev->chardev)) {
>          error_setg(errp, "chardev property not set");
>          return;
>      }
> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> index 943a0f3..62803e5 100644
> --- a/hw/char/terminal3270.c
> +++ b/hw/char/terminal3270.c
> @@ -239,7 +239,7 @@ static int write_payload_3270(EmulatedCcw3270Device
> *dev, uint8_t cmd,
>              return 0;
>          }
>      }
> -    if (!qemu_chr_fe_get_driver(&t->chr)) {
> +    if (!qemu_chr_fe_backend_connected(&t->chr)) {
>          /* We just say we consumed all data if there's no backend. */
>          return count;
>      }
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index cb7975f..b066176 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -150,7 +150,7 @@ static void xencons_send(struct XenConsole *con)
>      ssize_t len, size;
>
>      size = con->buffer.size - con->buffer.consumed;
> -    if (qemu_chr_fe_get_driver(&con->chr)) {
> +    if (qemu_chr_fe_backend_connected(&con->chr)) {
>          len = qemu_chr_fe_write(&con->chr,
>                                  con->buffer.data + con->buffer.consumed,
>                                  size);
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index 6f2339d..0ca9e92 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -447,7 +447,7 @@ static void ipmi_bmc_extern_realize(DeviceState *dev,
> Error **errp)
>  {
>      IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(dev);
>
> -    if (!qemu_chr_fe_get_driver(&ibe->chr)) {
> +    if (!qemu_chr_fe_backend_connected(&ibe->chr)) {
>          error_setg(errp, "IPMI external bmc requires chardev attribute");
>          return;
>      }
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index e2dece8..a1be4bb 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1130,7 +1130,7 @@ static void ivshmem_doorbell_realize(PCIDevice *dev,
> Error **errp)
>  {
>      IVShmemState *s = IVSHMEM_COMMON(dev);
>
> -    if (!qemu_chr_fe_get_driver(&s->server_chr)) {
> +    if (!qemu_chr_fe_backend_connected(&s->server_chr)) {
>          error_setg(errp, "You must specify a 'chardev'");
>          return;
>      }
> @@ -1259,7 +1259,7 @@ static void ivshmem_realize(PCIDevice *dev, Error
> **errp)
>                       " or ivshmem-doorbell instead");
>      }
>
> -    if (!!qemu_chr_fe_get_driver(&s->server_chr) + !!s->shmobj != 1) {
> +    if (qemu_chr_fe_backend_connected(&s->server_chr) + !!s->shmobj != 1)
> {
>          error_setg(errp, "You must specify either 'shm' or 'chardev'");
>          return;
>      }
> diff --git a/hw/usb/ccid-card-passthru.c b/hw/usb/ccid-card-passthru.c
> index 9ace5ac..c0f8acd 100644
> --- a/hw/usb/ccid-card-passthru.c
> +++ b/hw/usb/ccid-card-passthru.c
> @@ -325,7 +325,7 @@ static void passthru_apdu_from_guest(
>  {
>      PassthruState *card = PASSTHRU_CCID_CARD(base);
>
> -    if (!qemu_chr_fe_get_driver(&card->cs)) {
> +    if (!qemu_chr_fe_backend_connected(&card->cs)) {
>          printf("ccid-passthru: no chardev, discarding apdu length %d\n",
> len);
>          return;
>      }
> @@ -346,7 +346,7 @@ static int passthru_initfn(CCIDCardState *base)
>
>      card->vscard_in_pos = 0;
>      card->vscard_in_hdr = 0;
> -    if (qemu_chr_fe_get_driver(&card->cs)) {
> +    if (qemu_chr_fe_backend_connected(&card->cs)) {
>          DPRINTF(card, D_INFO, "initing chardev\n");
>          qemu_chr_fe_set_handlers(&card->cs,
>              ccid_card_vscard_can_read,
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index e6b2c7c..7aa7290 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -483,13 +483,12 @@ static void usb_serial_realize(USBDevice *dev, Error
> **errp)
>  {
>      USBSerialState *s = USB_SERIAL_DEV(dev);
>      Error *local_err = NULL;
> -    Chardev *chr = qemu_chr_fe_get_driver(&s->cs);
>
>      usb_desc_create_serial(dev);
>      usb_desc_init(dev);
>      dev->auto_attach = 0;
>
> -    if (!chr) {
> +    if (!qemu_chr_fe_backend_connected(&s->cs)) {
>          error_setg(errp, "Property chardev is required");
>          return;
>      }
> @@ -504,7 +503,7 @@ static void usb_serial_realize(USBDevice *dev, Error
> **errp)
>                               usb_serial_event, NULL, s, NULL, true);
>      usb_serial_handle_reset(dev);
>
> -    if (chr->be_open && !dev->attached) {
> +    if (qemu_chr_fe_backend_open(&s->cs) && !dev->attached) {
>          usb_device_attach(dev, &error_abort);
>      }
>  }
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 1e9bf69..6992d92 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -273,10 +273,9 @@ static gboolean usbredir_write_unblocked(GIOChannel
> *chan, GIOCondition cond,
>  static int usbredir_write(void *priv, uint8_t *data, int count)
>  {
>      USBRedirDevice *dev = priv;
> -    Chardev *chr = qemu_chr_fe_get_driver(&dev->cs);
>      int r;
>
> -    if (!chr->be_open) {
> +    if (!qemu_chr_fe_backend_open(&dev->cs)) {
>          return 0;
>      }
>
> @@ -1366,7 +1365,7 @@ static void usbredir_realize(USBDevice *udev, Error
> **errp)
>      USBRedirDevice *dev = USB_REDIRECT(udev);
>      int i;
>
> -    if (!qemu_chr_fe_get_driver(&dev->cs)) {
> +    if (!qemu_chr_fe_backend_connected(&dev->cs)) {
>          error_setg(errp, QERR_MISSING_PARAMETER, "chardev");
>          return;
>      }
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index fa21535..e1a0e52 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -418,6 +418,13 @@ Chardev *qemu_chr_fe_get_driver(CharBackend *be);
>  bool qemu_chr_fe_backend_connected(CharBackend *be);
>
>  /**
> + * @qemu_chr_fe_backend_open:
> + *
> + * Returns true if chardevice associated with @be is open.
> + */
> +bool qemu_chr_fe_backend_open(CharBackend *be);
> +
> +/**
>   * @qemu_chr_fe_deinit:
>   *
>   * Dissociate the CharBackend from the Chardev.
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> index 06321d7..72a6077 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -163,7 +163,7 @@ static ssize_t
> filter_redirector_receive_iov(NetFilterState *nf,
>      MirrorState *s = FILTER_REDIRECTOR(nf);
>      int ret;
>
> -    if (qemu_chr_fe_get_driver(&s->chr_out)) {
> +    if (qemu_chr_fe_backend_connected(&s->chr_out)) {
>          ret = filter_mirror_send(&s->chr_out, iov, iovcnt);
>          if (ret) {
>              error_report("filter_mirror_send failed(%s)", strerror(-ret));
> --
> 2.7.4
>
>
> --
Marc-André Lureau

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

* Re: [Qemu-devel] [PATCH v2 7/9] serial: move TIOCM update to a separate function
  2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 7/9] serial: move TIOCM update to a separate function Anton Nefedov
@ 2017-05-25 14:36   ` Marc-André Lureau
  0 siblings, 0 replies; 17+ messages in thread
From: Marc-André Lureau @ 2017-05-25 14:36 UTC (permalink / raw)
  To: Anton Nefedov, qemu-devel; +Cc: pbonzini, Michael S . Tsirkin, den

On Fri, May 19, 2017 at 4:55 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:

> will be used by the following patch
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  hw/char/serial.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index d8d34d0..1e6bdeb 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -312,6 +312,24 @@ static void serial_write_fcr(SerialState *s, uint8_t
> val)
>      }
>  }
>
> +static void serial_update_tiocm(SerialState *s)
> +{
> +    int flags;
> +
> +    qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_GET_TIOCM, &flags);
> +
> +    flags &= ~(CHR_TIOCM_RTS | CHR_TIOCM_DTR);
> +
> +    if (s->mcr & UART_MCR_RTS) {
> +        flags |= CHR_TIOCM_RTS;
> +    }
> +    if (s->mcr & UART_MCR_DTR) {
> +        flags |= CHR_TIOCM_DTR;
> +    }
> +
> +    qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_TIOCM, &flags);
> +}
> +
>  static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
>                                  unsigned size)
>  {
> @@ -426,24 +444,13 @@ static void serial_ioport_write(void *opaque, hwaddr
> addr, uint64_t val,
>          break;
>      case 4:
>          {
> -            int flags;
>              int old_mcr = s->mcr;
>              s->mcr = val & 0x1f;
>              if (val & UART_MCR_LOOP)
>                  break;
>
>              if (s->poll_msl >= 0 && old_mcr != s->mcr) {
> -
> -                qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_GET_TIOCM,
> &flags);
> -
> -                flags &= ~(CHR_TIOCM_RTS | CHR_TIOCM_DTR);
> -
> -                if (val & UART_MCR_RTS)
> -                    flags |= CHR_TIOCM_RTS;
> -                if (val & UART_MCR_DTR)
> -                    flags |= CHR_TIOCM_DTR;
> -
> -                qemu_chr_fe_ioctl(&s->chr, CHR_IOCTL_SERIAL_SET_TIOCM,
> &flags);
> +                serial_update_tiocm(s);
>                  /* Update the modem status after a one-character-send
> wait-time, since there may be a response
>                     from the device/computer at the other end of the
> serial line */
>                  timer_mod(s->modem_status_poll,
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + s->char_transmit_time);
> --
> 2.7.4
>
>
> --
Marc-André Lureau

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

* Re: [Qemu-devel] [PATCH v2 3/9] char: chardevice hotswap
  2017-05-25 14:29   ` Marc-André Lureau
@ 2017-05-25 17:44     ` Anton Nefedov
  0 siblings, 0 replies; 17+ messages in thread
From: Anton Nefedov @ 2017-05-25 17:44 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel; +Cc: pbonzini, den

> Hi
> 

Hi Marc-André, thanks for reviewing, pls see answers below

> On Fri, May 19, 2017 at 4:51 PM Anton Nefedov <address@hidden>
> wrote:
> 
>> This patch adds a possibility to change a char device without a frontend
>> removal.
>>
>> 1. Ideally, it would have to happen transparently to a frontend, i.e.
>> frontend would continue its regular operation.
>> However, backends are not stateless and are set up by the frontends
>> via qemu_chr_fe_<> functions, and it's not (generally) possible to replay
>> that setup entirely in a backend code, as different chardevs respond
>> to the setup calls differently, so do frontends work differently basing
>> on those setup responses.
>> Moreover, some frontend can generally get and save the backend pointer
>> (qemu_chr_fe_get_driver()), and it will become invalid after backend
>> change.
>>
>> So, a frontend which would like to support chardev hotswap has to register
>> a "backend change" handler, and redo its backend setup there.
>>
>> 2. Write path can be used by multiple threads and thus protected with
>> chr_write_lock.
>> So hotswap also has to be protected so write functions won't access
>> a backend being replaced.
>>
>>
> Why not use the chr_write_lock then? (rename it chr_lock?)
> 

chr_write_lock is a Chardev property, it will be gone together with a
swapped device.

We could remove chr_write_lock and leave only the new be->chr_lock, 
since it covers everything; I guess the only problem is mux, where
multiple CharBackends share the same Chardev.

Would smth like this be better?:

static void char_lock(CharBackend *be)
{
     qemu_mutex_lock(be->chr && CHARDEV_IS_MUX(be->chr) ?
                     &be->chr->chr_lock : &be->chr_lock);
}

and use this instead of lock(be->chr_lock) and remove chr_write_lock.
(This would rely on a fact that mux is never hotswapped so be->chr_lock
is not needed).


> 
>> 3. Hotswap function can be called from e.g. a read handler of a monitor
>> socket. This can cause troubles so it's safer to defer execution to
>> a bottom-half (however, it means we cannot return some of the errors
>> synchronously - but most of them we can)
>>
> 
> What kind of troubles?
> 

if someone tried to hotswap monitor, it would look like:

(gdb) bt
#0 qmp_chardev_change (id=id@entry=0x7fc24bee7a60 "charmonitor",
backend=backend@entry=0x7fc24cc28290, errp=errp@entry=0x7ffde4d8d4f0)
at /mnt/code/us-qemu/chardev/char.c:1438
#1 0x00007fc249fd914b in hmp_chardev_change (mon=<optimized out>,
qdict=<optimized out>) at /mnt/code/us-qemu/hmp.c:2252
#2 0x00007fc249edeece in handle_hmp_command (mon=mon@entry=0x7fc24bef87c0,
cmdline=0x7fc24bf2345f "charmonitor
socket,path=/vz/vmprivate/centosos/mon.socket,server,nowait") at
/mnt/code/us-qemu/monitor.c:3100
#3 0x00007fc249ee02fa in monitor_command_cb (opaque=0x7fc24bef87c0,
cmdline=<optimized out>, readline_opaque=<optimized out>)
at /mnt/code/us-qemu/monitor.c:3897
#4 0x00007fc24a242428 in readline_handle_byte (rs=0x7fc24bf23450,
ch=<optimized out>) at /mnt/code/us-qemu/util/readline.c:393
#5 0x00007fc249edf0e2 in monitor_read (opaque=<optimized out>,
buf=<optimized out>, size=<optimized out>)
at /mnt/code/us-qemu/monitor.c:3880
#6 0x00007fc24a1deb2b in tcp_chr_read (chan=<optimized out>,
cond=<optimized out>, opaque=<optimized out>)
at /mnt/code/us-qemu/chardev/char-socket.c:441
#7 0x00007fc240997d7a in g_main_dispatch (context=0x7fc24beda950) at
gmain.c:3152
#8 g_main_context_dispatch (context=context@entry=0x7fc24beda950) at
gmain.c:3767
#9 0x00007fc24a22fe7c in glib_pollfds_poll () at
/mnt/code/us-qemu/util/main-loop.c:213
#10 os_host_main_loop_wait (timeout=<optimized out>) at
/mnt/code/us-qemu/util/main-loop.c:261
#11 main_loop_wait (nonblocking=nonblocking@entry=0) at
/mnt/code/us-qemu/util/main-loop.c:517
#12 0x00007fc249e9a19a in main_loop () at /mnt/code/us-qemu/vl.c:1900
#13 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized
out>) at /mnt/code/us-qemu/vl.c:4732

i.e. hotswap the device that is in tcp_chr_read (frame #6), and can
potentially be accessed (with old pointer on a stack) after it is
destroyed by qmp_chardev_change()

however, there is no support for monitor hotswap in the patchset. Maybe
it's not worth to mess with bottom-halves at all? It causes problems,
like those mentioned in your further comments


>>
>> Signed-off-by: Anton Nefedov <address@hidden>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>  chardev/char.c        | 147
>> ++++++++++++++++++++++++++++++++++++++++++++++----
>>  include/sysemu/char.h |  10 ++++
>>  qapi-schema.json      |  40 ++++++++++++++
>>  3 files changed, 187 insertions(+), 10 deletions(-)
>>
>> -bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
>> +static bool fe_connect(CharBackend *b, Chardev *s, Error **errp)
>>  {
>>
> 
> I would rather keep a qemu_chr prefix for consistency
> 

Done.

> 
>>      int tag = 0;
>>
>> @@ -507,6 +516,17 @@ unavailable:
>>      return false;
>>  }
>>
>> +bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
>> +{
>> +    if (!fe_connect(b, s, errp)) {
>> +        return false;
>> +    }
>> +
>> +    qemu_mutex_init(&b->chr_lock);
>> +    b->hotswap_bh = NULL;
>> +    return true;
>> +}
>> +
>>  static bool qemu_chr_is_busy(Chardev *s)
>>  {
>>      if (CHARDEV_IS_MUX(s)) {
>> @@ -531,6 +551,10 @@ void qemu_chr_fe_deinit(CharBackend *b)
>>              d->backends[b->tag] = NULL;
>>          }
>>          b->chr = NULL;
>> +        qemu_mutex_destroy(&b->chr_lock);
>> +        if (b->hotswap_bh) {
>> +            qemu_bh_delete(b->hotswap_bh);
>>
> 
> Could there be a chr_new leak here?
> 
> 

Yes you're right it can leak.
Maybe we don't need this asynchrony, see above, otherwise I will think
how to fix

>> +        }
>>      }
>>  }
>>
>> @@ -1308,6 +1332,109 @@ ChardevReturn *qmp_chardev_add(const char *id,
>> ChardevBackend *backend,
>>      return ret;
>>  }
>>
>> +static void chardev_change_bh(void *opaque)
>> +{
>> +    Chardev *chr_new = opaque;
>> +    const char *id = chr_new->label;
>> +    Chardev *chr = qemu_chr_find(id);
>>
> 
> Could chr be gone in the meantime? potentially
> 
> 

Potentially yes. Will fix (clean chr_new and bail out).


>> +
>> +    chr_new = qemu_chardev_new(NULL,
>> object_class_get_name(OBJECT_CLASS(cc)),
>> +                               backend, errp);
>> +    chr_new->label = g_strdup(id);
>>
> 
> move it below the check for !chr_new
> 
> 

my bad, thanks

>> +    if (!chr_new) {
>> +        return NULL;
>> +    }
>> +
>> +    if (chr->be->hotswap_bh) {
>> +        qemu_bh_delete(chr->be->hotswap_bh);
>> +    }
>>
> 
> Is it necessary to delete and recreate the bh? Could there be a chr_new
> leak if the bh didn't run? It's probably best to deny backend change while
> one is going on.
> 
> 

Hmm probably not, but new() is the only function that sets the argument.
Will think how to fix this if we decide to keep the bh.

>> +    chr->be->hotswap_bh = qemu_bh_new(chardev_change_bh, chr_new);
>> +    qemu_bh_schedule(chr->be->hotswap_bh);
>> +
>> +    ret = g_new0(ChardevReturn, 1);
>> +    if (CHARDEV_IS_PTY(chr_new)) {
>> +        ret->pty = g_strdup(chr_new->filename + 4);
>> +        ret->has_pty = true;
>> +    }
>> +
>> +    return ret;
>>
> 
> ah, potentially a case where qmp-async would be better..
> 
> +}
>> +
>>  void qmp_chardev_remove(const char *id, Error **errp)
>>  {
>>      Chardev *chr;
>> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
>> index 9f8df07..68c7876 100644
>> --- a/include/sysemu/char.h
>> +++ b/include/sysemu/char.h
>> @@ -92,6 +92,8 @@ typedef struct CharBackend {
>>      void *opaque;
>>      int tag;
>>      int fe_open;
>> +    QemuMutex chr_lock;
>> +    QEMUBH *hotswap_bh;
>>  } CharBackend;
>>
>>  struct Chardev {
>> @@ -141,6 +143,14 @@ void qemu_chr_parse_common(QemuOpts *opts,
>> ChardevCommon *backend);
>>   */
>>  Chardev *qemu_chr_new(const char *label, const char *filename);
>>
>> +/**
>> + * @qemu_chr_change:
>> + *
>> + * Change an existing character backend
>> + *
>> + * @opts the new backend options
>> + */
>> +void qemu_chr_change(QemuOpts *opts, Error **errp);
>>
>>
> This patch needs some test-char.c tests.
> 
> 

Will do

/Anton

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

end of thread, other threads:[~2017-05-25 17:45 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-19 12:47 [Qemu-devel] [PATCH v2 0/9] chardevice hotswap Anton Nefedov
2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 1/9] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
2017-05-25 14:29   ` Marc-André Lureau
2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 2/9] char: add backend hotswap handler Anton Nefedov
2017-05-25 14:32   ` Marc-André Lureau
2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 3/9] char: chardevice hotswap Anton Nefedov
2017-05-25 14:29   ` Marc-André Lureau
2017-05-25 17:44     ` Anton Nefedov
2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 4/9] hmp: add hmp analogue for qmp-chardev-change Anton Nefedov
2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 5/9] char: forbid direct chardevice access for hotswap devices Anton Nefedov
2017-05-25 14:31   ` Marc-André Lureau
2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 6/9] virtio-console: chardev hotswap support Anton Nefedov
2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 7/9] serial: move TIOCM update to a separate function Anton Nefedov
2017-05-25 14:36   ` Marc-André Lureau
2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 8/9] serial: chardev hotswap support Anton Nefedov
2017-05-19 12:47 ` [Qemu-devel] [PATCH v2 9/9] char: avoid chardevice direct access Anton Nefedov
2017-05-25 14:32   ` Marc-André Lureau

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