[Qemu-devel] [PATCH 1/5] qemu-char: make qemu_chr_event public

[Qemu-devel] [PATCH 1/5] qemu-char: make qemu_chr_event public

[Hans de Goede]

Make qemu_chr_event public so that it can be used by chardev code
which lives outside of qemu-char.c

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 qemu-char.c |    2 +-
 qemu-char.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 8d39500..5d5a6d5 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -119,7 +119,7 @@ static void char_write_unblocked(void *opaque)
     chr->chr_write_unblocked(chr->handler_opaque);
 }
 
-static void qemu_chr_event(CharDriverState *s, int event)
+void qemu_chr_event(CharDriverState *s, int event)
 {
     /* Keep track if the char device is open */
     switch (event) {
diff --git a/qemu-char.h b/qemu-char.h
index 68e7b5b..77ad62d 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -107,6 +107,7 @@ int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg);
 void qemu_chr_generic_open(CharDriverState *s);
 int qemu_chr_can_read(CharDriverState *s);
 void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
+void qemu_chr_event(CharDriverState *s, int event);
 int qemu_chr_get_msgfd(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
 int qemu_chr_add_client(CharDriverState *s, int fd);
-- 
1.7.5.1

[Qemu-devel] [PATCH 2/5] spice-qemu-char: Generate chardev open/close events

[Hans de Goede]

Define a state callback and make that generate chardev open/close events when
called by the spice-server.

Note that for all but the newest spice-server versions (which have a fix for
this) the code ignores these events for a spicevmc with a subtype of vdagent,
this subtype specific knowledge is undesirable, but unavoidable for now, see:
http://lists.freedesktop.org/archives/spice-devel/2011-July/004837.html

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 spice-qemu-char.c |   35 ++++++++++++++++++++++++++++++++++-
 1 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 2b8aec4..d55e74a 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -89,11 +89,39 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t *buf, int len)
     return bytes;
 }
 
+static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
+{
+    SpiceCharDriver *scd = container_of(sin, SpiceCharDriver, sin);
+
+#if SPICE_SERVER_VERSION < 0x000901
+    /*
+     * spice-server calls the state callback for the agent channel when the
+     * spice client connects / disconnects. Given that not the client but
+     * the server is doing the parsing of the messages this is wrong as the
+     * server is still listening. Worse, this causes the parser in the server
+     * to go out of sync, so we ignore state calls for subtype vdagent
+     * spicevmc chardevs. For the full story see:
+     * http://lists.freedesktop.org/archives/spice-devel/2011-July/004837.html
+     */
+    if (strcmp(sin->subtype, "vdagent") == 0) {
+        return;
+    }
+#endif
+
+    if ((scd->chr->opened && connected) ||
+        (!scd->chr->opened && !connected)) {
+        return;
+    }
+
+    qemu_chr_event(scd->chr, connected ? CHR_EVENT_OPENED : CHR_EVENT_CLOSED);
+}
+
 static SpiceCharDeviceInterface vmc_interface = {
     .base.type          = SPICE_INTERFACE_CHAR_DEVICE,
     .base.description   = "spice virtual channel char device",
     .base.major_version = SPICE_INTERFACE_CHAR_DEVICE_MAJOR,
     .base.minor_version = SPICE_INTERFACE_CHAR_DEVICE_MINOR,
+    .state              = vmc_state,
     .write              = vmc_write,
     .read               = vmc_read,
 };
@@ -222,7 +250,12 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
     chr->chr_guest_close = spice_chr_guest_close;
     s->unblock_timer = qemu_new_timer_ms(vm_clock, spice_chr_unblock, s);
 
-    qemu_chr_generic_open(chr);
+#if SPICE_SERVER_VERSION < 0x000901
+    /* See comment in vmc_state() */
+    if (strcmp(subtype, "vdagent") == 0) {
+        qemu_chr_generic_open(chr);
+    }
+#endif
 
     *_chr = chr;
     return 0;
-- 
1.7.5.1

[Qemu-devel] [PATCH 3/5] usb-redir: Call qemu_chr_guest_open/close

[Hans de Goede]

