Patchwork [4/8] qemu-char: Automatically do fe_open / fe_close on qemu_chr_add_handlers

login
register
mail settings
Submitter Hans de Goede
Date March 24, 2013, 12:39 p.m.
Message ID <1364128793-12689-5-git-send-email-hdegoede@redhat.com>
Download mbox | patch
Permalink /patch/230442/
State New
Headers show

Comments

Hans de Goede - March 24, 2013, 12:39 p.m.
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(-)
Anthony Liguori - March 25, 2013, 12:46 p.m.
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
Hans de Goede - March 25, 2013, 1:17 p.m.
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
Anthony Liguori - March 25, 2013, 3:07 p.m.
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

Patch

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) {