[Qemu-devel] [PATCHES for 1.0] various spice usb-redir integration patches

[Qemu-devel] [PATCHES for 1.0] various spice usb-redir integration patches

[Hans de Goede]

Hi All,

Sorry for sending these in so late, I send most of them in before a long time
ago, but then they got stuck on waiting for the big chardev rewrite. Since it
seems clear now that the big chardev rewrite won't happen before 1.0, I would
like to get these into 1.0 as is.

Thanks & Regards,

Hans

[Qemu-devel] [PATCH 1/5] qemu-char: rename qemu_chr_event to qemu_chr_be_event and make it public

[Hans de Goede]

Rename qemu_chr_event to qemu_chr_be_event, since it is only to be
called by backends and make it 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 |   26 +++++++++++++-------------
 qemu-char.h |   10 ++++++++++
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index b562bf8..27abcb9 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -106,7 +106,7 @@
 static QTAILQ_HEAD(CharDriverStateHead, CharDriverState) chardevs =
     QTAILQ_HEAD_INITIALIZER(chardevs);
 
-static void qemu_chr_event(CharDriverState *s, int event)
+void qemu_chr_be_event(CharDriverState *s, int event)
 {
     /* Keep track if the char device is open */
     switch (event) {
@@ -126,7 +126,7 @@ static void qemu_chr_event(CharDriverState *s, int event)
 static void qemu_chr_generic_open_bh(void *opaque)
 {
     CharDriverState *s = opaque;
-    qemu_chr_event(s, CHR_EVENT_OPENED);
+    qemu_chr_be_event(s, CHR_EVENT_OPENED);
     qemu_bh_delete(s->bh);
     s->bh = NULL;
 }
@@ -359,7 +359,7 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
             bdrv_commit_all();
             break;
         case 'b':
-            qemu_chr_event(chr, CHR_EVENT_BREAK);
+            qemu_chr_be_event(chr, CHR_EVENT_BREAK);
             break;
         case 'c':
             /* Switch to the next registered device */
@@ -580,7 +580,7 @@ static void fd_chr_read(void *opaque)
     if (size == 0) {
         /* FD has been closed. Remove it from the active list.  */
         qemu_set_fd_handler2(s->fd_in, NULL, NULL, NULL, NULL);
-        qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
         return;
     }
     if (size > 0) {
@@ -613,7 +613,7 @@ static void fd_chr_close(struct CharDriverState *chr)
     }
 
     g_free(s);
-    qemu_chr_event(chr, CHR_EVENT_CLOSED);
+    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
 /* open a character device to a unix fd */
@@ -715,7 +715,7 @@ static void stdio_read(void *opaque)
     if (size == 0) {
         /* stdin has been closed. Remove it from the active list.  */
         qemu_set_fd_handler2(0, NULL, NULL, NULL, NULL);
-        qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
         return;
     }
     if (size > 0) {
@@ -977,7 +977,7 @@ static void pty_chr_close(struct CharDriverState *chr)
     qemu_del_timer(s->timer);
     qemu_free_timer(s->timer);
     g_free(s);
-    qemu_chr_event(chr, CHR_EVENT_CLOSED);
+    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
 static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
@@ -1355,7 +1355,7 @@ static void pp_close(CharDriverState *chr)
     ioctl(fd, PPRELEASE);
     close(fd);
     g_free(drv);
-    qemu_chr_event(chr, CHR_EVENT_CLOSED);
+    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
 static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
@@ -1500,7 +1500,7 @@ static void win_chr_close(CharDriverState *chr)
     else
         qemu_del_polling_cb(win_chr_poll, chr);
 
-    qemu_chr_event(chr, CHR_EVENT_CLOSED);
+    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
 static int win_chr_init(CharDriverState *chr, const char *filename)
@@ -2108,7 +2108,7 @@ static void udp_chr_close(CharDriverState *chr)
         closesocket(s->fd);
     }
     g_free(s);
-    qemu_chr_event(chr, CHR_EVENT_CLOSED);
+    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
 static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
@@ -2213,7 +2213,7 @@ static void tcp_chr_process_IAC_bytes(CharDriverState *chr,
             } else {
                 if ((unsigned char)buf[i] == IAC_BREAK && s->do_telnetopt == 2) {
                     /* Handle IAC break commands by sending a serial break */
-                    qemu_chr_event(chr, CHR_EVENT_BREAK);
+                    qemu_chr_be_event(chr, CHR_EVENT_BREAK);
                     s->do_telnetopt++;
                 }
                 s->do_telnetopt++;
@@ -2321,7 +2321,7 @@ static void tcp_chr_read(void *opaque)
         qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
         closesocket(s->fd);
         s->fd = -1;
-        qemu_chr_event(chr, CHR_EVENT_CLOSED);
+        qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
     } else if (size > 0) {
         if (s->do_telnetopt)
             tcp_chr_process_IAC_bytes(chr, s, buf, &size);
@@ -2433,7 +2433,7 @@ static void tcp_chr_close(CharDriverState *chr)
         closesocket(s->listen_fd);
     }
     g_free(s);
-    qemu_chr_event(chr, CHR_EVENT_CLOSED);
+    qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
 static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
diff --git a/qemu-char.h b/qemu-char.h
index 7efcf99..8ca1e2d 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -212,6 +212,16 @@ int qemu_chr_be_can_write(CharDriverState *s);
  */
 void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len);
 
+
+/**
+ * @qemu_chr_be_event:
+ *
+ * Send an event from the back end to the front end.
+ *
+ * @event the event to send
+ */
+void qemu_chr_be_event(CharDriverState *s, int event);
+
 void qemu_chr_add_handlers(CharDriverState *s,
                            IOCanReadHandler *fd_can_read,
                            IOReadHandler *fd_read,
-- 
1.7.7.3

[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.

Notes:

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

2) This code deliberately sends the events immediately rather then from a
bh. This is done this way because:
a) There is no need to do it from a bh; and
b) Doing it from a bh actually causes issues because the spice-server may send
data immediately after the open and when the open runs from a bh, then
qemu_chr_be_can_write will return 0 for the first write which the spice-server
does not expect, when this happens the spice-server will never retry the write
causing communication to stall.

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

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index ac52202..7e8eaa9 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -69,11 +69,40 @@ 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_be_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,
 };
@@ -197,7 +226,12 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
     chr->chr_guest_open = spice_chr_guest_open;
     chr->chr_guest_close = spice_chr_guest_close;
 
-    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.7.3

[Qemu-devel] [PATCH 3/5] usb-redir: Call qemu_chr_fe_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 c74b156..9e13410 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -814,6 +814,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 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);
 
@@ -837,6 +839,7 @@ 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->open_close_bh);
-- 
1.7.7.3

[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 9e13410..24fdd31 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -881,6 +881,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");
@@ -909,19 +914,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,
@@ -1013,6 +1025,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);
@@ -1030,6 +1046,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.7.3

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

[Hans de Goede]

Since 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 call qemu_chr_fe_write on a closed
backend.

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

diff --git a/usb-redir.c b/usb-redir.c
index 24fdd31..fb91c92 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -225,6 +225,10 @@ static int usbredir_write(void *priv, uint8_t *data, int count)
 {
     USBRedirDevice *dev = priv;
 
+    if (!dev->cs->opened) {
+        return 0;
+    }
+
     return qemu_chr_fe_write(dev->cs, data, count);
 }
 
-- 
1.7.7.3

Re: [Qemu-devel] [PATCHES for 1.0] various spice usb-redir integration patches

[Anthony Liguori]

On 11/19/2011 03:22 AM, Hans de Goede wrote:
> Hi All,
>
> Sorry for sending these in so late, I send most of them in before a long time
> ago, but then they got stuck on waiting for the big chardev rewrite. Since it
> seems clear now that the big chardev rewrite won't happen before 1.0, I would
> like to get these into 1.0 as is.

This seems pretty well isolated.  I'm not going to put this in -rc3 because I'd 
like to see Gerd at least Ack 2/5.  Once it has an Ack, I'll take it either for 
-rc4 or for 1.0.z.

Regards,

Anthony Liguori

>
> Thanks&  Regards,
>
> Hans
>
>
>

Re: [Qemu-devel] [PATCHES for 1.0] various spice usb-redir integration patches

[Gerd Hoffmann]

On 11/21/11 22:05, Anthony Liguori wrote:
> On 11/19/2011 03:22 AM, Hans de Goede wrote:
>> Hi All,
>>
>> Sorry for sending these in so late, I send most of them in before a
>> long time
>> ago, but then they got stuck on waiting for the big chardev rewrite.
>> Since it
>> seems clear now that the big chardev rewrite won't happen before 1.0,
>> I would
>> like to get these into 1.0 as is.
> 
> This seems pretty well isolated.  I'm not going to put this in -rc3
> because I'd like to see Gerd at least Ack 2/5.  Once it has an Ack, I'll
> take it either for -rc4 or for 1.0.z.

Series looks good.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

Re: [Qemu-devel] [PATCHES for 1.0] various spice usb-redir integration patches

[Anthony Liguori]

On 11/19/2011 03:22 AM, Hans de Goede wrote:
> Hi All,
>
> Sorry for sending these in so late, I send most of them in before a long time
> ago, but then they got stuck on waiting for the big chardev rewrite. Since it
> seems clear now that the big chardev rewrite won't happen before 1.0, I would
> like to get these into 1.0 as is.

Applied all.  Thanks.

Regards,

Anthony Liguori

>
> Thanks&  Regards,
>
> Hans
>
>
>

[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 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