To let the chardev now we're ready start receiving data. This is necessary
with the spicevmc chardev to get it registered with the spice-server.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 6932beb..9ce2c8b 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -839,6 +839,8 @@ static int usbredir_initfn(USBDevice *udev)
     /* We'll do the attach once we receive the speed from the usb-host */
     udev->auto_attach = 0;
 
+    /* Let the other side know we are ready */
+    qemu_chr_guest_open(dev->cs);
     qemu_chr_add_handlers(dev->cs, &usbredir_chr_handlers, dev);
 
     return 0;
@@ -861,6 +863,7 @@ static void usbredir_handle_destroy(USBDevice *udev)
 {
     USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
 
+    qemu_chr_guest_close(dev->cs);
     qemu_chr_close(dev->cs);
     /* Note must be done after qemu_chr_close, as that causes a close event */
     qemu_bh_delete(dev->open_close_bh);
-- 
1.7.5.1

Re: [Qemu-devel] [PATCH 3/5] usb-redir: Call qemu_chr_guest_open/close

[Anthony Liguori]

On 08/11/2011 07:25 AM, Hans de Goede wrote:
> To let the chardev now we're ready start receiving data. This is necessary
> with the spicevmc chardev to get it registered with the spice-server.
>
> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
> ---
>   usb-redir.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/usb-redir.c b/usb-redir.c
> index 6932beb..9ce2c8b 100644
> --- a/usb-redir.c
> +++ b/usb-redir.c
> @@ -839,6 +839,8 @@ static int usbredir_initfn(USBDevice *udev)
>       /* We'll do the attach once we receive the speed from the usb-host */
>       udev->auto_attach = 0;
>
> +    /* Let the other side know we are ready */
> +    qemu_chr_guest_open(dev->cs);
>       qemu_chr_add_handlers(dev->cs,&usbredir_chr_handlers, dev);

Not sure what tree this is against, but it's not qemu.git.

Regards,

Anthony Liguori

>
>       return 0;
> @@ -861,6 +863,7 @@ static void usbredir_handle_destroy(USBDevice *udev)
>   {
>       USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
>
> +    qemu_chr_guest_close(dev->cs);
>       qemu_chr_close(dev->cs);
>       /* Note must be done after qemu_chr_close, as that causes a close event */
>       qemu_bh_delete(dev->open_close_bh);

[Qemu-devel] [PATCH 4/5] usb-redir: Device disconnect + re-connect robustness fixes

[Hans de Goede]

These fixes mainly target the other side sending some (error status)
packets after a disconnect packet. In some cases these would get queued
up and then reported to the controller when a new device gets connected.

* Fully reset device state on disconnect
* Don't allow a connect message when already connected
* Ignore iso and interrupt status messages when disconnected

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 9ce2c8b..6d8f986 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -905,6 +905,11 @@ static void usbredir_device_connect(void *priv,
 {
     USBRedirDevice *dev = priv;
 
+    if (qemu_timer_pending(dev->attach_timer) || dev->dev.attached) {
+        ERROR("Received device connect while already connected\n");
+        return;
+    }
+
     switch (device_connect->speed) {
     case usb_redir_speed_low:
         DPRINTF("attaching low speed device\n");
@@ -933,19 +938,26 @@ static void usbredir_device_connect(void *priv,
 static void usbredir_device_disconnect(void *priv)
 {
     USBRedirDevice *dev = priv;
+    int i;
 
     /* Stop any pending attaches */
     qemu_del_timer(dev->attach_timer);
 
     if (dev->dev.attached) {
         usb_device_detach(&dev->dev);
-        usbredir_cleanup_device_queues(dev);
         /*
          * Delay next usb device attach to give the guest a chance to see
          * see the detach / attach in case of quick close / open succession
          */
         dev->next_attach_time = qemu_get_clock_ms(vm_clock) + 200;
     }
+
+    /* Reset state so that the next dev connected starts with a clean slate */
+    usbredir_cleanup_device_queues(dev);
+    memset(dev->endpoint, 0, sizeof(dev->endpoint));
+    for (i = 0; i < MAX_ENDPOINTS; i++) {
+        QTAILQ_INIT(&dev->endpoint[i].bufpq);
+    }
 }
 
 static void usbredir_interface_info(void *priv,
@@ -1037,6 +1049,10 @@ static void usbredir_iso_stream_status(void *priv, uint32_t id,
     DPRINTF("iso status %d ep %02X id %u\n", iso_stream_status->status,
             ep, id);
 
+    if (!dev->dev.attached) {
+        return;
+    }
+
     dev->endpoint[EP2I(ep)].iso_error = iso_stream_status->status;
     if (iso_stream_status->status == usb_redir_stall) {
         DPRINTF("iso stream stopped by peer ep %02X\n", ep);
@@ -1054,6 +1070,10 @@ static void usbredir_interrupt_receiving_status(void *priv, uint32_t id,
     DPRINTF("interrupt recv status %d ep %02X id %u\n",
             interrupt_receiving_status->status, ep, id);
 
+    if (!dev->dev.attached) {
+        return;
+    }
+
     dev->endpoint[EP2I(ep)].interrupt_error =
         interrupt_receiving_status->status;
     if (interrupt_receiving_status->status == usb_redir_stall) {
-- 
1.7.5.1

[Qemu-devel] [PATCH 5/5] usb-redir: Don't try to write to the chardev after a close event

[Hans de Goede]

Sicne we handle close async in a bh, do_write and thus write can get
called after receiving a close event. This patch adds a check to
the usb-redir write callback to not do a qemu_chr_write on a closed
chardev.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index 6d8f986..732ddab 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -226,7 +226,7 @@ static int usbredir_write(void *priv, uint8_t *data, int count)
     USBRedirDevice *dev = priv;
     int r;
 
-    if (dev->cs->write_blocked) {
+    if (!dev->cs->opened || dev->cs->write_blocked) {
         return 0;
     }
 
-- 
1.7.5.1

Re: [Qemu-devel] [PATCH 1/5] qemu-char: make qemu_chr_event public

[Anthony Liguori]

On 08/11/2011 07:25 AM, Hans de Goede wrote:
> Make qemu_chr_event public so that it can be used by chardev code
> which lives outside of qemu-char.c

Normally, qemu_chr_generic_open() would be used to do this.  Of course, 
there is no generic_close().

Are you sure you don't need the BH indirection?

Regards,

Anthony Liguori

>
> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
> ---
>   qemu-char.c |    2 +-
>   qemu-char.h |    1 +
>   2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 8d39500..5d5a6d5 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -119,7 +119,7 @@ static void char_write_unblocked(void *opaque)
>       chr->chr_write_unblocked(chr->handler_opaque);
>   }
>
> -static void qemu_chr_event(CharDriverState *s, int event)
> +void qemu_chr_event(CharDriverState *s, int event)
>   {
>       /* Keep track if the char device is open */
>       switch (event) {
> diff --git a/qemu-char.h b/qemu-char.h
> index 68e7b5b..77ad62d 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -107,6 +107,7 @@ int qemu_chr_ioctl(CharDriverState *s, int cmd, void *arg);
>   void qemu_chr_generic_open(CharDriverState *s);
>   int qemu_chr_can_read(CharDriverState *s);
>   void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
> +void qemu_chr_event(CharDriverState *s, int event);
>   int qemu_chr_get_msgfd(CharDriverState *s);
>   void qemu_chr_accept_input(CharDriverState *s);
>   int qemu_chr_add_client(CharDriverState *s, int fd);

Re: [Qemu-devel] [PATCH 1/5] qemu-char: make qemu_chr_event public

[Hans de Goede]

Hi,

On 08/12/2011 03:57 PM, Anthony Liguori wrote:
> On 08/11/2011 07:25 AM, Hans de Goede wrote:
>> Make qemu_chr_event public so that it can be used by chardev code
>> which lives outside of qemu-char.c
>
> Normally, qemu_chr_generic_open() would be used to do this. Of course, there is no generic_close().
>
> Are you sure you don't need the BH indirection?

A bit of a late reply (I was / am still waiting to see how the new
improved chardev code ends up). Things seem to work fine without
the BH indirection for all the spice cases I've tested (agent and usbredir).

But it might indeed be a good idea to keep the BH indirection, so we
would need some way to have the BH indirection for close to, options:

1) DIY in spice-qemu-char.c
2) Add a generic_close function

I would prefer 2, what do you think?

Thanks & Regards,

Hans