* [Qemu-devel] [PATCH 1/8] qemu-char: Rename opened to be_open
2013-03-24 12:39 [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Hans de Goede
@ 2013-03-24 12:39 ` Hans de Goede
2013-03-24 12:39 ` [Qemu-devel] [PATCH 2/8] qemu-char: Rename qemu_chr_generic_open to qemu_chr_be_generic_open Hans de Goede
` (7 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2013-03-24 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, Hans de Goede
Rename the opened variable to be_open to reflect that it contains the
opened state of the backend.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/usb/dev-serial.c | 2 +-
hw/usb/redirect.c | 2 +-
include/char/char.h | 2 +-
qemu-char.c | 6 +++---
spice-qemu-char.c | 4 ++--
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 47ac8c9..7c314dc 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -495,7 +495,7 @@ static int usb_serial_initfn(USBDevice *dev)
usb_serial_event, s);
usb_serial_handle_reset(dev);
- if (s->cs->opened && !dev->attached) {
+ if (s->cs->be_open && !dev->attached) {
usb_device_attach(dev);
}
return 0;
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index e0df74b..4d565ba 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -272,7 +272,7 @@ static int usbredir_write(void *priv, uint8_t *data, int count)
USBRedirDevice *dev = priv;
int r;
- if (!dev->cs->opened) {
+ if (!dev->cs->be_open) {
return 0;
}
diff --git a/include/char/char.h b/include/char/char.h
index 0326b2a..d801f92 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -74,7 +74,7 @@ struct CharDriverState {
int idle_tag;
char *label;
char *filename;
- int opened;
+ int be_open;
int avail_connections;
QemuOpts *opts;
QTAILQ_ENTRY(CharDriverState) next;
diff --git a/qemu-char.c b/qemu-char.c
index 4e011df..32a05af 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -108,10 +108,10 @@ void qemu_chr_be_event(CharDriverState *s, int event)
/* Keep track if the char device is open */
switch (event) {
case CHR_EVENT_OPENED:
- s->opened = 1;
+ s->be_open = 1;
break;
case CHR_EVENT_CLOSED:
- s->opened = 0;
+ s->be_open = 0;
break;
}
@@ -207,7 +207,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
/* We're connecting to an already opened device, so let's make sure we
also get the open event */
- if (s->opened) {
+ if (s->be_open) {
qemu_chr_generic_open(s);
}
}
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 607abb6..613cc64 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -91,8 +91,8 @@ static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
{
SpiceCharDriver *scd = container_of(sin, SpiceCharDriver, sin);
- if ((scd->chr->opened && connected) ||
- (!scd->chr->opened && !connected)) {
+ if ((scd->chr->be_open && connected) ||
+ (!scd->chr->be_open && !connected)) {
return;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 2/8] qemu-char: Rename qemu_chr_generic_open to qemu_chr_be_generic_open
2013-03-24 12:39 [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Hans de Goede
2013-03-24 12:39 ` [Qemu-devel] [PATCH 1/8] qemu-char: Rename opened to be_open Hans de Goede
@ 2013-03-24 12:39 ` Hans de Goede
2013-03-24 12:39 ` [Qemu-devel] [PATCH 3/8] qemu-char: Add fe_open tracking Hans de Goede
` (6 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2013-03-24 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, Hans de Goede
To better reflect that it is for handling a backend being opened.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
backends/baum.c | 2 +-
include/char/char.h | 2 +-
qemu-char.c | 24 ++++++++++++------------
ui/console.c | 2 +-
ui/gtk.c | 2 +-
5 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/backends/baum.c b/backends/baum.c
index d7d658c..ea9ffe8 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -611,7 +611,7 @@ CharDriverState *chr_baum_init(void)
qemu_set_fd_handler(baum->brlapi_fd, baum_chr_read, NULL, baum);
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
return chr;
diff --git a/include/char/char.h b/include/char/char.h
index d801f92..dd8f39a 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -235,7 +235,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
IOEventHandler *fd_event,
void *opaque);
-void qemu_chr_generic_open(CharDriverState *s);
+void qemu_chr_be_generic_open(CharDriverState *s);
void qemu_chr_accept_input(CharDriverState *s);
int qemu_chr_add_client(CharDriverState *s, int fd);
void qemu_chr_info_print(Monitor *mon, const QObject *ret_data);
diff --git a/qemu-char.c b/qemu-char.c
index 32a05af..55795d7 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -120,7 +120,7 @@ void qemu_chr_be_event(CharDriverState *s, int event)
s->chr_event(s->handler_opaque, event);
}
-static gboolean qemu_chr_generic_open_bh(gpointer opaque)
+static gboolean qemu_chr_be_generic_open_bh(gpointer opaque)
{
CharDriverState *s = opaque;
qemu_chr_be_event(s, CHR_EVENT_OPENED);
@@ -128,10 +128,10 @@ static gboolean qemu_chr_generic_open_bh(gpointer opaque)
return FALSE;
}
-void qemu_chr_generic_open(CharDriverState *s)
+void qemu_chr_be_generic_open(CharDriverState *s)
{
if (s->idle_tag == 0) {
- s->idle_tag = g_idle_add(qemu_chr_generic_open_bh, s);
+ s->idle_tag = g_idle_add(qemu_chr_be_generic_open_bh, s);
}
}
@@ -208,7 +208,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
/* We're connecting to an already opened device, so let's make sure we
also get the open event */
if (s->be_open) {
- qemu_chr_generic_open(s);
+ qemu_chr_be_generic_open(s);
}
}
@@ -482,7 +482,7 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
chr->chr_guest_close = NULL;
/* Muxes are always open on creation */
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
return chr;
}
@@ -836,7 +836,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
chr->chr_update_read_handler = fd_chr_update_read_handler;
chr->chr_close = fd_chr_close;
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
return chr;
}
@@ -1133,7 +1133,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
pty_chr_rearm_timer(chr, 1000);
} else {
if (!s->connected)
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
s->connected = 1;
}
}
@@ -1549,7 +1549,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
chr->chr_close = pp_close;
chr->opaque = drv;
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
return chr;
}
@@ -1834,7 +1834,7 @@ static CharDriverState *qemu_chr_open_win_path(const char *filename)
g_free(chr);
return NULL;
}
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
return chr;
}
@@ -1934,7 +1934,7 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
g_free(chr);
return NULL;
}
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
return chr;
}
@@ -1948,7 +1948,7 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
s->hcom = fd_out;
chr->opaque = s;
chr->chr_write = win_chr_write;
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
return chr;
}
@@ -2513,7 +2513,7 @@ static void tcp_chr_connect(void *opaque)
if (s->chan) {
s->tag = io_add_watch_poll(s->chan, tcp_chr_read_poll, tcp_chr_read, chr);
}
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
}
#define IACSET(x,a,b,c) x[0] = a; x[1] = b; x[2] = c;
diff --git a/ui/console.c b/ui/console.c
index eb7a2bc..e84ba8b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1600,7 +1600,7 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
s->t_attrib = s->t_attrib_default;
}
- qemu_chr_generic_open(chr);
+ qemu_chr_be_generic_open(chr);
if (chr->init)
chr->init(chr);
}
diff --git a/ui/gtk.c b/ui/gtk.c
index 305940d..903b4d4 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1116,7 +1116,7 @@ static GSList *gd_vc_init(GtkDisplayState *s, VirtualConsole *vc, int index, GSL
gtk_menu_shell_append(GTK_MENU_SHELL(s->view_menu), vc->menu_item);
- qemu_chr_generic_open(vc->chr);
+ qemu_chr_be_generic_open(vc->chr);
if (vc->chr->init) {
vc->chr->init(vc->chr);
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 3/8] qemu-char: Add fe_open tracking
2013-03-24 12:39 [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Hans de Goede
2013-03-24 12:39 ` [Qemu-devel] [PATCH 1/8] qemu-char: Rename opened to be_open Hans de Goede
2013-03-24 12:39 ` [Qemu-devel] [PATCH 2/8] qemu-char: Rename qemu_chr_generic_open to qemu_chr_be_generic_open Hans de Goede
@ 2013-03-24 12:39 ` Hans de Goede
2013-03-25 12:45 ` Anthony Liguori
2013-03-24 12:39 ` [Qemu-devel] [PATCH 4/8] qemu-char: Automatically do fe_open / fe_close on qemu_chr_add_handlers Hans de Goede
` (5 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Hans de Goede @ 2013-03-24 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, Hans de Goede
Add tracking of the fe_open state to struct CharDriverState.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
include/char/char.h | 1 +
qemu-char.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/include/char/char.h b/include/char/char.h
index dd8f39a..3174575 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -75,6 +75,7 @@ struct CharDriverState {
char *label;
char *filename;
int be_open;
+ int fe_open;
int avail_connections;
QemuOpts *opts;
QTAILQ_ENTRY(CharDriverState) next;
diff --git a/qemu-char.c b/qemu-char.c
index 55795d7..554d72f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3385,6 +3385,7 @@ void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo)
void qemu_chr_fe_open(struct CharDriverState *chr)
{
+ chr->fe_open = 1;
if (chr->chr_guest_open) {
chr->chr_guest_open(chr);
}
@@ -3392,6 +3393,7 @@ void qemu_chr_fe_open(struct CharDriverState *chr)
void qemu_chr_fe_close(struct CharDriverState *chr)
{
+ chr->fe_open = 0;
if (chr->chr_guest_close) {
chr->chr_guest_close(chr);
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/8] qemu-char: Add fe_open tracking
2013-03-24 12:39 ` [Qemu-devel] [PATCH 3/8] qemu-char: Add fe_open tracking Hans de Goede
@ 2013-03-25 12:45 ` Anthony Liguori
2013-03-25 13:07 ` Hans de Goede
0 siblings, 1 reply; 15+ messages in thread
From: Anthony Liguori @ 2013-03-25 12:45 UTC (permalink / raw)
To: Hans de Goede, qemu-devel; +Cc: Amit Shah
Hans de Goede <hdegoede@redhat.com> writes:
> Add tracking of the fe_open state to struct CharDriverState.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> include/char/char.h | 1 +
> qemu-char.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/include/char/char.h b/include/char/char.h
> index dd8f39a..3174575 100644
> --- a/include/char/char.h
> +++ b/include/char/char.h
> @@ -75,6 +75,7 @@ struct CharDriverState {
> char *label;
> char *filename;
> int be_open;
> + int fe_open;
> int avail_connections;
> QemuOpts *opts;
> QTAILQ_ENTRY(CharDriverState) next;
> diff --git a/qemu-char.c b/qemu-char.c
> index 55795d7..554d72f 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3385,6 +3385,7 @@ void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo)
>
> void qemu_chr_fe_open(struct CharDriverState *chr)
> {
> + chr->fe_open = 1;
> if (chr->chr_guest_open) {
> chr->chr_guest_open(chr);
> }
> @@ -3392,6 +3393,7 @@ void qemu_chr_fe_open(struct CharDriverState *chr)
>
> void qemu_chr_fe_close(struct CharDriverState *chr)
> {
> + chr->fe_open = 0;
Even though this gets rewritten later on, you should avoid calling the
callback when open is called when fe_open=1. Something like
if (!chr->fe_open) {
return;
}
Then later when this becomes set_fe_open() the backend doesn't need to
deal with double open/close.
Regards,
Anthony Liguori
> if (chr->chr_guest_close) {
> chr->chr_guest_close(chr);
> }
> --
> 1.8.1.4
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 3/8] qemu-char: Add fe_open tracking
2013-03-25 12:45 ` Anthony Liguori
@ 2013-03-25 13:07 ` Hans de Goede
0 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2013-03-25 13:07 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Amit Shah, qemu-devel
Hi,
On 03/25/2013 01:45 PM, Anthony Liguori wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
>
>> Add tracking of the fe_open state to struct CharDriverState.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> include/char/char.h | 1 +
>> qemu-char.c | 2 ++
>> 2 files changed, 3 insertions(+)
>>
>> diff --git a/include/char/char.h b/include/char/char.h
>> index dd8f39a..3174575 100644
>> --- a/include/char/char.h
>> +++ b/include/char/char.h
>> @@ -75,6 +75,7 @@ struct CharDriverState {
>> char *label;
>> char *filename;
>> int be_open;
>> + int fe_open;
>> int avail_connections;
>> QemuOpts *opts;
>> QTAILQ_ENTRY(CharDriverState) next;
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 55795d7..554d72f 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -3385,6 +3385,7 @@ void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo)
>>
>> void qemu_chr_fe_open(struct CharDriverState *chr)
>> {
>> + chr->fe_open = 1;
>> if (chr->chr_guest_open) {
>> chr->chr_guest_open(chr);
>> }
>> @@ -3392,6 +3393,7 @@ void qemu_chr_fe_open(struct CharDriverState *chr)
>>
>> void qemu_chr_fe_close(struct CharDriverState *chr)
>> {
>> + chr->fe_open = 0;
>
> Even though this gets rewritten later on, you should avoid calling the
> callback when open is called when fe_open=1. Something like
>
> if (!chr->fe_open) {
> return;
> }
>
> Then later when this becomes set_fe_open() the backend doesn't need to
> deal with double open/close.
Ok, will fix in the next iteration of this patchset.
Regards,
Hans
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 4/8] qemu-char: Automatically do fe_open / fe_close on qemu_chr_add_handlers
2013-03-24 12:39 [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Hans de Goede
` (2 preceding siblings ...)
2013-03-24 12:39 ` [Qemu-devel] [PATCH 3/8] qemu-char: Add fe_open tracking Hans de Goede
@ 2013-03-24 12:39 ` Hans de Goede
2013-03-25 12:46 ` Anthony Liguori
2013-03-24 12:39 ` [Qemu-devel] [PATCH 5/8] qemu-char: Cleanup: consolidate fe_open/fe_close into fe_set_open Hans de Goede
` (4 subsequent siblings)
8 siblings, 1 reply; 15+ messages in thread
From: Hans de Goede @ 2013-03-24 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, Hans de Goede
Most frontends can't really determine if the guest actually has the frontend
side open. So lets automatically generate fe_open / fe_close as soon as a
frontend becomes ready (as signalled by calling qemu_chr_add_handlers) /
becomes non ready (as signalled by setting all handlers to NULL).
And allow frontends which can actually determine if the guest is listening to
opt-out of this.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/usb/redirect.c | 2 --
hw/virtio-console.c | 1 +
include/char/char.h | 1 +
qemu-char.c | 13 +++++++++++++
4 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 4d565ba..0ddb081 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1310,7 +1310,6 @@ static int usbredir_initfn(USBDevice *udev)
dev->compatible_speedmask = USB_SPEED_MASK_FULL | USB_SPEED_MASK_HIGH;
/* Let the backend know we are ready */
- qemu_chr_fe_open(dev->cs);
qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
usbredir_chardev_read, usbredir_chardev_event, dev);
@@ -1334,7 +1333,6 @@ static void usbredir_handle_destroy(USBDevice *udev)
{
USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
- qemu_chr_fe_close(dev->cs);
qemu_chr_delete(dev->cs);
/* Note must be done after qemu_chr_close, as that causes a close event */
qemu_bh_delete(dev->chardev_close_bh);
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 8ef76e2..209ea38 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -140,6 +140,7 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
}
if (vcon->chr) {
+ vcon->chr->explicit_fe_open = 1;
qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
vcon);
}
diff --git a/include/char/char.h b/include/char/char.h
index 3174575..27ebbc3 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -76,6 +76,7 @@ struct CharDriverState {
char *filename;
int be_open;
int fe_open;
+ int explicit_fe_open;
int avail_connections;
QemuOpts *opts;
QTAILQ_ENTRY(CharDriverState) next;
diff --git a/qemu-char.c b/qemu-char.c
index 554d72f..7c57971 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -194,9 +194,14 @@ void qemu_chr_add_handlers(CharDriverState *s,
IOEventHandler *fd_event,
void *opaque)
{
+ int fe_open;
+
if (!opaque && !fd_can_read && !fd_read && !fd_event) {
/* chr driver being released. */
+ fe_open = 0;
++s->avail_connections;
+ } else {
+ fe_open = 1;
}
s->chr_can_read = fd_can_read;
s->chr_read = fd_read;
@@ -205,6 +210,14 @@ void qemu_chr_add_handlers(CharDriverState *s,
if (s->chr_update_read_handler)
s->chr_update_read_handler(s);
+ if (!s->explicit_fe_open) {
+ if (fe_open) {
+ qemu_chr_fe_open(s);
+ } else {
+ qemu_chr_fe_close(s);
+ }
+ }
+
/* We're connecting to an already opened device, so let's make sure we
also get the open event */
if (s->be_open) {
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/8] qemu-char: Automatically do fe_open / fe_close on qemu_chr_add_handlers
2013-03-24 12:39 ` [Qemu-devel] [PATCH 4/8] qemu-char: Automatically do fe_open / fe_close on qemu_chr_add_handlers Hans de Goede
@ 2013-03-25 12:46 ` Anthony Liguori
2013-03-25 13:17 ` Hans de Goede
0 siblings, 1 reply; 15+ messages in thread
From: Anthony Liguori @ 2013-03-25 12:46 UTC (permalink / raw)
To: Hans de Goede, qemu-devel; +Cc: Amit Shah
Hans de Goede <hdegoede@redhat.com> writes:
> Most frontends can't really determine if the guest actually has the frontend
> side open. So lets automatically generate fe_open / fe_close as soon as a
> frontend becomes ready (as signalled by calling qemu_chr_add_handlers) /
> becomes non ready (as signalled by setting all handlers to NULL).
>
> And allow frontends which can actually determine if the guest is listening to
> opt-out of this.
Could we change virtio-serial to delay calling add_handlers so that we
could not introduce this variable?
Regards,
Anthony Liguori
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> hw/usb/redirect.c | 2 --
> hw/virtio-console.c | 1 +
> include/char/char.h | 1 +
> qemu-char.c | 13 +++++++++++++
> 4 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 4d565ba..0ddb081 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1310,7 +1310,6 @@ static int usbredir_initfn(USBDevice *udev)
> dev->compatible_speedmask = USB_SPEED_MASK_FULL | USB_SPEED_MASK_HIGH;
>
> /* Let the backend know we are ready */
> - qemu_chr_fe_open(dev->cs);
> qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
> usbredir_chardev_read, usbredir_chardev_event, dev);
>
> @@ -1334,7 +1333,6 @@ static void usbredir_handle_destroy(USBDevice *udev)
> {
> USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
>
> - qemu_chr_fe_close(dev->cs);
> qemu_chr_delete(dev->cs);
> /* Note must be done after qemu_chr_close, as that causes a close event */
> qemu_bh_delete(dev->chardev_close_bh);
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index 8ef76e2..209ea38 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -140,6 +140,7 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
> }
>
> if (vcon->chr) {
> + vcon->chr->explicit_fe_open = 1;
> qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
> vcon);
> }
> diff --git a/include/char/char.h b/include/char/char.h
> index 3174575..27ebbc3 100644
> --- a/include/char/char.h
> +++ b/include/char/char.h
> @@ -76,6 +76,7 @@ struct CharDriverState {
> char *filename;
> int be_open;
> int fe_open;
> + int explicit_fe_open;
> int avail_connections;
> QemuOpts *opts;
> QTAILQ_ENTRY(CharDriverState) next;
> diff --git a/qemu-char.c b/qemu-char.c
> index 554d72f..7c57971 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -194,9 +194,14 @@ void qemu_chr_add_handlers(CharDriverState *s,
> IOEventHandler *fd_event,
> void *opaque)
> {
> + int fe_open;
> +
> if (!opaque && !fd_can_read && !fd_read && !fd_event) {
> /* chr driver being released. */
> + fe_open = 0;
> ++s->avail_connections;
> + } else {
> + fe_open = 1;
> }
> s->chr_can_read = fd_can_read;
> s->chr_read = fd_read;
> @@ -205,6 +210,14 @@ void qemu_chr_add_handlers(CharDriverState *s,
> if (s->chr_update_read_handler)
> s->chr_update_read_handler(s);
>
> + if (!s->explicit_fe_open) {
> + if (fe_open) {
> + qemu_chr_fe_open(s);
> + } else {
> + qemu_chr_fe_close(s);
> + }
> + }
> +
> /* We're connecting to an already opened device, so let's make sure we
> also get the open event */
> if (s->be_open) {
> --
> 1.8.1.4
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/8] qemu-char: Automatically do fe_open / fe_close on qemu_chr_add_handlers
2013-03-25 12:46 ` Anthony Liguori
@ 2013-03-25 13:17 ` Hans de Goede
2013-03-25 15:07 ` Anthony Liguori
0 siblings, 1 reply; 15+ messages in thread
From: Hans de Goede @ 2013-03-25 13:17 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Amit Shah, qemu-devel
Hi,
On 03/25/2013 01:46 PM, Anthony Liguori wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
>
>> Most frontends can't really determine if the guest actually has the frontend
>> side open. So lets automatically generate fe_open / fe_close as soon as a
>> frontend becomes ready (as signalled by calling qemu_chr_add_handlers) /
>> becomes non ready (as signalled by setting all handlers to NULL).
>>
>> And allow frontends which can actually determine if the guest is listening to
>> opt-out of this.
>
> Could we change virtio-serial to delay calling add_handlers so that we
> could not introduce this variable?
Hmm, I was trying to avoid opening that can of worms. I've taken a quick look
and there are 2 issues with doing this:
1) It will wreck havoc with CharDriverState.avail_connections, since that
gets increased on a qemu_chr_add_handlers( NULL ), while it is decreased
only once in hw/qdev-properties-system.c
This can be fixed by moving the avail_connections++ to
hw/qdev-properties-system.c: release_chr, which seems the sensible thing to
do wrt nicely balancing out these calls anyways.
2) It will cause the virtio-console front-end to miss various events, such
as backend open/close events.
A backend open event will get "replayed" when qemu_chr_add_handlers( stuff )
is called, causing a potential double open from the virtio-console pov
(one before it called qemu_chr_add_handlers( NULL ), and one on the next
add_handlers call). And a close or any other events happening while the
frontend side is closed will be missed.
I'll gladly add a patch fixing 1). to the next revision of this patchset,
but given 2) I would prefer to stick with the explicit_fe_open flag and just
register the handlers once.
Regards,
Hans
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 4/8] qemu-char: Automatically do fe_open / fe_close on qemu_chr_add_handlers
2013-03-25 13:17 ` Hans de Goede
@ 2013-03-25 15:07 ` Anthony Liguori
0 siblings, 0 replies; 15+ messages in thread
From: Anthony Liguori @ 2013-03-25 15:07 UTC (permalink / raw)
To: Hans de Goede; +Cc: Amit Shah, qemu-devel
Hans de Goede <hdegoede@redhat.com> writes:
> Hi,
>
> On 03/25/2013 01:46 PM, Anthony Liguori wrote:
>> Hans de Goede <hdegoede@redhat.com> writes:
>>
>>> Most frontends can't really determine if the guest actually has the frontend
>>> side open. So lets automatically generate fe_open / fe_close as soon as a
>>> frontend becomes ready (as signalled by calling qemu_chr_add_handlers) /
>>> becomes non ready (as signalled by setting all handlers to NULL).
>>>
>>> And allow frontends which can actually determine if the guest is listening to
>>> opt-out of this.
>>
>> Could we change virtio-serial to delay calling add_handlers so that we
>> could not introduce this variable?
>
> Hmm, I was trying to avoid opening that can of worms. I've taken a quick look
> and there are 2 issues with doing this:
> 1) It will wreck havoc with CharDriverState.avail_connections, since that
> gets increased on a qemu_chr_add_handlers( NULL ), while it is decreased
> only once in hw/qdev-properties-system.c
>
> This can be fixed by moving the avail_connections++ to
> hw/qdev-properties-system.c: release_chr, which seems the sensible thing to
> do wrt nicely balancing out these calls anyways.
>
> 2) It will cause the virtio-console front-end to miss various events, such
> as backend open/close events.
>
> A backend open event will get "replayed" when qemu_chr_add_handlers( stuff )
> is called, causing a potential double open from the virtio-console pov
> (one before it called qemu_chr_add_handlers( NULL ), and one on the next
> add_handlers call). And a close or any other events happening while the
> frontend side is closed will be missed.
>
> I'll gladly add a patch fixing 1). to the next revision of this patchset,
> but given 2) I would prefer to stick with the explicit_fe_open flag and just
> register the handlers once.
Okay, seems reasonable.
REgards,
Anthony Liguori
>
> Regards,
>
> Hans
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 5/8] qemu-char: Cleanup: consolidate fe_open/fe_close into fe_set_open
2013-03-24 12:39 [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Hans de Goede
` (3 preceding siblings ...)
2013-03-24 12:39 ` [Qemu-devel] [PATCH 4/8] qemu-char: Automatically do fe_open / fe_close on qemu_chr_add_handlers Hans de Goede
@ 2013-03-24 12:39 ` Hans de Goede
2013-03-24 12:39 ` [Qemu-devel] [PATCH 6/8] qemu-char: Consolidate guest_close/guest_open into a set_fe_open callback Hans de Goede
` (3 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2013-03-24 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, Hans de Goede
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/virtio-console.c | 4 ++--
include/char/char.h | 17 ++++-------------
qemu-char.c | 19 +++++--------------
3 files changed, 11 insertions(+), 29 deletions(-)
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 209ea38..c0d41c5 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -79,7 +79,7 @@ static void guest_open(VirtIOSerialPort *port)
if (!vcon->chr) {
return;
}
- qemu_chr_fe_open(vcon->chr);
+ qemu_chr_fe_set_open(vcon->chr, 1);
}
/* Callback function that's called when the guest closes the port */
@@ -90,7 +90,7 @@ static void guest_close(VirtIOSerialPort *port)
if (!vcon->chr) {
return;
}
- qemu_chr_fe_close(vcon->chr);
+ qemu_chr_fe_set_open(vcon->chr, 0);
}
/* Readiness of the guest to accept data on a port */
diff --git a/include/char/char.h b/include/char/char.h
index 27ebbc3..3c8dd28 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -129,21 +129,12 @@ void qemu_chr_delete(CharDriverState *chr);
void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo);
/**
- * @qemu_chr_fe_open:
+ * @qemu_chr_fe_set_open:
*
- * Open a character backend. This function call is an indication that the
- * front end is ready to begin doing I/O.
+ * Set character frontend open status. This is an indication that the
+ * front end is ready (or not) to begin doing I/O.
*/
-void qemu_chr_fe_open(struct CharDriverState *chr);
-
-/**
- * @qemu_chr_fe_close:
- *
- * Close a character backend. This function call indicates that the front end
- * no longer is able to process I/O. To process I/O again, the front end will
- * call @qemu_chr_fe_open.
- */
-void qemu_chr_fe_close(struct CharDriverState *chr);
+void qemu_chr_fe_set_open(struct CharDriverState *chr, int fe_open);
/**
* @qemu_chr_fe_printf:
diff --git a/qemu-char.c b/qemu-char.c
index 7c57971..713c154 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -211,11 +211,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
s->chr_update_read_handler(s);
if (!s->explicit_fe_open) {
- if (fe_open) {
- qemu_chr_fe_open(s);
- } else {
- qemu_chr_fe_close(s);
- }
+ qemu_chr_fe_set_open(s, fe_open);
}
/* We're connecting to an already opened device, so let's make sure we
@@ -3396,18 +3392,13 @@ void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo)
}
}
-void qemu_chr_fe_open(struct CharDriverState *chr)
+void qemu_chr_fe_set_open(struct CharDriverState *chr, int fe_open)
{
- chr->fe_open = 1;
- if (chr->chr_guest_open) {
+ chr->fe_open = fe_open;
+ if (fe_open && chr->chr_guest_open) {
chr->chr_guest_open(chr);
}
-}
-
-void qemu_chr_fe_close(struct CharDriverState *chr)
-{
- chr->fe_open = 0;
- if (chr->chr_guest_close) {
+ if (!fe_open && chr->chr_guest_close) {
chr->chr_guest_close(chr);
}
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 6/8] qemu-char: Consolidate guest_close/guest_open into a set_fe_open callback
2013-03-24 12:39 [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Hans de Goede
` (4 preceding siblings ...)
2013-03-24 12:39 ` [Qemu-devel] [PATCH 5/8] qemu-char: Cleanup: consolidate fe_open/fe_close into fe_set_open Hans de Goede
@ 2013-03-24 12:39 ` Hans de Goede
2013-03-24 12:39 ` [Qemu-devel] [PATCH 7/8] virtio-serial: Consolidate guest_open/guest_close into set_guest_connected Hans de Goede
` (2 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2013-03-24 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, Hans de Goede
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
include/char/char.h | 3 +--
qemu-char.c | 10 +++-------
spice-qemu-char.c | 17 +++++++----------
3 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/include/char/char.h b/include/char/char.h
index 3c8dd28..1457e80 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -68,8 +68,7 @@ struct CharDriverState {
void (*chr_close)(struct CharDriverState *chr);
void (*chr_accept_input)(struct CharDriverState *chr);
void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
- void (*chr_guest_open)(struct CharDriverState *chr);
- void (*chr_guest_close)(struct CharDriverState *chr);
+ void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
void *opaque;
int idle_tag;
char *label;
diff --git a/qemu-char.c b/qemu-char.c
index 713c154..5be2ae7 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -487,8 +487,7 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
chr->chr_update_read_handler = mux_chr_update_read_handler;
chr->chr_accept_input = mux_chr_accept_input;
/* Frontend guest-open / -close notification is not support with muxes */
- chr->chr_guest_open = NULL;
- chr->chr_guest_close = NULL;
+ chr->chr_set_fe_open = NULL;
/* Muxes are always open on creation */
qemu_chr_be_generic_open(chr);
@@ -3395,11 +3394,8 @@ void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo)
void qemu_chr_fe_set_open(struct CharDriverState *chr, int fe_open)
{
chr->fe_open = fe_open;
- if (fe_open && chr->chr_guest_open) {
- chr->chr_guest_open(chr);
- }
- if (!fe_open && chr->chr_guest_close) {
- chr->chr_guest_close(chr);
+ if (chr->chr_set_fe_open) {
+ chr->chr_set_fe_open(chr, fe_open);
}
}
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 613cc64..ba59374 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -213,16 +213,14 @@ static void spice_chr_close(struct CharDriverState *chr)
g_free(s);
}
-static void spice_chr_guest_open(struct CharDriverState *chr)
+static void spice_chr_set_fe_open(struct CharDriverState *chr, int fe_open)
{
SpiceCharDriver *s = chr->opaque;
- vmc_register_interface(s);
-}
-
-static void spice_chr_guest_close(struct CharDriverState *chr)
-{
- SpiceCharDriver *s = chr->opaque;
- vmc_unregister_interface(s);
+ if (fe_open) {
+ vmc_register_interface(s);
+ } else {
+ vmc_unregister_interface(s);
+ }
}
static void print_allowed_subtypes(void)
@@ -256,8 +254,7 @@ static CharDriverState *chr_open(const char *subtype)
chr->chr_write = spice_chr_write;
chr->chr_add_watch = spice_chr_add_watch;
chr->chr_close = spice_chr_close;
- chr->chr_guest_open = spice_chr_guest_open;
- chr->chr_guest_close = spice_chr_guest_close;
+ chr->chr_set_fe_open = spice_chr_set_fe_open;
QLIST_INSERT_HEAD(&spice_chars, s, next);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 7/8] virtio-serial: Consolidate guest_open/guest_close into set_guest_connected
2013-03-24 12:39 [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Hans de Goede
` (5 preceding siblings ...)
2013-03-24 12:39 ` [Qemu-devel] [PATCH 6/8] qemu-char: Consolidate guest_close/guest_open into a set_fe_open callback Hans de Goede
@ 2013-03-24 12:39 ` Hans de Goede
2013-03-24 12:39 ` [Qemu-devel] [PATCH 8/8] spice-qemu-char: Drop hackish vmc_register on spice_chr_write Hans de Goede
2013-03-25 9:56 ` [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Alon Levy
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2013-03-24 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, Hans de Goede
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
hw/virtio-console.c | 23 +++++------------------
hw/virtio-serial-bus.c | 15 +++++----------
hw/virtio-serial.h | 6 ++----
3 files changed, 12 insertions(+), 32 deletions(-)
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index c0d41c5..e28c54f 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -71,26 +71,15 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
return ret;
}
-/* Callback function that's called when the guest opens the port */
-static void guest_open(VirtIOSerialPort *port)
+/* Callback function that's called when the guest opens/closes the port */
+static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
{
VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
if (!vcon->chr) {
return;
}
- qemu_chr_fe_set_open(vcon->chr, 1);
-}
-
-/* Callback function that's called when the guest closes the port */
-static void guest_close(VirtIOSerialPort *port)
-{
- VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
-
- if (!vcon->chr) {
- return;
- }
- qemu_chr_fe_set_open(vcon->chr, 0);
+ qemu_chr_fe_set_open(vcon->chr, guest_connected);
}
/* Readiness of the guest to accept data on a port */
@@ -173,8 +162,7 @@ static void virtconsole_class_init(ObjectClass *klass, void *data)
k->init = virtconsole_initfn;
k->exit = virtconsole_exitfn;
k->have_data = flush_buf;
- k->guest_open = guest_open;
- k->guest_close = guest_close;
+ k->set_guest_connected = set_guest_connected;
dc->props = virtconsole_properties;
}
@@ -197,8 +185,7 @@ static void virtserialport_class_init(ObjectClass *klass, void *data)
k->init = virtconsole_initfn;
k->have_data = flush_buf;
- k->guest_open = guest_open;
- k->guest_close = guest_close;
+ k->set_guest_connected = set_guest_connected;
dc->props = virtserialport_properties;
}
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index ab7168e..eb7af21 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -372,14 +372,9 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
case VIRTIO_CONSOLE_PORT_OPEN:
port->guest_connected = cpkt.value;
- if (cpkt.value && vsc->guest_open) {
+ if (vsc->set_guest_connected) {
/* Send the guest opened notification if an app is interested */
- vsc->guest_open(port);
- }
-
- if (!cpkt.value && vsc->guest_close) {
- /* Send the guest closed notification if an app is interested */
- vsc->guest_close(port);
+ vsc->set_guest_connected(port, cpkt.value);
}
break;
}
@@ -484,9 +479,9 @@ static void guest_reset(VirtIOSerial *vser)
vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
if (port->guest_connected) {
port->guest_connected = false;
-
- if (vsc->guest_close)
- vsc->guest_close(port);
+ if (vsc->set_guest_connected) {
+ vsc->set_guest_connected(port, false);
+ }
}
}
}
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 484dcfe..516400f 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -92,10 +92,8 @@ typedef struct VirtIOSerialPortClass {
int (*exit)(VirtIOSerialPort *port);
/* Callbacks for guest events */
- /* Guest opened device. */
- void (*guest_open)(VirtIOSerialPort *port);
- /* Guest closed device. */
- void (*guest_close)(VirtIOSerialPort *port);
+ /* Guest opened/closed device. */
+ void (*set_guest_connected)(VirtIOSerialPort *port, int guest_connected);
/* Guest is now ready to accept data (virtqueues set up). */
void (*guest_ready)(VirtIOSerialPort *port);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Qemu-devel] [PATCH 8/8] spice-qemu-char: Drop hackish vmc_register on spice_chr_write
2013-03-24 12:39 [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Hans de Goede
` (6 preceding siblings ...)
2013-03-24 12:39 ` [Qemu-devel] [PATCH 7/8] virtio-serial: Consolidate guest_open/guest_close into set_guest_connected Hans de Goede
@ 2013-03-24 12:39 ` Hans de Goede
2013-03-25 9:56 ` [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Alon Levy
8 siblings, 0 replies; 15+ messages in thread
From: Hans de Goede @ 2013-03-24 12:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Amit Shah, Hans de Goede
Now that the core takes care of fe_open tracking we no longer need this hack.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
spice-qemu-char.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index ba59374..7e6bd2d 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -184,7 +184,6 @@ static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
SpiceCharDriver *s = chr->opaque;
int read_bytes;
- vmc_register_interface(s);
assert(s->datalen == 0);
s->datapos = buf;
s->datalen = len;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup
2013-03-24 12:39 [Qemu-devel] [PATCH 1/8] RFC: chardev frontend open handling cleanup Hans de Goede
` (7 preceding siblings ...)
2013-03-24 12:39 ` [Qemu-devel] [PATCH 8/8] spice-qemu-char: Drop hackish vmc_register on spice_chr_write Hans de Goede
@ 2013-03-25 9:56 ` Alon Levy
8 siblings, 0 replies; 15+ messages in thread
From: Alon Levy @ 2013-03-25 9:56 UTC (permalink / raw)
To: Hans de Goede; +Cc: Amit Shah, qemu-devel
> This patch-series is the result of the
> "[PATCH 1/2] char: add qemu_chr_be_is_fe_connected" discussion
> thread.
>
> This patch series (tries to) make(s) the frontend open concept both
> more
> explicit and generic, and significantly cleans up the surrounding
> code.
The whole patch series looks good to me.
>
> Regards,
>
> Hans
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread