* [Qemu-devel] [PATCH v3 00/13] chardevice hotswap
@ 2017-05-30 13:57 Anton Nefedov
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 01/13] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
` (12 more replies)
0 siblings, 13 replies; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 UTC (permalink / raw)
To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov
Changed in v3:
- minor remarks to patch 1 applied
- patch 3: avoid using bottom-half, handle syncronously
As mentioned, it gets thing complicated and is only a problem for
a monitor-connected chardev hotswap and that is not supported for now
- tests added (patches 6-9)
========
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 (13):
char: move QemuOpts->ChardevBackend translation to a separate func
char: add backend hotswap handler
char: chardevice hotswap
char: forbid direct chardevice access for hotswap devices
char: avoid chardevice direct access
test-char: unref chardev-udp after test
test-char: split char_udp_test
test-char: split char_file_test
test-char: add hotswap test
hmp: add hmp analogue for qmp-chardev-change
virtio-console: chardev hotswap support
serial: move TIOCM update to a separate function
serial: chardev hotswap support
backends/rng-egd.c | 2 +-
chardev/char-mux.c | 1 +
chardev/char.c | 212 ++++++++++++++++++++++++++++-------
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 | 43 ++++++++
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 | 263 +++++++++++++++++++++++++++++++++-----------
tests/vhost-user-test.c | 2 +-
52 files changed, 669 insertions(+), 202 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v3 01/13] char: move QemuOpts->ChardevBackend translation to a separate func
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
@ 2017-05-30 13:57 ` Anton Nefedov
2017-05-31 19:18 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 02/13] char: add backend hotswap handler Anton Nefedov
` (11 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 UTC (permalink / raw)
To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov
parse function will be used by the following patch
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
chardev/char.c | 70 ++++++++++++++++++++++++++++++++++++----------------------
1 file changed, 44 insertions(+), 26 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index 4e24dc3..3a0f543 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -854,17 +854,13 @@ 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 +868,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 +883,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 +896,47 @@ 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] 30+ messages in thread
* [Qemu-devel] [PATCH v3 02/13] char: add backend hotswap handler
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 01/13] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
@ 2017-05-30 13:57 ` Anton Nefedov
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap Anton Nefedov
` (10 subsequent siblings)
12 siblings, 0 replies; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 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>
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 3a0f543..1b097b3 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 baa73c9..41bf2b8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4087,12 +4087,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 2639c7f..623f08c 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -543,7 +543,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");
}
@@ -560,7 +560,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");
}
@@ -588,9 +588,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);
@@ -801,9 +803,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 8b1b069..82bb157 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;
@@ -245,7 +245,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] 30+ messages in thread
* [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 01/13] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 02/13] char: add backend hotswap handler Anton Nefedov
@ 2017-05-30 13:57 ` Anton Nefedov
2017-05-31 19:20 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 04/13] char: forbid direct chardevice access for hotswap devices Anton Nefedov
` (9 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 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.
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
chardev/char.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++----
include/sysemu/char.h | 9 ++++
qapi-schema.json | 40 ++++++++++++++++
3 files changed, 165 insertions(+), 10 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index 1b097b3..ba1a5f5 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);
+
+ s = be->chr;
+ ret = s ? qemu_chr_write_all(s, buf, len) : 0;
- return qemu_chr_write_all(s, buf, len);
+ 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 qemu_chr_fe_connect(CharBackend *b, Chardev *s, Error **errp)
{
int tag = 0;
@@ -507,6 +516,16 @@ unavailable:
return false;
}
+bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
+{
+ if (!qemu_chr_fe_connect(b, s, errp)) {
+ return false;
+ }
+
+ qemu_mutex_init(&b->chr_lock);
+ return true;
+}
+
static bool qemu_chr_is_busy(Chardev *s)
{
if (CHARDEV_IS_MUX(s)) {
@@ -531,6 +550,7 @@ void qemu_chr_fe_deinit(CharBackend *b)
d->backends[b->tag] = NULL;
}
b->chr = NULL;
+ qemu_mutex_destroy(&b->chr_lock);
}
}
@@ -1306,6 +1326,92 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
return ret;
}
+ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
+ Error **errp)
+{
+ CharBackend *be;
+ const ChardevClass *cc;
+ Chardev *chr, *chr_new;
+ bool closed_sent = false;
+ 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;
+ }
+
+ be = chr->be;
+ if (!be) {
+ /* easy case */
+ object_unparent(OBJECT(chr));
+ return qmp_chardev_add(id, backend, errp);
+ }
+
+ if (!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);
+ if (!chr_new) {
+ return NULL;
+ }
+ chr_new->label = g_strdup(id);
+
+ 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;
+ qemu_chr_fe_connect(be, chr_new, &error_abort);
+
+ if (be->chr_be_change(be->opaque) < 0) {
+ error_setg(errp, "Chardev '%s' change failed", chr_new->label);
+ chr_new->be = NULL;
+ qemu_chr_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 NULL;
+ }
+ qemu_mutex_unlock(&be->chr_lock);
+
+ object_unparent(OBJECT(chr));
+ object_property_add_child(get_chardevs_root(), chr_new->label,
+ OBJECT(chr_new), &error_abort);
+ object_unref(OBJECT(chr_new));
+
+ 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..014ebbc 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -92,6 +92,7 @@ typedef struct CharBackend {
void *opaque;
int tag;
int fe_open;
+ QemuMutex chr_lock;
} CharBackend;
struct Chardev {
@@ -141,6 +142,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 e38c5f0..0f0df36 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5097,6 +5097,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] 30+ messages in thread
* [Qemu-devel] [PATCH v3 04/13] char: forbid direct chardevice access for hotswap devices
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (2 preceding siblings ...)
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap Anton Nefedov
@ 2017-05-30 13:57 ` Anton Nefedov
2017-05-31 19:21 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 05/13] char: avoid chardevice direct access Anton Nefedov
` (8 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 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 ba1a5f5..1eed934 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 qemu_chr_fe_connect(CharBackend *b, Chardev *s, Error **errp)
{
int tag = 0;
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 014ebbc..117d628 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -391,10 +391,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] 30+ messages in thread
* [Qemu-devel] [PATCH v3 05/13] char: avoid chardevice direct access
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (3 preceding siblings ...)
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 04/13] char: forbid direct chardevice access for hotswap devices Anton Nefedov
@ 2017-05-30 13:57 ` Anton Nefedov
2017-05-31 19:21 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 06/13] test-char: unref chardev-udp after test Anton Nefedov
` (7 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 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 1eed934..2d6e204 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 qemu_chr_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 117d628..342f531 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -405,6 +405,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 82bb157..a1295cc 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_send(&s->chr_out, iov, iovcnt);
if (ret) {
error_report("filter redirector send failed(%s)", strerror(-ret));
--
2.7.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v3 06/13] test-char: unref chardev-udp after test
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (4 preceding siblings ...)
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 05/13] char: avoid chardevice direct access Anton Nefedov
@ 2017-05-30 13:57 ` Anton Nefedov
2017-05-31 19:22 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 07/13] test-char: split char_udp_test Anton Nefedov
` (6 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 UTC (permalink / raw)
To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov
this is only not a problem if the test is last in a suite,
otherwise it makes the following main_loop() calls to fail
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
tests/test-char.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/test-char.c b/tests/test-char.c
index f3b377f..d63d3d2 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -456,6 +456,8 @@ static void char_udp_test(void)
close(sock);
g_free(tmp);
+ qemu_chr_fe_deinit(&be);
+ object_unparent(OBJECT(chr));
}
static void char_file_test(void)
--
2.7.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v3 07/13] test-char: split char_udp_test
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (5 preceding siblings ...)
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 06/13] test-char: unref chardev-udp after test Anton Nefedov
@ 2017-05-30 13:57 ` Anton Nefedov
2017-05-31 19:22 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 08/13] test-char: split char_file_test Anton Nefedov
` (5 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 UTC (permalink / raw)
To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov
makes it possible to test the existing chardev-udp
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
tests/test-char.c | 58 +++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 41 insertions(+), 17 deletions(-)
diff --git a/tests/test-char.c b/tests/test-char.c
index d63d3d2..ad0752a 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -413,16 +413,11 @@ static void char_pipe_test(void)
}
#endif
-static void char_udp_test(void)
+static int make_udp_socket(int *port)
{
- struct sockaddr_in addr = { 0, }, other;
- SocketIdleData d = { 0, };
- Chardev *chr;
- CharBackend be;
+ struct sockaddr_in addr = { 0, };
socklen_t alen = sizeof(addr);
int ret, sock = qemu_socket(PF_INET, SOCK_DGRAM, 0);
- char buf[10];
- char *tmp;
g_assert_cmpint(sock, >, 0);
addr.sin_family = AF_INET ;
@@ -433,19 +428,41 @@ static void char_udp_test(void)
ret = getsockname(sock, (struct sockaddr *)&addr, &alen);
g_assert_cmpint(ret, ==, 0);
- tmp = g_strdup_printf("udp:127.0.0.1:%d",
- ntohs(addr.sin_port));
- chr = qemu_chr_new("client", tmp);
- g_assert_nonnull(chr);
+ *port = ntohs(addr.sin_port);
+ return sock;
+}
+
+static void char_udp_test_internal(Chardev *reuse_chr, int sock)
+{
+ struct sockaddr_in other;
+ SocketIdleData d = { 0, };
+ Chardev *chr;
+ CharBackend *be;
+ socklen_t alen = sizeof(other);
+ int ret;
+ char buf[10];
+ char *tmp = NULL;
+
+ if (reuse_chr) {
+ chr = reuse_chr;
+ be = chr->be;
+ } else {
+ int port;
+ sock = make_udp_socket(&port);
+ tmp = g_strdup_printf("udp:127.0.0.1:%d", port);
+ chr = qemu_chr_new("client", tmp);
+ g_assert_nonnull(chr);
+
+ be = g_alloca(sizeof(CharBackend));
+ qemu_chr_fe_init(be, chr, &error_abort);
+ }
d.chr = chr;
- qemu_chr_fe_init(&be, chr, &error_abort);
- qemu_chr_fe_set_handlers(&be, socket_can_read_hello, socket_read_hello,
+ qemu_chr_fe_set_handlers(be, socket_can_read_hello, socket_read_hello,
NULL, NULL, &d, NULL, true);
ret = qemu_chr_write_all(chr, (uint8_t *)"hello", 5);
g_assert_cmpint(ret, ==, 5);
- alen = sizeof(addr);
ret = recvfrom(sock, buf, sizeof(buf), 0,
(struct sockaddr *)&other, &alen);
g_assert_cmpint(ret, ==, 5);
@@ -454,10 +471,17 @@ static void char_udp_test(void)
main_loop();
- close(sock);
+ if (!reuse_chr) {
+ close(sock);
+ qemu_chr_fe_deinit(be);
+ object_unparent(OBJECT(chr));
+ }
g_free(tmp);
- qemu_chr_fe_deinit(&be);
- object_unparent(OBJECT(chr));
+}
+
+static void char_udp_test(void)
+{
+ char_udp_test_internal(NULL, 0);
}
static void char_file_test(void)
--
2.7.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v3 08/13] test-char: split char_file_test
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (6 preceding siblings ...)
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 07/13] test-char: split char_udp_test Anton Nefedov
@ 2017-05-30 13:57 ` Anton Nefedov
2017-05-31 19:24 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 09/13] test-char: add hotswap test Anton Nefedov
` (4 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 UTC (permalink / raw)
To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov
makes it possible to test the existing chardev-file
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
tests/test-char.c | 127 +++++++++++++++++++++++++++++++++---------------------
1 file changed, 79 insertions(+), 48 deletions(-)
diff --git a/tests/test-char.c b/tests/test-char.c
index ad0752a..ed6b18f 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -484,75 +484,103 @@ static void char_udp_test(void)
char_udp_test_internal(NULL, 0);
}
-static void char_file_test(void)
+#ifndef _WIN32
+static void char_file_fifo_test(void)
{
+ Chardev *chr;
+ CharBackend be;
char *tmp_path = g_dir_make_tmp("qemu-test-char.XXXXXX", NULL);
+ char *fifo = g_build_filename(tmp_path, "fifo", NULL);
char *out = g_build_filename(tmp_path, "out", NULL);
- char *contents = NULL;
- ChardevFile file = { .out = out };
+ ChardevFile file = { .in = fifo,
+ .has_in = true,
+ .out = out };
ChardevBackend backend = { .type = CHARDEV_BACKEND_KIND_FILE,
.u.file.data = &file };
+ FeHandler fe = { 0, };
+ int fd, ret;
+
+ if (mkfifo(fifo, 0600) < 0) {
+ abort();
+ }
+
+ fd = open(fifo, O_RDWR);
+ ret = write(fd, "fifo-in", 8);
+ g_assert_cmpint(ret, ==, 8);
+
+ chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
+ &error_abort);
+
+ qemu_chr_fe_init(&be, chr, &error_abort);
+ qemu_chr_fe_set_handlers(&be,
+ fe_can_read,
+ fe_read,
+ fe_event,
+ NULL,
+ &fe, NULL, true);
+
+ main_loop();
+
+ close(fd);
+
+ g_assert_cmpint(fe.read_count, ==, 8);
+ g_assert_cmpstr(fe.read_buf, ==, "fifo-in");
+
+ qemu_chr_fe_deinit(&be);
+ object_unref(OBJECT(chr));
+
+ g_unlink(fifo);
+ g_free(fifo);
+ g_unlink(out);
+ g_free(out);
+ g_rmdir(tmp_path);
+ g_free(tmp_path);
+}
+#endif
+
+static void char_file_test_internal(Chardev *ext_chr, const char *filepath)
+{
+ char *tmp_path = g_dir_make_tmp("qemu-test-char.XXXXXX", NULL);
+ char *out;
Chardev *chr;
+ char *contents = NULL;
+ ChardevFile file = {};
+ ChardevBackend backend = { .type = CHARDEV_BACKEND_KIND_FILE,
+ .u.file.data = &file };
gsize length;
int ret;
- chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
- &error_abort);
+ if (ext_chr) {
+ chr = ext_chr;
+ out = g_strdup(filepath);
+ file.out = out;
+ } else {
+ out = g_build_filename(tmp_path, "out", NULL);
+ file.out = out;
+ chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
+ &error_abort);
+ }
ret = qemu_chr_write_all(chr, (uint8_t *)"hello!", 6);
g_assert_cmpint(ret, ==, 6);
- object_unref(OBJECT(chr));
ret = g_file_get_contents(out, &contents, &length, NULL);
g_assert(ret == TRUE);
g_assert_cmpint(length, ==, 6);
g_assert(strncmp(contents, "hello!", 6) == 0);
- g_free(contents);
-
-#ifndef _WIN32
- {
- CharBackend be;
- FeHandler fe = { 0, };
- char *fifo = g_build_filename(tmp_path, "fifo", NULL);
- int fd;
-
- if (mkfifo(fifo, 0600) < 0) {
- abort();
- }
-
- fd = open(fifo, O_RDWR);
- ret = write(fd, "fifo-in", 8);
- g_assert_cmpint(ret, ==, 8);
-
- file.in = fifo;
- file.has_in = true;
- chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
- &error_abort);
-
- qemu_chr_fe_init(&be, chr, &error_abort);
- qemu_chr_fe_set_handlers(&be,
- fe_can_read,
- fe_read,
- fe_event,
- NULL,
- &fe, NULL, true);
-
- main_loop();
- close(fd);
-
- g_assert_cmpint(fe.read_count, ==, 8);
- g_assert_cmpstr(fe.read_buf, ==, "fifo-in");
- qemu_chr_fe_deinit(&be);
+ if (!ext_chr) {
object_unref(OBJECT(chr));
- g_unlink(fifo);
- g_free(fifo);
+ g_unlink(out);
+ g_free(out);
}
-#endif
-
- g_unlink(out);
+ g_free(contents);
g_rmdir(tmp_path);
g_free(tmp_path);
- g_free(out);
+}
+
+static void char_file_test(void)
+{
+ char_file_test_internal(NULL, NULL);
}
static void char_null_test(void)
@@ -633,6 +661,9 @@ int main(int argc, char **argv)
g_test_add_func("/char/pipe", char_pipe_test);
#endif
g_test_add_func("/char/file", char_file_test);
+#ifndef _WIN32
+ g_test_add_func("/char/file-fifo", char_file_fifo_test);
+#endif
g_test_add_func("/char/socket", char_socket_test);
g_test_add_func("/char/udp", char_udp_test);
--
2.7.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v3 09/13] test-char: add hotswap test
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (7 preceding siblings ...)
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 08/13] test-char: split char_file_test Anton Nefedov
@ 2017-05-30 13:57 ` Anton Nefedov
2017-05-31 19:25 ` Marc-André Lureau
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change Anton Nefedov
` (3 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:57 UTC (permalink / raw)
To: qemu-devel; +Cc: den, pbonzini, marcandre.lureau, Anton Nefedov
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
tests/test-char.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/tests/test-char.c b/tests/test-char.c
index ed6b18f..cd54f88 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -635,6 +635,73 @@ static void char_invalid_test(void)
g_assert_null(chr);
}
+static int chardev_change(void *opaque)
+{
+ return 0;
+}
+
+static int chardev_change_denied(void *opaque)
+{
+ return -1;
+}
+
+static void char_hotswap_test(void)
+{
+ char *chr_args;
+ Chardev *chr;
+ CharBackend be;
+
+ gchar *tmp_path = g_dir_make_tmp("qemu-test-char.XXXXXX", NULL);
+ char *filename = g_build_filename(tmp_path, "file", NULL);
+ ChardevFile file = { .out = filename };
+ ChardevBackend backend = { .type = CHARDEV_BACKEND_KIND_FILE,
+ .u.file.data = &file };
+
+ int port;
+ int sock = make_udp_socket(&port);
+ g_assert_cmpint(sock, >, 0);
+
+ chr_args = g_strdup_printf("udp:127.0.0.1:%d", port);
+
+ chr = qemu_chr_new("chardev", chr_args);
+ qemu_chr_fe_init(&be, chr, &error_abort);
+
+ /* check that chardev operates correctly */
+ char_udp_test_internal(chr, sock);
+
+ /* set the handler that denies the hotswap */
+ qemu_chr_fe_set_handlers(&be, NULL, NULL,
+ NULL, chardev_change_denied, NULL, NULL, true);
+
+ /* now, change is denied and has to keep the old backend operating */
+ qmp_chardev_change("chardev", &backend, NULL);
+ g_assert(be.chr == chr);
+
+ char_udp_test_internal(chr, sock);
+
+ /* now allow the change */
+ qemu_chr_fe_set_handlers(&be, NULL, NULL,
+ NULL, chardev_change, NULL, NULL, true);
+
+ /* has to succeed now */
+ qmp_chardev_change("chardev", &backend, &error_abort);
+ g_assert(be.chr != chr);
+
+ close(sock);
+ chr = be.chr;
+
+ /* run the file chardev test */
+ char_file_test_internal(chr, filename);
+
+ object_unparent(OBJECT(chr));
+
+ g_unlink(filename);
+ g_free(filename);
+ g_rmdir(tmp_path);
+ g_free(tmp_path);
+ g_free(chr_args);
+}
+
int main(int argc, char **argv)
{
qemu_init_main_loop(&error_abort);
@@ -666,6 +733,7 @@ int main(int argc, char **argv)
#endif
g_test_add_func("/char/socket", char_socket_test);
g_test_add_func("/char/udp", char_udp_test);
+ g_test_add_func("/char/hotswap", char_hotswap_test);
return g_test_run();
}
--
2.7.4
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (8 preceding siblings ...)
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 09/13] test-char: add hotswap test Anton Nefedov
@ 2017-05-30 13:58 ` Anton Nefedov
2017-06-01 7:12 ` Marc-André Lureau
2017-06-01 14:56 ` Dr. David Alan Gilbert
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 11/13] virtio-console: chardev hotswap support Anton Nefedov
` (2 subsequent siblings)
12 siblings, 2 replies; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:58 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 | 2 +-
hmp-commands.hx | 16 ++++++++++++++++
hmp.c | 34 ++++++++++++++++++++++++++++++++++
hmp.h | 1 +
include/sysemu/char.h | 12 ++++++++++++
5 files changed, 64 insertions(+), 1 deletion(-)
diff --git a/chardev/char.c b/chardev/char.c
index 2d6e204..1e2a2dd 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -888,7 +888,7 @@ 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 baeac47..19bfddd 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 20f5dab..7660495 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2224,6 +2224,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 342f531..18fcd26 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -131,6 +131,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] 30+ messages in thread
* [Qemu-devel] [PATCH v3 11/13] virtio-console: chardev hotswap support
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (9 preceding siblings ...)
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change Anton Nefedov
@ 2017-05-30 13:58 ` Anton Nefedov
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 12/13] serial: move TIOCM update to a separate function Anton Nefedov
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 13/13] serial: chardev hotswap support Anton Nefedov
12 siblings, 0 replies; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:58 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] 30+ messages in thread
* [Qemu-devel] [PATCH v3 12/13] serial: move TIOCM update to a separate function
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (10 preceding siblings ...)
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 11/13] virtio-console: chardev hotswap support Anton Nefedov
@ 2017-05-30 13:58 ` Anton Nefedov
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 13/13] serial: chardev hotswap support Anton Nefedov
12 siblings, 0 replies; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:58 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>
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v3 13/13] serial: chardev hotswap support
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
` (11 preceding siblings ...)
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 12/13] serial: move TIOCM update to a separate function Anton Nefedov
@ 2017-05-30 13:58 ` Anton Nefedov
12 siblings, 0 replies; 30+ messages in thread
From: Anton Nefedov @ 2017-05-30 13:58 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] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 01/13] char: move QemuOpts->ChardevBackend translation to a separate func
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 01/13] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
@ 2017-05-31 19:18 ` Marc-André Lureau
0 siblings, 0 replies; 30+ messages in thread
From: Marc-André Lureau @ 2017-05-31 19:18 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
On Tue, May 30, 2017 at 6:09 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:
> parse function will be used by the following patch
>
> 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>
---
> chardev/char.c | 70
> ++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 4e24dc3..3a0f543 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -854,17 +854,13 @@ 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 +868,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 +883,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 +896,47 @@ 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
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap Anton Nefedov
@ 2017-05-31 19:20 ` Marc-André Lureau
2017-06-01 11:23 ` Anton Nefedov
0 siblings, 1 reply; 30+ messages in thread
From: Marc-André Lureau @ 2017-05-31 19:20 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
Hi
On Tue, May 30, 2017 at 6:12 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.
>
>
Tbh, I don't understand the need for a different lock. Could you explain?
Even better would be to write a test that shows in which way the lock helps.
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
patch looks good otherwise
> ---
> chardev/char.c | 126
> ++++++++++++++++++++++++++++++++++++++++++++++----
> include/sysemu/char.h | 9 ++++
> qapi-schema.json | 40 ++++++++++++++++
> 3 files changed, 165 insertions(+), 10 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 1b097b3..ba1a5f5 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);
> +
> + s = be->chr;
> + ret = s ? qemu_chr_write_all(s, buf, len) : 0;
>
> - return qemu_chr_write_all(s, buf, len);
> + 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 qemu_chr_fe_connect(CharBackend *b, Chardev *s, Error **errp)
> {
> int tag = 0;
>
> @@ -507,6 +516,16 @@ unavailable:
> return false;
> }
>
> +bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
> +{
> + if (!qemu_chr_fe_connect(b, s, errp)) {
> + return false;
> + }
> +
> + qemu_mutex_init(&b->chr_lock);
> + return true;
> +}
> +
> static bool qemu_chr_is_busy(Chardev *s)
> {
> if (CHARDEV_IS_MUX(s)) {
> @@ -531,6 +550,7 @@ void qemu_chr_fe_deinit(CharBackend *b)
> d->backends[b->tag] = NULL;
> }
> b->chr = NULL;
> + qemu_mutex_destroy(&b->chr_lock);
> }
> }
>
> @@ -1306,6 +1326,92 @@ ChardevReturn *qmp_chardev_add(const char *id,
> ChardevBackend *backend,
> return ret;
> }
>
> +ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> + Error **errp)
> +{
> + CharBackend *be;
> + const ChardevClass *cc;
> + Chardev *chr, *chr_new;
> + bool closed_sent = false;
> + 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;
> + }
> +
> + be = chr->be;
> + if (!be) {
> + /* easy case */
> + object_unparent(OBJECT(chr));
> + return qmp_chardev_add(id, backend, errp);
> + }
> +
> + if (!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);
> + if (!chr_new) {
> + return NULL;
> + }
> + chr_new->label = g_strdup(id);
> +
> + 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;
> + qemu_chr_fe_connect(be, chr_new, &error_abort);
> +
> + if (be->chr_be_change(be->opaque) < 0) {
> + error_setg(errp, "Chardev '%s' change failed", chr_new->label);
> + chr_new->be = NULL;
> + qemu_chr_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 NULL;
> + }
> + qemu_mutex_unlock(&be->chr_lock);
> +
> + object_unparent(OBJECT(chr));
> + object_property_add_child(get_chardevs_root(), chr_new->label,
> + OBJECT(chr_new), &error_abort);
> + object_unref(OBJECT(chr_new));
> +
> + 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..014ebbc 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -92,6 +92,7 @@ typedef struct CharBackend {
> void *opaque;
> int tag;
> int fe_open;
> + QemuMutex chr_lock;
> } CharBackend;
>
> struct Chardev {
> @@ -141,6 +142,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 e38c5f0..0f0df36 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -5097,6 +5097,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] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/13] char: forbid direct chardevice access for hotswap devices
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 04/13] char: forbid direct chardevice access for hotswap devices Anton Nefedov
@ 2017-05-31 19:21 ` Marc-André Lureau
0 siblings, 0 replies; 30+ messages in thread
From: Marc-André Lureau @ 2017-05-31 19:21 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
On Tue, May 30, 2017 at 6:13 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>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.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 ba1a5f5..1eed934 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 qemu_chr_fe_connect(CharBackend *b, Chardev *s, Error **errp)
> {
> int tag = 0;
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 014ebbc..117d628 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -391,10 +391,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] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 05/13] char: avoid chardevice direct access
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 05/13] char: avoid chardevice direct access Anton Nefedov
@ 2017-05-31 19:21 ` Marc-André Lureau
0 siblings, 0 replies; 30+ messages in thread
From: Marc-André Lureau @ 2017-05-31 19:21 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
On Tue, May 30, 2017 at 5:59 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>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.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 1eed934..2d6e204 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 qemu_chr_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 117d628..342f531 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -405,6 +405,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 82bb157..a1295cc 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_send(&s->chr_out, iov, iovcnt);
> if (ret) {
> error_report("filter redirector send failed(%s)",
> strerror(-ret));
> --
> 2.7.4
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 06/13] test-char: unref chardev-udp after test
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 06/13] test-char: unref chardev-udp after test Anton Nefedov
@ 2017-05-31 19:22 ` Marc-André Lureau
0 siblings, 0 replies; 30+ messages in thread
From: Marc-André Lureau @ 2017-05-31 19:22 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
On Tue, May 30, 2017 at 6:06 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:
> this is only not a problem if the test is last in a suite,
> otherwise it makes the following main_loop() calls to fail
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> tests/test-char.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tests/test-char.c b/tests/test-char.c
> index f3b377f..d63d3d2 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -456,6 +456,8 @@ static void char_udp_test(void)
>
> close(sock);
> g_free(tmp);
> + qemu_chr_fe_deinit(&be);
> + object_unparent(OBJECT(chr));
> }
>
> static void char_file_test(void)
> --
> 2.7.4
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 07/13] test-char: split char_udp_test
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 07/13] test-char: split char_udp_test Anton Nefedov
@ 2017-05-31 19:22 ` Marc-André Lureau
0 siblings, 0 replies; 30+ messages in thread
From: Marc-André Lureau @ 2017-05-31 19:22 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
On Tue, May 30, 2017 at 6:02 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:
> makes it possible to test the existing chardev-udp
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> tests/test-char.c | 58
> +++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/tests/test-char.c b/tests/test-char.c
> index d63d3d2..ad0752a 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -413,16 +413,11 @@ static void char_pipe_test(void)
> }
> #endif
>
> -static void char_udp_test(void)
> +static int make_udp_socket(int *port)
> {
> - struct sockaddr_in addr = { 0, }, other;
> - SocketIdleData d = { 0, };
> - Chardev *chr;
> - CharBackend be;
> + struct sockaddr_in addr = { 0, };
> socklen_t alen = sizeof(addr);
> int ret, sock = qemu_socket(PF_INET, SOCK_DGRAM, 0);
> - char buf[10];
> - char *tmp;
>
> g_assert_cmpint(sock, >, 0);
> addr.sin_family = AF_INET ;
> @@ -433,19 +428,41 @@ static void char_udp_test(void)
> ret = getsockname(sock, (struct sockaddr *)&addr, &alen);
> g_assert_cmpint(ret, ==, 0);
>
> - tmp = g_strdup_printf("udp:127.0.0.1:%d",
> - ntohs(addr.sin_port));
> - chr = qemu_chr_new("client", tmp);
> - g_assert_nonnull(chr);
> + *port = ntohs(addr.sin_port);
> + return sock;
> +}
> +
> +static void char_udp_test_internal(Chardev *reuse_chr, int sock)
> +{
> + struct sockaddr_in other;
> + SocketIdleData d = { 0, };
> + Chardev *chr;
> + CharBackend *be;
> + socklen_t alen = sizeof(other);
> + int ret;
> + char buf[10];
> + char *tmp = NULL;
> +
> + if (reuse_chr) {
> + chr = reuse_chr;
> + be = chr->be;
> + } else {
> + int port;
> + sock = make_udp_socket(&port);
> + tmp = g_strdup_printf("udp:127.0.0.1:%d", port);
> + chr = qemu_chr_new("client", tmp);
> + g_assert_nonnull(chr);
> +
> + be = g_alloca(sizeof(CharBackend));
> + qemu_chr_fe_init(be, chr, &error_abort);
> + }
>
> d.chr = chr;
> - qemu_chr_fe_init(&be, chr, &error_abort);
> - qemu_chr_fe_set_handlers(&be, socket_can_read_hello,
> socket_read_hello,
> + qemu_chr_fe_set_handlers(be, socket_can_read_hello, socket_read_hello,
> NULL, NULL, &d, NULL, true);
> ret = qemu_chr_write_all(chr, (uint8_t *)"hello", 5);
> g_assert_cmpint(ret, ==, 5);
>
> - alen = sizeof(addr);
> ret = recvfrom(sock, buf, sizeof(buf), 0,
> (struct sockaddr *)&other, &alen);
> g_assert_cmpint(ret, ==, 5);
> @@ -454,10 +471,17 @@ static void char_udp_test(void)
>
> main_loop();
>
> - close(sock);
> + if (!reuse_chr) {
> + close(sock);
> + qemu_chr_fe_deinit(be);
> + object_unparent(OBJECT(chr));
> + }
> g_free(tmp);
> - qemu_chr_fe_deinit(&be);
> - object_unparent(OBJECT(chr));
> +}
> +
> +static void char_udp_test(void)
> +{
> + char_udp_test_internal(NULL, 0);
> }
>
> static void char_file_test(void)
> --
> 2.7.4
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 08/13] test-char: split char_file_test
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 08/13] test-char: split char_file_test Anton Nefedov
@ 2017-05-31 19:24 ` Marc-André Lureau
0 siblings, 0 replies; 30+ messages in thread
From: Marc-André Lureau @ 2017-05-31 19:24 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
On Tue, May 30, 2017 at 6:15 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:
> makes it possible to test the existing chardev-file
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
> tests/test-char.c | 127
> +++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 79 insertions(+), 48 deletions(-)
>
> diff --git a/tests/test-char.c b/tests/test-char.c
> index ad0752a..ed6b18f 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -484,75 +484,103 @@ static void char_udp_test(void)
> char_udp_test_internal(NULL, 0);
> }
>
> -static void char_file_test(void)
> +#ifndef _WIN32
> +static void char_file_fifo_test(void)
> {
> + Chardev *chr;
> + CharBackend be;
> char *tmp_path = g_dir_make_tmp("qemu-test-char.XXXXXX", NULL);
> + char *fifo = g_build_filename(tmp_path, "fifo", NULL);
> char *out = g_build_filename(tmp_path, "out", NULL);
> - char *contents = NULL;
> - ChardevFile file = { .out = out };
> + ChardevFile file = { .in = fifo,
> + .has_in = true,
> + .out = out };
> ChardevBackend backend = { .type = CHARDEV_BACKEND_KIND_FILE,
> .u.file.data = &file };
> + FeHandler fe = { 0, };
> + int fd, ret;
> +
> + if (mkfifo(fifo, 0600) < 0) {
> + abort();
> + }
> +
> + fd = open(fifo, O_RDWR);
> + ret = write(fd, "fifo-in", 8);
> + g_assert_cmpint(ret, ==, 8);
> +
> + chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
> + &error_abort);
> +
> + qemu_chr_fe_init(&be, chr, &error_abort);
> + qemu_chr_fe_set_handlers(&be,
> + fe_can_read,
> + fe_read,
> + fe_event,
> + NULL,
> + &fe, NULL, true);
> +
> + main_loop();
> +
> + close(fd);
> +
> + g_assert_cmpint(fe.read_count, ==, 8);
> + g_assert_cmpstr(fe.read_buf, ==, "fifo-in");
> +
> + qemu_chr_fe_deinit(&be);
> + object_unref(OBJECT(chr));
> +
> + g_unlink(fifo);
> + g_free(fifo);
> + g_unlink(out);
> + g_free(out);
> + g_rmdir(tmp_path);
> + g_free(tmp_path);
> +}
> +#endif
> +
> +static void char_file_test_internal(Chardev *ext_chr, const char
> *filepath)
> +{
> + char *tmp_path = g_dir_make_tmp("qemu-test-char.XXXXXX", NULL);
> + char *out;
> Chardev *chr;
> + char *contents = NULL;
> + ChardevFile file = {};
> + ChardevBackend backend = { .type = CHARDEV_BACKEND_KIND_FILE,
> + .u.file.data = &file };
> gsize length;
> int ret;
>
> - chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
> - &error_abort);
> + if (ext_chr) {
> + chr = ext_chr;
> + out = g_strdup(filepath);
> + file.out = out;
> + } else {
> + out = g_build_filename(tmp_path, "out", NULL);
> + file.out = out;
>
+ chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
> + &error_abort);
> + }
>
ret = qemu_chr_write_all(chr, (uint8_t *)"hello!", 6);
> g_assert_cmpint(ret, ==, 6);
> - object_unref(OBJECT(chr));
>
> ret = g_file_get_contents(out, &contents, &length, NULL);
> g_assert(ret == TRUE);
> g_assert_cmpint(length, ==, 6);
> g_assert(strncmp(contents, "hello!", 6) == 0);
> - g_free(contents);
> -
> -#ifndef _WIN32
> - {
> - CharBackend be;
> - FeHandler fe = { 0, };
> - char *fifo = g_build_filename(tmp_path, "fifo", NULL);
> - int fd;
> -
> - if (mkfifo(fifo, 0600) < 0) {
> - abort();
> - }
> -
> - fd = open(fifo, O_RDWR);
> - ret = write(fd, "fifo-in", 8);
> - g_assert_cmpint(ret, ==, 8);
> -
> - file.in = fifo;
> - file.has_in = true;
> - chr = qemu_chardev_new(NULL, TYPE_CHARDEV_FILE, &backend,
> - &error_abort);
> -
> - qemu_chr_fe_init(&be, chr, &error_abort);
> - qemu_chr_fe_set_handlers(&be,
> - fe_can_read,
> - fe_read,
> - fe_event,
> - NULL,
> - &fe, NULL, true);
> -
> - main_loop();
>
> - close(fd);
> -
> - g_assert_cmpint(fe.read_count, ==, 8);
> - g_assert_cmpstr(fe.read_buf, ==, "fifo-in");
> - qemu_chr_fe_deinit(&be);
> + if (!ext_chr) {
> object_unref(OBJECT(chr));
> - g_unlink(fifo);
> - g_free(fifo);
> + g_unlink(out);
> + g_free(out);
> }
>
It leaks out in the other case, move the free outside the condition.
other than that:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> -#endif
> -
> - g_unlink(out);
> + g_free(contents);
> g_rmdir(tmp_path);
> g_free(tmp_path);
> - g_free(out);
> +}
> +
> +static void char_file_test(void)
> +{
> + char_file_test_internal(NULL, NULL);
> }
>
> static void char_null_test(void)
> @@ -633,6 +661,9 @@ int main(int argc, char **argv)
> g_test_add_func("/char/pipe", char_pipe_test);
> #endif
> g_test_add_func("/char/file", char_file_test);
> +#ifndef _WIN32
> + g_test_add_func("/char/file-fifo", char_file_fifo_test);
> +#endif
> g_test_add_func("/char/socket", char_socket_test);
> g_test_add_func("/char/udp", char_udp_test);
>
> --
> 2.7.4
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 09/13] test-char: add hotswap test
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 09/13] test-char: add hotswap test Anton Nefedov
@ 2017-05-31 19:25 ` Marc-André Lureau
0 siblings, 0 replies; 30+ messages in thread
From: Marc-André Lureau @ 2017-05-31 19:25 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
Hi
On Tue, May 30, 2017 at 6:15 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
> tests/test-char.c | 68
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/tests/test-char.c b/tests/test-char.c
> index ed6b18f..cd54f88 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -635,6 +635,73 @@ static void char_invalid_test(void)
> g_assert_null(chr);
> }
>
> +static int chardev_change(void *opaque)
> +{
> + return 0;
> +}
> +
> +static int chardev_change_denied(void *opaque)
> +{
> + return -1;
> +}
> +
> +static void char_hotswap_test(void)
> +{
> + char *chr_args;
> + Chardev *chr;
> + CharBackend be;
> +
> + gchar *tmp_path = g_dir_make_tmp("qemu-test-char.XXXXXX", NULL);
> + char *filename = g_build_filename(tmp_path, "file", NULL);
> + ChardevFile file = { .out = filename };
> + ChardevBackend backend = { .type = CHARDEV_BACKEND_KIND_FILE,
> + .u.file.data = &file };
> +
> + int port;
> + int sock = make_udp_socket(&port);
> + g_assert_cmpint(sock, >, 0);
> +
> + chr_args = g_strdup_printf("udp:127.0.0.1:%d", port);
> +
> + chr = qemu_chr_new("chardev", chr_args);
> + qemu_chr_fe_init(&be, chr, &error_abort);
> +
> + /* check that chardev operates correctly */
> + char_udp_test_internal(chr, sock);
> +
> + /* set the handler that denies the hotswap */
> + qemu_chr_fe_set_handlers(&be, NULL, NULL,
> + NULL, chardev_change_denied, NULL, NULL,
> true);
> +
> + /* now, change is denied and has to keep the old backend operating */
> + qmp_chardev_change("chardev", &backend, NULL);
>
Please add qapi_free_ChardevReturn(ret);
other than that:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
+ g_assert(be.chr == chr);
> +
> + char_udp_test_internal(chr, sock);
> +
> + /* now allow the change */
> + qemu_chr_fe_set_handlers(&be, NULL, NULL,
> + NULL, chardev_change, NULL, NULL, true);
> +
> + /* has to succeed now */
> + qmp_chardev_change("chardev", &backend, &error_abort);
> + g_assert(be.chr != chr);
> +
> + close(sock);
> + chr = be.chr;
> +
> + /* run the file chardev test */
> + char_file_test_internal(chr, filename);
> +
> + object_unparent(OBJECT(chr));
> +
> + g_unlink(filename);
> + g_free(filename);
> + g_rmdir(tmp_path);
> + g_free(tmp_path);
> + g_free(chr_args);
> +}
> +
> int main(int argc, char **argv)
> {
> qemu_init_main_loop(&error_abort);
> @@ -666,6 +733,7 @@ int main(int argc, char **argv)
> #endif
> g_test_add_func("/char/socket", char_socket_test);
> g_test_add_func("/char/udp", char_udp_test);
> + g_test_add_func("/char/hotswap", char_hotswap_test);
>
> return g_test_run();
> }
> --
> 2.7.4
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change Anton Nefedov
@ 2017-06-01 7:12 ` Marc-André Lureau
2017-06-01 11:24 ` Anton Nefedov
2017-06-01 14:56 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 30+ messages in thread
From: Marc-André Lureau @ 2017-06-01 7:12 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den, Dr . David Alan Gilbert
Hi
On Tue, May 30, 2017 at 6:08 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>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> chardev/char.c | 2 +-
> hmp-commands.hx | 16 ++++++++++++++++
> hmp.c | 34 ++++++++++++++++++++++++++++++++++
> hmp.h | 1 +
> include/sysemu/char.h | 12 ++++++++++++
> 5 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 2d6e204..1e2a2dd 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -888,7 +888,7 @@ 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 baeac47..19bfddd 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
>
chardev_change vs chardev-change. Apparently, there is this difference for
chardev-add too
> +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 20f5dab..7660495 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2224,6 +2224,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");
>
enlighten me, how is this triggered?
> + 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 342f531..18fcd26 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -131,6 +131,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
> + *
>
You may remove that line
> + * Returns: a new backend
>
or NULL on error
> + */
> +ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> + Error **errp);
> +
> +/**
> * @qemu_chr_new:
> *
> * Create a new character backend from a URI.
> --
> 2.7.4
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap
2017-05-31 19:20 ` Marc-André Lureau
@ 2017-06-01 11:23 ` Anton Nefedov
2017-06-09 14:53 ` Marc-André Lureau
0 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-06-01 11:23 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: pbonzini, den
On 05/31/2017 10:20 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, May 30, 2017 at 6:12 PM Anton Nefedov
> <anton.nefedov@virtuozzo.com <mailto: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.
>
>
> Tbh, I don't understand the need for a different lock. Could you
> explain? Even better would be to write a test that shows in which way
> the lock helps.
>
hi Marc-André,
The existing chr_write_lock belongs to Chardev.
For the hotswap case, we need to ensure that be->chr won't change and
the old Chardev (together with its mutex) won't be destroyed while it's
used in the write functions.
Maybe we could move the lock to CharBackend, instead of creating a new
one. But I guess this
1. won't work for mux, where multiple CharBackends share the same
Chardev
2. won't work for some chardevs, like pty which uses the lock for the
timer handler
Sorry if I'm not explaining clearly enough or maybe I'm missing some
easier solution?
I can try to add a test but can't quite see yet how to freeze the old
chardev somewhere in cc->chr_write() and hotswap it while it's there.
>
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com
> <mailto:anton.nefedov@virtuozzo.com>>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com
> <mailto:vsementsov@virtuozzo.com>>
>
>
> patch looks good otherwise
>
> ---
> chardev/char.c | 126
> ++++++++++++++++++++++++++++++++++++++++++++++----
> include/sysemu/char.h | 9 ++++
> qapi-schema.json | 40 ++++++++++++++++
> 3 files changed, 165 insertions(+), 10 deletions(-)
>
> --
> Marc-André Lureau
/Anton
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change
2017-06-01 7:12 ` Marc-André Lureau
@ 2017-06-01 11:24 ` Anton Nefedov
0 siblings, 0 replies; 30+ messages in thread
From: Anton Nefedov @ 2017-06-01 11:24 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: pbonzini, den, Dr . David Alan Gilbert
On 06/01/2017 10:12 AM, Marc-André Lureau wrote:
> Hi
>
> On Tue, May 30, 2017 at 6:08 PM Anton Nefedov
> <anton.nefedov@virtuozzo.com <mailto:anton.nefedov@virtuozzo.com>> wrote:
>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com
> <mailto:anton.nefedov@virtuozzo.com>>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com
> <mailto:vsementsov@virtuozzo.com>>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com
> <mailto:dgilbert@redhat.com>>
> ---
> chardev/char.c | 2 +-
> hmp-commands.hx | 16 ++++++++++++++++
> hmp.c | 34 ++++++++++++++++++++++++++++++++++
> hmp.h | 1 +
> include/sysemu/char.h | 12 ++++++++++++
> 5 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 2d6e204..1e2a2dd 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -888,7 +888,7 @@ 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 baeac47..19bfddd 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
>
>
> chardev_change vs chardev-change. Apparently, there is this difference
> for chardev-add too
>
Done.
> +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 20f5dab..7660495 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2224,6 +2224,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");
>
>
> enlighten me, how is this triggered?
>
correct variant:
chardev-change char2 pipe,path=/tmp/cent.fifo
vs. (without this check)
(qemu) chardev-change char2 pipe,id=char2,path=/tmp/cent.fifo
chardev-change char2 pipe,id=char2,path=/tmp/cent.fifo
Duplicate ID 'char2' for chardev
Parsing chardev args failed
or
(qemu) chardev-change char2 pipe,id=newchar3,path=/tmp/cent.fifo
the command would succeed but the user might expect the id to change,
but it wouldn't
> + 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 342f531..18fcd26 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -131,6 +131,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
> + *
>
>
> You may remove that line
>
> + * Returns: a new backend
>
>
> or NULL on error
>
Done.
> + */
> +ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
> + Error **errp);
> +
> +/**
> * @qemu_chr_new:
> *
> * Create a new character backend from a URI.
> --
> 2.7.4
>
>
> --
> Marc-André Lureau
/Anton
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change Anton Nefedov
2017-06-01 7:12 ` Marc-André Lureau
@ 2017-06-01 14:56 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 30+ messages in thread
From: Dr. David Alan Gilbert @ 2017-06-01 14:56 UTC (permalink / raw)
To: Anton Nefedov; +Cc: qemu-devel, den, pbonzini, marcandre.lureau
* Anton Nefedov (anton.nefedov@virtuozzo.com) wrote:
> 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>
Thanks, with Marc-Andre's comments, this looks fine from the HMP point.
If you can find a way to add a test to tests/test-hmp.c it would be
great.
Dave
> ---
> chardev/char.c | 2 +-
> hmp-commands.hx | 16 ++++++++++++++++
> hmp.c | 34 ++++++++++++++++++++++++++++++++++
> hmp.h | 1 +
> include/sysemu/char.h | 12 ++++++++++++
> 5 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 2d6e204..1e2a2dd 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -888,7 +888,7 @@ 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 baeac47..19bfddd 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 20f5dab..7660495 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2224,6 +2224,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 342f531..18fcd26 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -131,6 +131,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
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap
2017-06-01 11:23 ` Anton Nefedov
@ 2017-06-09 14:53 ` Marc-André Lureau
2017-06-13 11:53 ` Anton Nefedov
0 siblings, 1 reply; 30+ messages in thread
From: Marc-André Lureau @ 2017-06-09 14:53 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
On Thu, Jun 1, 2017 at 3:23 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:
>
>
> On 05/31/2017 10:20 PM, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, May 30, 2017 at 6:12 PM Anton Nefedov
> > <anton.nefedov@virtuozzo.com <mailto: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.
> >
> >
> > Tbh, I don't understand the need for a different lock. Could you
> > explain? Even better would be to write a test that shows in which way
> > the lock helps.
> >
>
> hi Marc-André,
>
> The existing chr_write_lock belongs to Chardev.
> For the hotswap case, we need to ensure that be->chr won't change and
> the old Chardev (together with its mutex) won't be destroyed while it's
> used in the write functions.
>
> Maybe we could move the lock to CharBackend, instead of creating a new
> one. But I guess this
> 1. won't work for mux, where multiple CharBackends share the same
> Chardev
> 2. won't work for some chardevs, like pty which uses the lock for the
> timer handler
>
> Sorry if I'm not explaining clearly enough or maybe I'm missing some
> easier solution?
>
>
It looks to me like you would get the same guarantees by using the
chr_write_lock directly:
@@ -1381,7 +1374,7 @@ ChardevReturn *qmp_chardev_change(const char *id,
ChardevBackend *backend,
closed_sent = true;
}
- qemu_mutex_lock(&be->chr_lock);
+ qemu_mutex_lock(&chr->chr_write_lock);
chr->be = NULL;
qemu_chr_fe_connect(be, chr_new, &error_abort);
@@ -1389,14 +1382,14 @@ ChardevReturn *qmp_chardev_change(const char *id,
ChardevBackend *backend,
error_setg(errp, "Chardev '%s' change failed", chr_new->label);
chr_new->be = NULL;
qemu_chr_fe_connect(be, chr, &error_abort);
- qemu_mutex_unlock(&be->chr_lock);
+ qemu_mutex_unlock(&chr->chr_write_lock);
if (closed_sent) {
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
}
object_unref(OBJECT(chr_new));
return NULL;
}
- qemu_mutex_unlock(&be->chr_lock);
+ qemu_mutex_unlock(&chr->chr_write_lock);
I wonder if we should rename 'chr_write_lock' to just 'lock' :)
> I can try to add a test but can't quite see yet how to freeze the old
> chardev somewhere in cc->chr_write() and hotswap it while it's there.
>
>
> >
> >
> > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com
> > <mailto:anton.nefedov@virtuozzo.com>>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com
> > <mailto:vsementsov@virtuozzo.com>>
> >
> >
> > patch looks good otherwise
> >
> > ---
> > chardev/char.c | 126
> > ++++++++++++++++++++++++++++++++++++++++++++++----
> > include/sysemu/char.h | 9 ++++
> > qapi-schema.json | 40 ++++++++++++++++
> > 3 files changed, 165 insertions(+), 10 deletions(-)
> >
> > --
> > Marc-André Lureau
>
> /Anton
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap
2017-06-09 14:53 ` Marc-André Lureau
@ 2017-06-13 11:53 ` Anton Nefedov
2017-06-13 12:32 ` Marc-André Lureau
0 siblings, 1 reply; 30+ messages in thread
From: Anton Nefedov @ 2017-06-13 11:53 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: pbonzini, den
On 06/09/2017 05:53 PM, Marc-André Lureau wrote:
>
>
> On Thu, Jun 1, 2017 at 3:23 PM Anton Nefedov
> <anton.nefedov@virtuozzo.com <mailto:anton.nefedov@virtuozzo.com>> wrote:
>
>
>
> On 05/31/2017 10:20 PM, Marc-André Lureau wrote:
> > Hi
> >
> > On Tue, May 30, 2017 at 6:12 PM Anton Nefedov
> > <anton.nefedov@virtuozzo.com <mailto:anton.nefedov@virtuozzo.com>
> <mailto:anton.nefedov@virtuozzo.com
> <mailto: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.
> >
> >
> > Tbh, I don't understand the need for a different lock. Could you
> > explain? Even better would be to write a test that shows in which way
> > the lock helps.
> >
>
> hi Marc-André,
>
> The existing chr_write_lock belongs to Chardev.
> For the hotswap case, we need to ensure that be->chr won't change and
> the old Chardev (together with its mutex) won't be destroyed while it's
> used in the write functions.
>
> Maybe we could move the lock to CharBackend, instead of creating a new
> one. But I guess this
> 1. won't work for mux, where multiple CharBackends share the same
> Chardev
> 2. won't work for some chardevs, like pty which uses the lock for the
> timer handler
>
> Sorry if I'm not explaining clearly enough or maybe I'm missing some
> easier solution?
>
>
> It looks to me like you would get the same guarantees by using the
> chr_write_lock directly:
>
> @@ -1381,7 +1374,7 @@ ChardevReturn *qmp_chardev_change(const char *id,
> ChardevBackend *backend,
> closed_sent = true;
> }
>
> - qemu_mutex_lock(&be->chr_lock);
> + qemu_mutex_lock(&chr->chr_write_lock);
> chr->be = NULL;
> qemu_chr_fe_connect(be, chr_new, &error_abort);
>
> @@ -1389,14 +1382,14 @@ ChardevReturn *qmp_chardev_change(const char
> *id, ChardevBackend *backend,
> error_setg(errp, "Chardev '%s' change failed", chr_new->label);
> chr_new->be = NULL;
> qemu_chr_fe_connect(be, chr, &error_abort);
> - qemu_mutex_unlock(&be->chr_lock);
> + qemu_mutex_unlock(&chr->chr_write_lock);
> if (closed_sent) {
> qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> }
> object_unref(OBJECT(chr_new));
> return NULL;
> }
> - qemu_mutex_unlock(&be->chr_lock);
> + qemu_mutex_unlock(&chr->chr_write_lock);
>
> I wonder if we should rename 'chr_write_lock' to just 'lock' :)
>
hi,
but isn't there a potential race?
Assume thread-1 is somewhere in qemu_chr_write() and thread-2 is in
qmp_chardev_change().
T2 can change the chardev, and destroy the old one while T1 still has
the pointer ( = use after free).
Or T1 unlocks chr_write_lock, T2 acquires that in
qemu_chr_write_buffer(), then T1 destroys it together
with the chardev ( = undefined behaviour).
What I'm trying to say is that the critical section for the hotswap is
bigger than what chr_write_lock currently covers - we also need to cover
the be->chr pointer.
Or am I missing something?
> I can try to add a test but can't quite see yet how to freeze the old
> chardev somewhere in cc->chr_write() and hotswap it while it's there.
>
>
> >
> >
> > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com
> <mailto:anton.nefedov@virtuozzo.com>
> > <mailto:anton.nefedov@virtuozzo.com
> <mailto:anton.nefedov@virtuozzo.com>>>
> > Reviewed-by: Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com <mailto:vsementsov@virtuozzo.com>
> > <mailto:vsementsov@virtuozzo.com
> <mailto:vsementsov@virtuozzo.com>>>
> >
> >
> > patch looks good otherwise
> >
> > ---
> > chardev/char.c | 126
> > ++++++++++++++++++++++++++++++++++++++++++++++----
> > include/sysemu/char.h | 9 ++++
> > qapi-schema.json | 40 ++++++++++++++++
> > 3 files changed, 165 insertions(+), 10 deletions(-)
> >
> > --
> > Marc-André Lureau
>
> /Anton
>
> --
> Marc-André Lureau
/Anton
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap
2017-06-13 11:53 ` Anton Nefedov
@ 2017-06-13 12:32 ` Marc-André Lureau
2017-06-15 14:03 ` Anton Nefedov
0 siblings, 1 reply; 30+ messages in thread
From: Marc-André Lureau @ 2017-06-13 12:32 UTC (permalink / raw)
To: Anton Nefedov, qemu-devel; +Cc: pbonzini, den
Hi
On Tue, Jun 13, 2017 at 3:53 PM Anton Nefedov <anton.nefedov@virtuozzo.com>
wrote:
> > The existing chr_write_lock belongs to Chardev.
> > For the hotswap case, we need to ensure that be->chr won't change and
> > the old Chardev (together with its mutex) won't be destroyed while
> it's
> > used in the write functions.
> >
> > Maybe we could move the lock to CharBackend, instead of creating a
> new
> > one. But I guess this
> > 1. won't work for mux, where multiple CharBackends share the same
> > Chardev
> > 2. won't work for some chardevs, like pty which uses the lock for
> the
> > timer handler
> >
> > Sorry if I'm not explaining clearly enough or maybe I'm missing some
> > easier solution?
> >
> >
> > It looks to me like you would get the same guarantees by using the
> > chr_write_lock directly:
> >
> > @@ -1381,7 +1374,7 @@ ChardevReturn *qmp_chardev_change(const char *id,
> > ChardevBackend *backend,
> > closed_sent = true;
> > }
> >
> > - qemu_mutex_lock(&be->chr_lock);
> > + qemu_mutex_lock(&chr->chr_write_lock);
> > chr->be = NULL;
> > qemu_chr_fe_connect(be, chr_new, &error_abort);
> >
> > @@ -1389,14 +1382,14 @@ ChardevReturn *qmp_chardev_change(const char
> > *id, ChardevBackend *backend,
> > error_setg(errp, "Chardev '%s' change failed", chr_new->label);
> > chr_new->be = NULL;
> > qemu_chr_fe_connect(be, chr, &error_abort);
> > - qemu_mutex_unlock(&be->chr_lock);
> > + qemu_mutex_unlock(&chr->chr_write_lock);
> > if (closed_sent) {
> > qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> > }
> > object_unref(OBJECT(chr_new));
> > return NULL;
> > }
> > - qemu_mutex_unlock(&be->chr_lock);
> > + qemu_mutex_unlock(&chr->chr_write_lock);
> >
> > I wonder if we should rename 'chr_write_lock' to just 'lock' :)
> >
>
> hi,
>
> but isn't there a potential race?
>
> Assume thread-1 is somewhere in qemu_chr_write() and thread-2 is in
> qmp_chardev_change().
>
> T2 can change the chardev, and destroy the old one while T1 still has
> the pointer ( = use after free).
> Or T1 unlocks chr_write_lock, T2 acquires that in
> qemu_chr_write_buffer(), then T1 destroys it together
> with the chardev ( = undefined behaviour).
>
> What I'm trying to say is that the critical section for the hotswap is
> bigger than what chr_write_lock currently covers - we also need to cover
> the be->chr pointer.
> Or am I missing something?
>
Thanks for the detail, I think you are correct, and probably your solution
is good. Another way would be to object_ref/unref the Chardev when using it
in seperate thread, this way we wouldn't need an extra lock, I think. Paolo
might be of good advice to get this right.
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap
2017-06-13 12:32 ` Marc-André Lureau
@ 2017-06-15 14:03 ` Anton Nefedov
0 siblings, 0 replies; 30+ messages in thread
From: Anton Nefedov @ 2017-06-15 14:03 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: pbonzini, den
On 06/13/2017 03:32 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Jun 13, 2017 at 3:53 PM Anton Nefedov
> <anton.nefedov@virtuozzo.com <mailto:anton.nefedov@virtuozzo.com>> wrote:
>
> > The existing chr_write_lock belongs to Chardev.
> > For the hotswap case, we need to ensure that be->chr won't
> change and
> > the old Chardev (together with its mutex) won't be destroyed
> while it's
> > used in the write functions.
> >
> > Maybe we could move the lock to CharBackend, instead of
> creating a new
> > one. But I guess this
> > 1. won't work for mux, where multiple CharBackends share
> the same
> > Chardev
> > 2. won't work for some chardevs, like pty which uses the
> lock for the
> > timer handler
> >
> > Sorry if I'm not explaining clearly enough or maybe I'm
> missing some
> > easier solution?
> >
> >
> > It looks to me like you would get the same guarantees by using the
> > chr_write_lock directly:
> >
> > @@ -1381,7 +1374,7 @@ ChardevReturn *qmp_chardev_change(const
> char *id,
> > ChardevBackend *backend,
> > closed_sent = true;
> > }
> >
> > - qemu_mutex_lock(&be->chr_lock);
> > + qemu_mutex_lock(&chr->chr_write_lock);
> > chr->be = NULL;
> > qemu_chr_fe_connect(be, chr_new, &error_abort);
> >
> > @@ -1389,14 +1382,14 @@ ChardevReturn *qmp_chardev_change(const char
> > *id, ChardevBackend *backend,
> > error_setg(errp, "Chardev '%s' change failed",
> chr_new->label);
> > chr_new->be = NULL;
> > qemu_chr_fe_connect(be, chr, &error_abort);
> > - qemu_mutex_unlock(&be->chr_lock);
> > + qemu_mutex_unlock(&chr->chr_write_lock);
> > if (closed_sent) {
> > qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> > }
> > object_unref(OBJECT(chr_new));
> > return NULL;
> > }
> > - qemu_mutex_unlock(&be->chr_lock);
> > + qemu_mutex_unlock(&chr->chr_write_lock);
> >
> > I wonder if we should rename 'chr_write_lock' to just 'lock' :)
> >
>
> hi,
>
> but isn't there a potential race?
>
> Assume thread-1 is somewhere in qemu_chr_write() and thread-2 is in
> qmp_chardev_change().
>
> T2 can change the chardev, and destroy the old one while T1 still has
> the pointer ( = use after free).
> Or T1 unlocks chr_write_lock, T2 acquires that in
> qemu_chr_write_buffer(), then T1 destroys it together
> with the chardev ( = undefined behaviour).
>
>
> What I'm trying to say is that the critical section for the hotswap is
> bigger than what chr_write_lock currently covers - we also need to cover
> the be->chr pointer.
> Or am I missing something?
>
>
> Thanks for the detail, I think you are correct, and probably your
> solution is good. Another way would be to object_ref/unref the Chardev
> when using it in seperate thread, this way we wouldn't need an extra
> lock, I think. Paolo might be of good advice to get this right.
>
We also need to protect be->chr pointer, not just the Chardev itself, so
probably ref/unref won't be thread-safe, strictly speaking?
e.g.
fe_write(CharBackend *be) {
ref(be->chr);
...
unref(be->chr); // oops, be->chr might have changed
}
or
fe_write(CharBackend *be) {
Chardev *chr = be->chr;
ref(chr); // not good either, chr might be destroyed already
...
unref(chr);
}
>
> --
> Marc-André Lureau
/Anton
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2017-06-15 14:03 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-30 13:57 [Qemu-devel] [PATCH v3 00/13] chardevice hotswap Anton Nefedov
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 01/13] char: move QemuOpts->ChardevBackend translation to a separate func Anton Nefedov
2017-05-31 19:18 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 02/13] char: add backend hotswap handler Anton Nefedov
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 03/13] char: chardevice hotswap Anton Nefedov
2017-05-31 19:20 ` Marc-André Lureau
2017-06-01 11:23 ` Anton Nefedov
2017-06-09 14:53 ` Marc-André Lureau
2017-06-13 11:53 ` Anton Nefedov
2017-06-13 12:32 ` Marc-André Lureau
2017-06-15 14:03 ` Anton Nefedov
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 04/13] char: forbid direct chardevice access for hotswap devices Anton Nefedov
2017-05-31 19:21 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 05/13] char: avoid chardevice direct access Anton Nefedov
2017-05-31 19:21 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 06/13] test-char: unref chardev-udp after test Anton Nefedov
2017-05-31 19:22 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 07/13] test-char: split char_udp_test Anton Nefedov
2017-05-31 19:22 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 08/13] test-char: split char_file_test Anton Nefedov
2017-05-31 19:24 ` Marc-André Lureau
2017-05-30 13:57 ` [Qemu-devel] [PATCH v3 09/13] test-char: add hotswap test Anton Nefedov
2017-05-31 19:25 ` Marc-André Lureau
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change Anton Nefedov
2017-06-01 7:12 ` Marc-André Lureau
2017-06-01 11:24 ` Anton Nefedov
2017-06-01 14:56 ` Dr. David Alan Gilbert
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 11/13] virtio-console: chardev hotswap support Anton Nefedov
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 12/13] serial: move TIOCM update to a separate function Anton Nefedov
2017-05-30 13:58 ` [Qemu-devel] [PATCH v3 13/13] serial: chardev hotswap support Anton Nefedov
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.