From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43792) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKs0b-0007iW-5z for qemu-devel@nongnu.org; Wed, 27 Mar 2013 11:11:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UKs0Z-0002EY-4k for qemu-devel@nongnu.org; Wed, 27 Mar 2013 11:11:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25599) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UKs0Y-0002ER-Sy for qemu-devel@nongnu.org; Wed, 27 Mar 2013 11:11:23 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2RFBM23006857 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 27 Mar 2013 11:11:22 -0400 Message-ID: <51530C15.2090901@redhat.com> Date: Wed, 27 Mar 2013 16:11:17 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1364393440-6054-1-git-send-email-hdegoede@redhat.com> In-Reply-To: <1364393440-6054-1-git-send-email-hdegoede@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] chardev-frontends: Explicitly check, inc and dec avail_connections List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hans de Goede Cc: qemu-devel@nongnu.org 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 > --- > 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