* [Qemu-devel] [PATCH] chardev-frontends: Explicitly check, inc and dec avail_connections
@ 2013-03-27 14:10 Hans de Goede
2013-03-27 15:11 ` Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2013-03-27 14:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Hans de Goede
chardev-frontends need to explictly check, increase and decrement the
avail_connections "property" of the chardev when they are not using a
qdev-chardev-property for the chardev.
This fixes things like:
qemu-kvm -chardev stdio,id=foo -device isa-serial,chardev=foo \
-mon chardev=foo
Working, where they should fail. Most of the changes here are due to
old hardware emulation code which is using serial_hds directly rather then
a qdev-chardev-property.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
backends/rng-egd.c | 7 +++++++
gdbstub.c | 1 +
hw/arm/pxa2xx.c | 9 ++++++++-
hw/bt-hci-csr.c | 1 +
hw/ipoctal232.c | 1 +
hw/ivshmem.c | 1 +
hw/mcf_uart.c | 6 ++++++
hw/serial.c | 16 ++++++++++++++++
hw/serial.h | 1 +
hw/sh_serial.c | 9 ++++++++-
hw/xen_console.c | 19 +++++++++++++++----
net/slirp.c | 1 +
qemu-char.c | 14 +++++++++++++-
vl.c | 7 +++++++
14 files changed, 86 insertions(+), 7 deletions(-)
diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 5e012e9..d8e9d63 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -149,6 +149,12 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
return;
}
+ if (s->chr->avail_connections < 1) {
+ error_set(errp, QERR_DEVICE_IN_USE, s->chr_name);
+ return;
+ }
+ s->chr->avail_connections--;
+
/* FIXME we should resubmit pending requests when the CDS reconnects. */
qemu_chr_add_handlers(s->chr, rng_egd_chr_can_read, rng_egd_chr_read,
NULL, s);
@@ -191,6 +197,7 @@ static void rng_egd_finalize(Object *obj)
if (s->chr) {
qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
+ s->chr->avail_connections++;
}
g_free(s->chr_name);
diff --git a/gdbstub.c b/gdbstub.c
index a666cb5..83267e0 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -3025,6 +3025,7 @@ int gdbserver_start(const char *device)
if (!chr)
return -1;
+ chr->avail_connections--;
qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
gdb_chr_event, NULL);
}
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 7467cca..df4b458 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1981,9 +1981,16 @@ static PXA2xxFIrState *pxa2xx_fir_init(MemoryRegion *sysmem,
memory_region_init_io(&s->iomem, &pxa2xx_fir_ops, s, "pxa2xx-fir", 0x1000);
memory_region_add_subregion(sysmem, base, &s->iomem);
- if (chr)
+ if (chr) {
+ if (chr->avail_connections < 1) {
+ fprintf(stderr, "pxa2xx_fir_init error chardev %s already used\n",
+ chr->label);
+ exit(1);
+ }
+ chr->avail_connections--;
qemu_chr_add_handlers(chr, pxa2xx_fir_is_empty,
pxa2xx_fir_rx, pxa2xx_fir_event, s);
+ }
register_savevm(NULL, "pxa2xx_fir", 0, 0, pxa2xx_fir_save,
pxa2xx_fir_load, s);
diff --git a/hw/bt-hci-csr.c b/hw/bt-hci-csr.c
index e4ada3c..55c819b 100644
--- a/hw/bt-hci-csr.c
+++ b/hw/bt-hci-csr.c
@@ -439,6 +439,7 @@ CharDriverState *uart_hci_init(qemu_irq wakeup)
s->chr.opaque = s;
s->chr.chr_write = csrhci_write;
s->chr.chr_ioctl = csrhci_ioctl;
+ s->chr.avail_connections = 1;
s->hci = qemu_next_hci();
s->hci->opaque = s;
diff --git a/hw/ipoctal232.c b/hw/ipoctal232.c
index 1da6a99..f93ad5c 100644
--- a/hw/ipoctal232.c
+++ b/hw/ipoctal232.c
@@ -556,6 +556,7 @@ static int ipoctal_init(IPackDevice *ip)
if (ch->dev) {
index++;
+ ch->dev->avail_connections--;
qemu_chr_add_handlers(ch->dev, hostdev_can_receive,
hostdev_receive, hostdev_event, ch);
DPRINTF("Redirecting channel %u to %s (%s)\n",
diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 68a2cf2..82d34b7 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -292,6 +292,7 @@ static CharDriverState* create_eventfd_chr_device(void * opaque, EventNotifier *
fprintf(stderr, "creating eventfd for eventfd %d failed\n", eventfd);
exit(-1);
}
+ chr->avail_connections--;
/* if MSI is supported we need multiple interrupts */
if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
diff --git a/hw/mcf_uart.c b/hw/mcf_uart.c
index aacf0f0..079e776 100644
--- a/hw/mcf_uart.c
+++ b/hw/mcf_uart.c
@@ -280,6 +280,12 @@ void *mcf_uart_init(qemu_irq irq, CharDriverState *chr)
s->chr = chr;
s->irq = irq;
if (chr) {
+ if (chr->avail_connections < 1) {
+ fprintf(stderr, "mcf_uart_init error chardev %s already used\n",
+ chr->label);
+ exit(1);
+ }
+ chr->avail_connections--;
qemu_chr_add_handlers(chr, mcf_uart_can_receive, mcf_uart_receive,
mcf_uart_event, s);
}
diff --git a/hw/serial.c b/hw/serial.c
index 0ccc499..4e342fd 100644
--- a/hw/serial.c
+++ b/hw/serial.c
@@ -676,6 +676,15 @@ void serial_init_core(SerialState *s)
fprintf(stderr, "Can't create serial device, empty char device\n");
exit(1);
}
+ if (s->chr_owned_by_serial_core) {
+ if (s->chr->avail_connections < 1) {
+ fprintf(stderr,
+ "Can't create serial device, char device \"%s\" in use\n",
+ s->chr->label);
+ exit(1);
+ }
+ s->chr->avail_connections--;
+ }
s->modem_status_poll = qemu_new_timer_ns(vm_clock, (QEMUTimerCB *) serial_update_msl, s);
@@ -689,6 +698,9 @@ void serial_init_core(SerialState *s)
void serial_exit_core(SerialState *s)
{
qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
+ if (s->chr_owned_by_serial_core) {
+ s->chr->avail_connections++;
+ }
qemu_unregister_reset(serial_reset, s);
}
@@ -719,6 +731,8 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase,
s->irq = irq;
s->baudbase = baudbase;
s->chr = chr;
+ /* We always get called with chr an entry of serial_hds */
+ s->chr_owned_by_serial_core = 1;
serial_init_core(s);
vmstate_register(NULL, base, &vmstate_serial, s);
@@ -776,6 +790,8 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
s->irq = irq;
s->baudbase = baudbase;
s->chr = chr;
+ /* We always get called with chr an entry of serial_hds */
+ s->chr_owned_by_serial_core = 1;
serial_init_core(s);
vmstate_register(NULL, base, &vmstate_serial, s);
diff --git a/hw/serial.h b/hw/serial.h
index e884499..7703881 100644
--- a/hw/serial.h
+++ b/hw/serial.h
@@ -59,6 +59,7 @@ struct SerialState {
int thr_ipending;
qemu_irq irq;
CharDriverState *chr;
+ int chr_owned_by_serial_core;
int last_break_enable;
int it_shift;
int baudbase;
diff --git a/hw/sh_serial.c b/hw/sh_serial.c
index 40e797c..fb5e542 100644
--- a/hw/sh_serial.c
+++ b/hw/sh_serial.c
@@ -396,9 +396,16 @@ void sh_serial_init(MemoryRegion *sysmem,
s->chr = chr;
- if (chr)
+ if (chr) {
+ if (chr->avail_connections < 1) {
+ fprintf(stderr, "sh_serial_init error chardev %s already used\n",
+ chr->label);
+ exit(1);
+ }
+ chr->avail_connections--;
qemu_chr_add_handlers(chr, sh_serial_can_receive1, sh_serial_receive1,
sh_serial_event, s);
+ }
s->eri = eri_source;
s->rxi = rxi_source;
diff --git a/hw/xen_console.c b/hw/xen_console.c
index a8db6f8..e8e1038 100644
--- a/hw/xen_console.c
+++ b/hw/xen_console.c
@@ -241,9 +241,18 @@ static int con_initialise(struct XenDevice *xendev)
return -1;
xen_be_bind_evtchn(&con->xendev);
- if (con->chr)
- qemu_chr_add_handlers(con->chr, xencons_can_receive, xencons_receive,
- NULL, con);
+ if (con->chr) {
+ if (con->chr->avail_connections >= 1) {
+ qemu_chr_add_handlers(con->chr, xencons_can_receive,
+ xencons_receive, NULL, con);
+ con->chr->avail_connections--;
+ } else {
+ xen_be_printf(xendev, 0,
+ "xen_console_init error chardev %s already used\n",
+ con->chr->label);
+ con->chr = NULL;
+ }
+ }
xen_be_printf(xendev, 1, "ring mfn %d, remote port %d, local port %d, limit %zd\n",
con->ring_ref,
@@ -260,8 +269,10 @@ static void con_disconnect(struct XenDevice *xendev)
if (!xendev->dev) {
return;
}
- if (con->chr)
+ if (con->chr) {
qemu_chr_add_handlers(con->chr, NULL, NULL, NULL, NULL);
+ con->chr->avail_connections++;
+ }
xen_be_unbind_evtchn(&con->xendev);
if (con->sring) {
diff --git a/net/slirp.c b/net/slirp.c
index 4df550f..76c700b 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -649,6 +649,7 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
g_free(fwd);
return -1;
}
+ fwd->hd->avail_connections--;
if (slirp_add_exec(s->slirp, 3, fwd->hd, &server, port) < 0) {
error_report("conflicting/invalid host:port in guest forwarding "
diff --git a/qemu-char.c b/qemu-char.c
index edf3779..e49f1ac 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3377,6 +3377,7 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
error_free(err);
}
if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
+ chr->avail_connections--;
monitor_init(chr, MONITOR_USE_READLINE);
}
return chr;
@@ -3466,9 +3467,20 @@ CharDriverState *qemu_chr_find(const char *name)
CharDriverState *qemu_char_get_next_serial(void)
{
static int next_serial;
+ CharDriverState *chr;
/* FIXME: This function needs to go away: use chardev properties! */
- return serial_hds[next_serial++];
+
+ while (next_serial < MAX_SERIAL_PORTS && serial_hds[next_serial]) {
+ chr = serial_hds[next_serial++];
+ /* Skip already used chardevs */
+ if (chr->avail_connections < 1) {
+ continue;
+ }
+ chr->avail_connections--;
+ return chr;
+ }
+ return NULL;
}
QemuOptsList qemu_chardev_opts = {
diff --git a/vl.c b/vl.c
index aeed7f4..0f1c967 100644
--- a/vl.c
+++ b/vl.c
@@ -2391,6 +2391,13 @@ static int mon_init_func(QemuOpts *opts, void *opaque)
exit(1);
}
+ if (chr->avail_connections < 1) {
+ fprintf(stderr, "monitor init error chardev \"%s\" already used\n",
+ chardev);
+ exit(1);
+ }
+ chr->avail_connections--;
+
monitor_init(chr, flags);
return 0;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] chardev-frontends: Explicitly check, inc and dec avail_connections
2013-03-27 14:10 [Qemu-devel] [PATCH] chardev-frontends: Explicitly check, inc and dec avail_connections Hans de Goede
@ 2013-03-27 15:11 ` Paolo Bonzini
2013-03-27 15:36 ` Hans de Goede
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2013-03-27 15:11 UTC (permalink / raw)
To: Hans de Goede; +Cc: qemu-devel
Il 27/03/2013 15:10, Hans de Goede ha scritto:
> chardev-frontends need to explictly check, increase and decrement the
> avail_connections "property" of the chardev when they are not using a
> qdev-chardev-property for the chardev.
>
> This fixes things like:
> qemu-kvm -chardev stdio,id=foo -device isa-serial,chardev=foo \
> -mon chardev=foo
>
> Working, where they should fail. Most of the changes here are due to
> old hardware emulation code which is using serial_hds directly rather then
> a qdev-chardev-property.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> backends/rng-egd.c | 7 +++++++
> gdbstub.c | 1 +
> hw/arm/pxa2xx.c | 9 ++++++++-
> hw/bt-hci-csr.c | 1 +
> hw/ipoctal232.c | 1 +
> hw/ivshmem.c | 1 +
> hw/mcf_uart.c | 6 ++++++
> hw/serial.c | 16 ++++++++++++++++
> hw/serial.h | 1 +
> hw/sh_serial.c | 9 ++++++++-
> hw/xen_console.c | 19 +++++++++++++++----
> net/slirp.c | 1 +
> qemu-char.c | 14 +++++++++++++-
> vl.c | 7 +++++++
> 14 files changed, 86 insertions(+), 7 deletions(-)
>
> diff --git a/backends/rng-egd.c b/backends/rng-egd.c
> index 5e012e9..d8e9d63 100644
> --- a/backends/rng-egd.c
> +++ b/backends/rng-egd.c
> @@ -149,6 +149,12 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
> return;
> }
>
> + if (s->chr->avail_connections < 1) {
> + error_set(errp, QERR_DEVICE_IN_USE, s->chr_name);
> + return;
> + }
> + s->chr->avail_connections--;
> +
> /* FIXME we should resubmit pending requests when the CDS reconnects. */
> qemu_chr_add_handlers(s->chr, rng_egd_chr_can_read, rng_egd_chr_read,
> NULL, s);
> @@ -191,6 +197,7 @@ static void rng_egd_finalize(Object *obj)
>
> if (s->chr) {
> qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
> + s->chr->avail_connections++;
> }
>
> g_free(s->chr_name);
Ok, but please create wrappers for these (e.g. qemu_chr_be_start/stop
and qemu_chr_be_start_nofail) and use them throughout.
> diff --git a/gdbstub.c b/gdbstub.c
> index a666cb5..83267e0 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -3025,6 +3025,7 @@ int gdbserver_start(const char *device)
> if (!chr)
> return -1;
>
> + chr->avail_connections--;
> qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
> gdb_chr_event, NULL);
> }
Ok.
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index 7467cca..df4b458 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -1981,9 +1981,16 @@ static PXA2xxFIrState *pxa2xx_fir_init(MemoryRegion *sysmem,
> memory_region_init_io(&s->iomem, &pxa2xx_fir_ops, s, "pxa2xx-fir", 0x1000);
> memory_region_add_subregion(sysmem, base, &s->iomem);
>
> - if (chr)
> + if (chr) {
> + if (chr->avail_connections < 1) {
> + fprintf(stderr, "pxa2xx_fir_init error chardev %s already used\n",
> + chr->label);
> + exit(1);
> + }
> + chr->avail_connections--;
> qemu_chr_add_handlers(chr, pxa2xx_fir_is_empty,
> pxa2xx_fir_rx, pxa2xx_fir_event, s);
> + }
>
> register_savevm(NULL, "pxa2xx_fir", 0, 0, pxa2xx_fir_save,
> pxa2xx_fir_load, s);
Errors won't be reported, because serial_hds[] will always create its
own CharDriverState and avail_connections will always be 1. Use a
wrapper and the code can ignore this.
> diff --git a/hw/bt-hci-csr.c b/hw/bt-hci-csr.c
> index e4ada3c..55c819b 100644
> --- a/hw/bt-hci-csr.c
> +++ b/hw/bt-hci-csr.c
> @@ -439,6 +439,7 @@ CharDriverState *uart_hci_init(qemu_irq wakeup)
> s->chr.opaque = s;
> s->chr.chr_write = csrhci_write;
> s->chr.chr_ioctl = csrhci_ioctl;
> + s->chr.avail_connections = 1;
>
> s->hci = qemu_next_hci();
> s->hci->opaque = s;
Ok.
> diff --git a/hw/ipoctal232.c b/hw/ipoctal232.c
> index 1da6a99..f93ad5c 100644
> --- a/hw/ipoctal232.c
> +++ b/hw/ipoctal232.c
> @@ -556,6 +556,7 @@ static int ipoctal_init(IPackDevice *ip)
>
> if (ch->dev) {
> index++;
> + ch->dev->avail_connections--;
> qemu_chr_add_handlers(ch->dev, hostdev_can_receive,
> hostdev_receive, hostdev_event, ch);
> DPRINTF("Redirecting channel %u to %s (%s)\n",
Ouch. WTF was I thinking when I reviewed this? :) Please change this
to use DEFINE_PROP_CHARDEV. I don't really care if it is
backwards-incompatible.
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 68a2cf2..82d34b7 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -292,6 +292,7 @@ static CharDriverState* create_eventfd_chr_device(void * opaque, EventNotifier *
> fprintf(stderr, "creating eventfd for eventfd %d failed\n", eventfd);
> exit(-1);
> }
> + chr->avail_connections--;
>
> /* if MSI is supported we need multiple interrupts */
> if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
Ok.
> diff --git a/hw/mcf_uart.c b/hw/mcf_uart.c
> index aacf0f0..079e776 100644
> --- a/hw/mcf_uart.c
> +++ b/hw/mcf_uart.c
> @@ -280,6 +280,12 @@ void *mcf_uart_init(qemu_irq irq, CharDriverState *chr)
> s->chr = chr;
> s->irq = irq;
> if (chr) {
> + if (chr->avail_connections < 1) {
> + fprintf(stderr, "mcf_uart_init error chardev %s already used\n",
> + chr->label);
> + exit(1);
> + }
> + chr->avail_connections--;
> qemu_chr_add_handlers(chr, mcf_uart_can_receive, mcf_uart_receive,
> mcf_uart_event, s);
> }
Ok.
> diff --git a/hw/serial.c b/hw/serial.c
> index 0ccc499..4e342fd 100644
> --- a/hw/serial.c
> +++ b/hw/serial.c
> @@ -676,6 +676,15 @@ void serial_init_core(SerialState *s)
> fprintf(stderr, "Can't create serial device, empty char device\n");
> exit(1);
> }
> + if (s->chr_owned_by_serial_core) {
> + if (s->chr->avail_connections < 1) {
> + fprintf(stderr,
> + "Can't create serial device, char device \"%s\" in use\n",
> + s->chr->label);
> + exit(1);
> + }
> + s->chr->avail_connections--;
> + }
>
> s->modem_status_poll = qemu_new_timer_ns(vm_clock, (QEMUTimerCB *) serial_update_msl, s);
>
> @@ -689,6 +698,9 @@ void serial_init_core(SerialState *s)
> void serial_exit_core(SerialState *s)
> {
> qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
> + if (s->chr_owned_by_serial_core) {
> + s->chr->avail_connections++;
> + }
> qemu_unregister_reset(serial_reset, s);
> }
>
> @@ -719,6 +731,8 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase,
> s->irq = irq;
> s->baudbase = baudbase;
> s->chr = chr;
> + /* We always get called with chr an entry of serial_hds */
> + s->chr_owned_by_serial_core = 1;
> serial_init_core(s);
>
> vmstate_register(NULL, base, &vmstate_serial, s);
> @@ -776,6 +790,8 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
> s->irq = irq;
> s->baudbase = baudbase;
> s->chr = chr;
> + /* We always get called with chr an entry of serial_hds */
> + s->chr_owned_by_serial_core = 1;
>
> serial_init_core(s);
> vmstate_register(NULL, base, &vmstate_serial, s);
> diff --git a/hw/serial.h b/hw/serial.h
> index e884499..7703881 100644
> --- a/hw/serial.h
> +++ b/hw/serial.h
> @@ -59,6 +59,7 @@ struct SerialState {
> int thr_ipending;
> qemu_irq irq;
> CharDriverState *chr;
> + int chr_owned_by_serial_core;
> int last_break_enable;
> int it_shift;
> int baudbase;
Please leave these aside. It is better to QOM-ify SerialState, I'll put
it on my list...
> diff --git a/hw/sh_serial.c b/hw/sh_serial.c
> index 40e797c..fb5e542 100644
> --- a/hw/sh_serial.c
> +++ b/hw/sh_serial.c
> @@ -396,9 +396,16 @@ void sh_serial_init(MemoryRegion *sysmem,
>
> s->chr = chr;
>
> - if (chr)
> + if (chr) {
> + if (chr->avail_connections < 1) {
> + fprintf(stderr, "sh_serial_init error chardev %s already used\n",
> + chr->label);
> + exit(1);
> + }
> + chr->avail_connections--;
> qemu_chr_add_handlers(chr, sh_serial_can_receive1, sh_serial_receive1,
> sh_serial_event, s);
> + }
>
> s->eri = eri_source;
> s->rxi = rxi_source;
> diff --git a/hw/xen_console.c b/hw/xen_console.c
> index a8db6f8..e8e1038 100644
> --- a/hw/xen_console.c
> +++ b/hw/xen_console.c
> @@ -241,9 +241,18 @@ static int con_initialise(struct XenDevice *xendev)
> return -1;
>
> xen_be_bind_evtchn(&con->xendev);
> - if (con->chr)
> - qemu_chr_add_handlers(con->chr, xencons_can_receive, xencons_receive,
> - NULL, con);
> + if (con->chr) {
> + if (con->chr->avail_connections >= 1) {
> + qemu_chr_add_handlers(con->chr, xencons_can_receive,
> + xencons_receive, NULL, con);
> + con->chr->avail_connections--;
> + } else {
> + xen_be_printf(xendev, 0,
> + "xen_console_init error chardev %s already used\n",
> + con->chr->label);
> + con->chr = NULL;
> + }
> + }
>
> xen_be_printf(xendev, 1, "ring mfn %d, remote port %d, local port %d, limit %zd\n",
> con->ring_ref,
> @@ -260,8 +269,10 @@ static void con_disconnect(struct XenDevice *xendev)
> if (!xendev->dev) {
> return;
> }
> - if (con->chr)
> + if (con->chr) {
> qemu_chr_add_handlers(con->chr, NULL, NULL, NULL, NULL);
> + con->chr->avail_connections++;
> + }
> xen_be_unbind_evtchn(&con->xendev);
>
> if (con->sring) {
> diff --git a/net/slirp.c b/net/slirp.c
> index 4df550f..76c700b 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -649,6 +649,7 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
> g_free(fwd);
> return -1;
> }
> + fwd->hd->avail_connections--;
>
> if (slirp_add_exec(s->slirp, 3, fwd->hd, &server, port) < 0) {
> error_report("conflicting/invalid host:port in guest forwarding "
> diff --git a/qemu-char.c b/qemu-char.c
> index edf3779..e49f1ac 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3377,6 +3377,7 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
> error_free(err);
> }
> if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
> + chr->avail_connections--;
> monitor_init(chr, MONITOR_USE_READLINE);
> }
> return chr;
> @@ -3466,9 +3467,20 @@ CharDriverState *qemu_chr_find(const char *name)
> CharDriverState *qemu_char_get_next_serial(void)
> {
> static int next_serial;
> + CharDriverState *chr;
>
> /* FIXME: This function needs to go away: use chardev properties! */
> - return serial_hds[next_serial++];
> +
> + while (next_serial < MAX_SERIAL_PORTS && serial_hds[next_serial]) {
> + chr = serial_hds[next_serial++];
> + /* Skip already used chardevs */
> + if (chr->avail_connections < 1) {
> + continue;
> + }
> + chr->avail_connections--;
> + return chr;
> + }
> + return NULL;
> }
>
> QemuOptsList qemu_chardev_opts = {
> diff --git a/vl.c b/vl.c
> index aeed7f4..0f1c967 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2391,6 +2391,13 @@ static int mon_init_func(QemuOpts *opts, void *opaque)
> exit(1);
> }
>
> + if (chr->avail_connections < 1) {
> + fprintf(stderr, "monitor init error chardev \"%s\" already used\n",
> + chardev);
> + exit(1);
> + }
> + chr->avail_connections--;
> +
> monitor_init(chr, flags);
> return 0;
> }
>
Ok.
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] chardev-frontends: Explicitly check, inc and dec avail_connections
2013-03-27 15:11 ` Paolo Bonzini
@ 2013-03-27 15:36 ` Hans de Goede
2013-03-27 15:37 ` Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2013-03-27 15:36 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
Hi,
On 03/27/2013 04:11 PM, Paolo Bonzini wrote:
<snip>
>> diff --git a/backends/rng-egd.c b/backends/rng-egd.c
>> index 5e012e9..d8e9d63 100644
>> --- a/backends/rng-egd.c
>> +++ b/backends/rng-egd.c
>> @@ -149,6 +149,12 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
>> return;
>> }
>>
>> + if (s->chr->avail_connections < 1) {
>> + error_set(errp, QERR_DEVICE_IN_USE, s->chr_name);
>> + return;
>> + }
>> + s->chr->avail_connections--;
>> +
>> /* FIXME we should resubmit pending requests when the CDS reconnects. */
>> qemu_chr_add_handlers(s->chr, rng_egd_chr_can_read, rng_egd_chr_read,
>> NULL, s);
>> @@ -191,6 +197,7 @@ static void rng_egd_finalize(Object *obj)
>>
>> if (s->chr) {
>> qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
>> + s->chr->avail_connections++;
>> }
>>
>> g_free(s->chr_name);
>
> Ok, but please create wrappers for these (e.g. qemu_chr_be_start/stop
> and qemu_chr_be_start_nofail) and use them throughout.
That would be fe_start fe_stop, ack otherwise.
>> diff --git a/gdbstub.c b/gdbstub.c
>> index a666cb5..83267e0 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -3025,6 +3025,7 @@ int gdbserver_start(const char *device)
>> if (!chr)
>> return -1;
>>
>> + chr->avail_connections--;
>> qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
>> gdb_chr_event, NULL);
>> }
>
> Ok.
>
>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
>> index 7467cca..df4b458 100644
>> --- a/hw/arm/pxa2xx.c
>> +++ b/hw/arm/pxa2xx.c
>> @@ -1981,9 +1981,16 @@ static PXA2xxFIrState *pxa2xx_fir_init(MemoryRegion *sysmem,
>> memory_region_init_io(&s->iomem, &pxa2xx_fir_ops, s, "pxa2xx-fir", 0x1000);
>> memory_region_add_subregion(sysmem, base, &s->iomem);
>>
>> - if (chr)
>> + if (chr) {
>> + if (chr->avail_connections < 1) {
>> + fprintf(stderr, "pxa2xx_fir_init error chardev %s already used\n",
>> + chr->label);
>> + exit(1);
>> + }
>> + chr->avail_connections--;
>> qemu_chr_add_handlers(chr, pxa2xx_fir_is_empty,
>> pxa2xx_fir_rx, pxa2xx_fir_event, s);
>> + }
>>
>> register_savevm(NULL, "pxa2xx_fir", 0, 0, pxa2xx_fir_save,
>> pxa2xx_fir_load, s);
>
> Errors won't be reported, because serial_hds[] will always create its
> own CharDriverState and avail_connections will always be 1. Use a
> wrapper and the code can ignore this.
Unless some smartass adds, ie: -mon chardev=serial0 to the cmdline, then an
error will be reported.
I'll respin the patch taking your comments into account.
Regards,
Hans
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-03-27 15:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-27 14:10 [Qemu-devel] [PATCH] chardev-frontends: Explicitly check, inc and dec avail_connections Hans de Goede
2013-03-27 15:11 ` Paolo Bonzini
2013-03-27 15:36 ` Hans de Goede
2013-03-27 15:37 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).