[Qemu-devel] [PATCH 0/7] Spice / usb-redir chardev flowcontrol patches v3

[Qemu-devel] [PATCH 0/7] Spice / usb-redir chardev flowcontrol patches v3

[Hans de Goede]

I think you may already have these in a local branch, but since I've not
yet seen a pull request for them, here is another resend.

Here is v3 of the series adding watch support to the spicevmc chardev backend
and flowcontrol support to the usb-redir device. It also includes a few
virtio-consoled bugfixes which were found during the development of this
series.

Changes in v2:
-Address Amit's review of "[PATCH 2/8] virtio-console: Remove any pending 
watches on close"
-Drop 2 spice-qemu-char cleanup patches as Gerd has added them to his v5 series
-Add a (forward ported) patch from Alon Levy to fix a long standing
 spice-qemu-char migration issue in a way which should be acceptable upstream

Changes in v3:
-Rebased on top of latest master (some trivial manual conflict resolution)
-Dropped Alon's patch for the spice-qemu-char migration issue, this is fixed
 differently in another series which is already par of the latest master
-Add a new patch:
  usb-serial: Remove double call to qemu_chr_add_handlers( NULL )
-Fixed a checkpatch issue in:
  usb-redir: Add flow control support

Regards,

Hans

Alon Levy (1):
  spice-qemu-char: Remove intermediate buffer

Hans de Goede (6):
  virtio-console: Also throttle when less was written then requested
  virtio-console: Remove any pending watches on close
  spice-qemu-char: Remove #ifdef-ed code for old spice-server compat
  spice-qemu-char: Add watch support
  usb-redir: Add flow control support
  usb-serial: Remove double call to qemu_chr_add_handlers( NULL )

 hw/usb/dev-serial.c |   9 -----
 hw/usb/redirect.c   |  33 +++++++++++++++-
 hw/virtio-console.c |  29 ++++++++++++--
 spice-qemu-char.c   | 106 +++++++++++++++++++++++++++++++++-------------------
 4 files changed, 124 insertions(+), 53 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/7] virtio-console: Also throttle when less was written then requested

[Hans de Goede]

This is necessary so that we get properly woken up to write the rest.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 284180f..015947c 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -47,7 +47,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
     ret = qemu_chr_fe_write(vcon->chr, buf, len);
     trace_virtio_console_flush_buf(port->id, len, ret);
 
-    if (ret <= 0) {
+    if (ret < len) {
         VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
 
         /*
@@ -56,7 +56,8 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
          * we had a finer-grained message, like -EPIPE, we could close
          * this connection.
          */
-        ret = 0;
+        if (ret < 0)
+            ret = 0;
         if (!k->is_console) {
             virtio_serial_throttle_port(port, true);
             qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT, chr_write_unblocked,
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/7] virtio-console: Remove any pending watches on close

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/virtio-console.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 015947c..e28c54f 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -18,6 +18,7 @@
 typedef struct VirtConsole {
     VirtIOSerialPort port;
     CharDriverState *chr;
+    guint watch;
 } VirtConsole;
 
 /*
@@ -29,6 +30,7 @@ static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
 {
     VirtConsole *vcon = opaque;
 
+    vcon->watch = 0;
     virtio_serial_throttle_port(&vcon->port, false);
     return FALSE;
 }
@@ -60,8 +62,10 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
             ret = 0;
         if (!k->is_console) {
             virtio_serial_throttle_port(port, true);
-            qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT, chr_write_unblocked,
-                                  vcon);
+            if (!vcon->watch) {
+                vcon->watch = qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT,
+                                                    chr_write_unblocked, vcon);
+            }
         }
     }
     return ret;
@@ -105,6 +109,10 @@ static void chr_event(void *opaque, int event)
         virtio_serial_open(&vcon->port);
         break;
     case CHR_EVENT_CLOSED:
+        if (vcon->watch) {
+            g_source_remove(vcon->watch);
+            vcon->watch = 0;
+        }
         virtio_serial_close(&vcon->port);
         break;
     }
@@ -129,6 +137,17 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
     return 0;
 }
 
+static int virtconsole_exitfn(VirtIOSerialPort *port)
+{
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    if (vcon->watch) {
+        g_source_remove(vcon->watch);
+    }
+
+    return 0;
+}
+
 static Property virtconsole_properties[] = {
     DEFINE_PROP_CHR("chardev", VirtConsole, chr),
     DEFINE_PROP_END_OF_LIST(),
@@ -141,6 +160,7 @@ static void virtconsole_class_init(ObjectClass *klass, void *data)
 
     k->is_console = true;
     k->init = virtconsole_initfn;
+    k->exit = virtconsole_exitfn;
     k->have_data = flush_buf;
     k->set_guest_connected = set_guest_connected;
     dc->props = virtconsole_properties;
-- 
1.8.1.4

[Qemu-devel] [PATCH 3/7] spice-qemu-char: Remove #ifdef-ed code for old spice-server compat

[Hans de Goede]

We now require spice-server to be >= 0.12.0 so this is no longer needed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 spice-qemu-char.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 535f955..c4f81cf 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -85,21 +85,6 @@ 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->be_open && connected) ||
         (!scd->chr->be_open && !connected)) {
         return;
@@ -224,7 +209,6 @@ static CharDriverState *chr_open(const char *subtype)
 
 CharDriverState *qemu_chr_open_spice_vmc(const char *type)
 {
-    CharDriverState *chr;
     const char **psubtype = spice_server_char_device_recognized_subtypes();
 
     if (type == NULL) {
@@ -243,16 +227,7 @@ CharDriverState *qemu_chr_open_spice_vmc(const char *type)
         return NULL;
     }
 
-    chr = chr_open(type);
-
-#if SPICE_SERVER_VERSION < 0x000901
-    /* See comment in vmc_state() */
-    if (strcmp(type, "vdagent") == 0) {
-        qemu_chr_generic_open(chr);
-    }
-#endif
-
-    return chr;
+    return chr_open(type);
 }
 
 #if SPICE_SERVER_VERSION >= 0x000c02
-- 
1.8.1.4

[Qemu-devel] [PATCH 4/7] spice-qemu-char: Add watch support

[Hans de Goede]

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

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index c4f81cf..097a8c8 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -13,12 +13,18 @@ typedef struct SpiceCharDriver {
     SpiceCharDeviceInstance     sin;
     char                  *subtype;
     bool                  active;
+    bool                  blocked;
     uint8_t               *buffer;
     uint8_t               *datapos;
     ssize_t               bufsize, datalen;
     QLIST_ENTRY(SpiceCharDriver) next;
 } SpiceCharDriver;
 
+typedef struct SpiceCharSource {
+    GSource               source;
+    SpiceCharDriver       *scd;
+} SpiceCharSource;
+
 static QLIST_HEAD(, SpiceCharDriver) spice_chars =
     QLIST_HEAD_INITIALIZER(spice_chars);
 
@@ -54,9 +60,10 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t *buf, int len)
         scd->datapos += bytes;
         scd->datalen -= bytes;
         assert(scd->datalen >= 0);
-        if (scd->datalen == 0) {
-            scd->datapos = 0;
-        }
+    }
+    if (scd->datalen == 0) {
+        scd->datapos = 0;
+        scd->blocked = false;
     }
     trace_spice_vmc_read(bytes, len);
     return bytes;
@@ -129,10 +136,54 @@ static void vmc_unregister_interface(SpiceCharDriver *scd)
     trace_spice_vmc_unregister_interface(scd);
 }
 
+static gboolean spice_char_source_prepare(GSource *source, gint *timeout)
+{
+    SpiceCharSource *src = (SpiceCharSource *)source;
+
+    *timeout = -1;
+
+    return !src->scd->blocked;
+}
+
+static gboolean spice_char_source_check(GSource *source)
+{
+    SpiceCharSource *src = (SpiceCharSource *)source;
+
+    return !src->scd->blocked;
+}
+
+static gboolean spice_char_source_dispatch(GSource *source,
+    GSourceFunc callback, gpointer user_data)
+{
+    GIOFunc func = (GIOFunc)callback;
+
+    return func(NULL, G_IO_OUT, user_data);
+}
+
+GSourceFuncs SpiceCharSourceFuncs = {
+    .prepare  = spice_char_source_prepare,
+    .check    = spice_char_source_check,
+    .dispatch = spice_char_source_dispatch,
+};
+
+static GSource *spice_chr_add_watch(CharDriverState *chr, GIOCondition cond)
+{
+    SpiceCharDriver *scd = chr->opaque;
+    SpiceCharSource *src;
+
+    assert(cond == G_IO_OUT);
+
+    src = (SpiceCharSource *)g_source_new(&SpiceCharSourceFuncs,
+                                          sizeof(SpiceCharSource));
+    src->scd = scd;
+
+    return (GSource *)src;
+}
 
 static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     SpiceCharDriver *s = chr->opaque;
+    int read_bytes;
 
     assert(s->datalen == 0);
     if (s->bufsize < len) {
@@ -143,7 +194,14 @@ static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
     s->datapos = s->buffer;
     s->datalen = len;
     spice_server_char_device_wakeup(&s->sin);
-    return len;
+    read_bytes = len - s->datalen;
+    if (read_bytes != len) {
+        /* We'll get passed in the unconsumed data with the next call */
+        s->datalen = 0;
+        s->datapos = NULL;
+        s->blocked = true;
+    }
+    return read_bytes;
 }
 
 static void spice_chr_close(struct CharDriverState *chr)
@@ -199,6 +257,7 @@ static CharDriverState *chr_open(const char *subtype)
     s->sin.subtype = g_strdup(subtype);
     chr->opaque = s;
     chr->chr_write = spice_chr_write;
+    chr->chr_add_watch = spice_chr_add_watch;
     chr->chr_close = spice_chr_close;
     chr->chr_set_fe_open = spice_chr_set_fe_open;
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 5/7] spice-qemu-char: Remove intermediate buffer

[Hans de Goede]

From: Alon Levy <alevy@redhat.com>

virtio-serial's buffer is valid when it calls us, and we don't
access it otherwise: vmc_read is only called in response to wakeup,
or else we set datalen=0 and throttle. Then vmc_read is called back,
we return 0 (not accessing the buffer) and set the timer to unthrottle.

Also make datalen int and not ssize_t (to fit spice_chr_write signature).

HdG: Update to apply to spice-qemu-char with new gio-channel based
flowcontrol support.

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

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 097a8c8..7e6bd2d 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -14,9 +14,8 @@ typedef struct SpiceCharDriver {
     char                  *subtype;
     bool                  active;
     bool                  blocked;
-    uint8_t               *buffer;
-    uint8_t               *datapos;
-    ssize_t               bufsize, datalen;
+    const uint8_t         *datapos;
+    int                   datalen;
     QLIST_ENTRY(SpiceCharDriver) next;
 } SpiceCharDriver;
 
@@ -186,12 +185,7 @@ static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
     int read_bytes;
 
     assert(s->datalen == 0);
-    if (s->bufsize < len) {
-        s->bufsize = len;
-        s->buffer = g_realloc(s->buffer, s->bufsize);
-    }
-    memcpy(s->buffer, buf, len);
-    s->datapos = s->buffer;
+    s->datapos = buf;
     s->datalen = len;
     spice_server_char_device_wakeup(&s->sin);
     read_bytes = len - s->datalen;
-- 
1.8.1.4

[Qemu-devel] [PATCH 6/7] usb-redir: Add flow control support

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index d02a7b9..df682d1 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -104,6 +104,8 @@ struct USBRedirDevice {
     /* Data passed from chardev the fd_read cb to the usbredirparser read cb */
     const uint8_t *read_buf;
     int read_buf_size;
+    /* Active chardev-watch-tag */
+    guint watch;
     /* For async handling of close */
     QEMUBH *chardev_close_bh;
     /* To delay the usb attach in case of quick chardev close + open */
@@ -254,9 +256,21 @@ static int usbredir_read(void *priv, uint8_t *data, int count)
     return count;
 }
 
+static gboolean usbredir_write_unblocked(GIOChannel *chan, GIOCondition cond,
+                                         void *opaque)
+{
+    USBRedirDevice *dev = opaque;
+
+    dev->watch = 0;
+    usbredirparser_do_write(dev->parser);
+
+    return FALSE;
+}
+
 static int usbredir_write(void *priv, uint8_t *data, int count)
 {
     USBRedirDevice *dev = priv;
+    int r;
 
     if (!dev->cs->be_open) {
         return 0;
@@ -267,7 +281,17 @@ static int usbredir_write(void *priv, uint8_t *data, int count)
         return 0;
     }
 
-    return qemu_chr_fe_write(dev->cs, data, count);
+    r = qemu_chr_fe_write(dev->cs, data, count);
+    if (r < count) {
+        if (!dev->watch) {
+            dev->watch = qemu_chr_fe_add_watch(dev->cs, G_IO_OUT,
+                                               usbredir_write_unblocked, dev);
+        }
+        if (r < 0) {
+            r = 0;
+        }
+    }
+    return r;
 }
 
 /*
@@ -1085,6 +1109,10 @@ static void usbredir_chardev_close_bh(void *opaque)
         usbredirparser_destroy(dev->parser);
         dev->parser = NULL;
     }
+    if (dev->watch) {
+        g_source_remove(dev->watch);
+        dev->watch = 0;
+    }
 }
 
 static void usbredir_create_parser(USBRedirDevice *dev)
@@ -1317,6 +1345,9 @@ static void usbredir_handle_destroy(USBDevice *udev)
     if (dev->parser) {
         usbredirparser_destroy(dev->parser);
     }
+    if (dev->watch) {
+        g_source_remove(dev->watch);
+    }
 
     free(dev->filter_rules);
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 7/7] usb-serial: Remove double call to qemu_chr_add_handlers( NULL )

[Hans de Goede]

usb-serial has a qdev chardev property, and hw/qdev-properties-system.c
already contains:

static void release_chr(Object *obj, const char *name, void *opaque)
{
    DeviceState *dev = DEVICE(obj);
    Property *prop = opaque;
    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);

    if (*ptr) {
        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
    }
}

So doing the qemu_chr_add_handlers(s->cs, NULL, NULL, NULL, NULL); from
the usb handle_destroy function too will lead to it being done twice, which
will result in a wrong value for cs->avail_connections.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/dev-serial.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 7c314dc..21ddef6 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -410,13 +410,6 @@ static void usb_serial_handle_data(USBDevice *dev, USBPacket *p)
     }
 }
 
-static void usb_serial_handle_destroy(USBDevice *dev)
-{
-    USBSerialState *s = (USBSerialState *)dev;
-
-    qemu_chr_add_handlers(s->cs, NULL, NULL, NULL, NULL);
-}
-
 static int usb_serial_can_read(void *opaque)
 {
     USBSerialState *s = opaque;
@@ -595,7 +588,6 @@ static void usb_serial_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = usb_serial_handle_reset;
     uc->handle_control = usb_serial_handle_control;
     uc->handle_data    = usb_serial_handle_data;
-    uc->handle_destroy = usb_serial_handle_destroy;
     dc->vmsd = &vmstate_usb_serial;
     dc->props = serial_properties;
 }
@@ -623,7 +615,6 @@ static void usb_braille_class_initfn(ObjectClass *klass, void *data)
     uc->handle_reset   = usb_serial_handle_reset;
     uc->handle_control = usb_serial_handle_control;
     uc->handle_data    = usb_serial_handle_data;
-    uc->handle_destroy = usb_serial_handle_destroy;
     dc->vmsd = &vmstate_usb_serial;
     dc->props = braille_properties;
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/7] virtio-console: Also throttle when less was written then requested

[Amit Shah]

On (Thu) 28 Mar 2013 [14:28:11], Hans de Goede wrote:
> This is necessary so that we get properly woken up to write the rest.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-console.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index 284180f..015947c 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -47,7 +47,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
>      ret = qemu_chr_fe_write(vcon->chr, buf, len);
>      trace_virtio_console_flush_buf(port->id, len, ret);
>  
> -    if (ret <= 0) {
> +    if (ret < len) {

Oops, there's a conversion bug here: len is size_t, and ret is
ssize_t.  Both need to be the same type.

Since we're not returning negative, ret should be changed to size_t.
The function signature changes too, but that's alright.

>          VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>  
>          /*
> @@ -56,7 +56,8 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
>           * we had a finer-grained message, like -EPIPE, we could close
>           * this connection.
>           */
> -        ret = 0;
> +        if (ret < 0)
> +            ret = 0;

And there need to be braces here.

		Amit

Re: [Qemu-devel] [PATCH 1/7] virtio-console: Also throttle when less was written then requested

[Hans de Goede]

Hi,

On 03/29/2013 10:31 AM, Amit Shah wrote:
> On (Thu) 28 Mar 2013 [14:28:11], Hans de Goede wrote:
>> This is necessary so that we get properly woken up to write the rest.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Amit Shah <amit.shah@redhat.com>
>> ---
>>   hw/virtio-console.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
>> index 284180f..015947c 100644
>> --- a/hw/virtio-console.c
>> +++ b/hw/virtio-console.c
>> @@ -47,7 +47,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
>>       ret = qemu_chr_fe_write(vcon->chr, buf, len);
>>       trace_virtio_console_flush_buf(port->id, len, ret);
>>
>> -    if (ret <= 0) {
>> +    if (ret < len) {
>
> Oops, there's a conversion bug here: len is size_t, and ret is
> ssize_t.  Both need to be the same type.
>
> Since we're not returning negative, ret should be changed to size_t.
> The function signature changes too, but that's alright.

Erm changing ret to a size_t will break things, since qemu_chr_fe_write can
return < 0 and storing that into an unsigned will change it into a very
large value instead ...

A better fix would be to change len to ssize_t. Although that will cause
issues if we ever get a single buf which is larger then 2^31 - 1. Note
we already have that problem since qemu_chr_fe_write already cannot handle
such large buffers...


>
>>           VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
>>
>>           /*
>> @@ -56,7 +56,8 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
>>            * we had a finer-grained message, like -EPIPE, we could close
>>            * this connection.
>>            */
>> -        ret = 0;
>> +        if (ret < 0)
>> +            ret = 0;
>
> And there need to be braces here.

Ack.

Regards,

Hans

Re: [Qemu-devel] [PATCH 1/7] virtio-console: Also throttle when less was written then requested

[Amit Shah]

On (Mon) 01 Apr 2013 [18:00:55], Hans de Goede wrote:
> Hi,
> 
> On 03/29/2013 10:31 AM, Amit Shah wrote:
> >On (Thu) 28 Mar 2013 [14:28:11], Hans de Goede wrote:
> >>This is necessary so that we get properly woken up to write the rest.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>Acked-by: Amit Shah <amit.shah@redhat.com>
> >>---
> >>  hw/virtio-console.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> >>index 284180f..015947c 100644
> >>--- a/hw/virtio-console.c
> >>+++ b/hw/virtio-console.c
> >>@@ -47,7 +47,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
> >>      ret = qemu_chr_fe_write(vcon->chr, buf, len);
> >>      trace_virtio_console_flush_buf(port->id, len, ret);
> >>
> >>-    if (ret <= 0) {
> >>+    if (ret < len) {
> >
> >Oops, there's a conversion bug here: len is size_t, and ret is
> >ssize_t.  Both need to be the same type.
> >
> >Since we're not returning negative, ret should be changed to size_t.
> >The function signature changes too, but that's alright.
> 
> Erm changing ret to a size_t will break things, since qemu_chr_fe_write can
> return < 0 and storing that into an unsigned will change it into a very
> large value instead ...

Oh, definitely.  I meant changing the return type of flush_buf(), but
that doesn't help matters much...

> A better fix would be to change len to ssize_t. Although that will cause
> issues if we ever get a single buf which is larger then 2^31 - 1. Note
> we already have that problem since qemu_chr_fe_write already cannot handle
> such large buffers...

Yep.  Do you want to do that as part of this series?

		Amit

Re: [Qemu-devel] [PATCH 1/7] virtio-console: Also throttle when less was written then requested

[Hans de Goede]

Hi,

On 04/02/2013 12:40 PM, Amit Shah wrote:
> On (Mon) 01 Apr 2013 [18:00:55], Hans de Goede wrote:
>> Hi,
>>
>> On 03/29/2013 10:31 AM, Amit Shah wrote:
>>> On (Thu) 28 Mar 2013 [14:28:11], Hans de Goede wrote:
>>>> This is necessary so that we get properly woken up to write the rest.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> Acked-by: Amit Shah <amit.shah@redhat.com>
>>>> ---
>>>>   hw/virtio-console.c | 5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
>>>> index 284180f..015947c 100644
>>>> --- a/hw/virtio-console.c
>>>> +++ b/hw/virtio-console.c
>>>> @@ -47,7 +47,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
>>>>       ret = qemu_chr_fe_write(vcon->chr, buf, len);
>>>>       trace_virtio_console_flush_buf(port->id, len, ret);
>>>>
>>>> -    if (ret <= 0) {
>>>> +    if (ret < len) {
>>>
>>> Oops, there's a conversion bug here: len is size_t, and ret is
>>> ssize_t.  Both need to be the same type.
>>>
>>> Since we're not returning negative, ret should be changed to size_t.
>>> The function signature changes too, but that's alright.
>>
>> Erm changing ret to a size_t will break things, since qemu_chr_fe_write can
>> return < 0 and storing that into an unsigned will change it into a very
>> large value instead ...
>
> Oh, definitely.  I meant changing the return type of flush_buf(), but
> that doesn't help matters much...
>
>> A better fix would be to change len to ssize_t. Although that will cause
>> issues if we ever get a single buf which is larger then 2^31 - 1. Note
>> we already have that problem since qemu_chr_fe_write already cannot handle
>> such large buffers...
>
> Yep.  Do you want to do that as part of this series?

Yes, I'll send a v2 of this patch incorporating the change for Gerd to
pick up.

Regards,

Hans

[Qemu-devel] [PATCH 2/7] virtio-console: Remove any pending watches on close

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/virtio-console.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 61f9ff5..908ec17 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -18,6 +18,7 @@
 typedef struct VirtConsole {
     VirtIOSerialPort port;
     CharDriverState *chr;
+    guint watch;
 } VirtConsole;
 
 /*
@@ -29,6 +30,7 @@ static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
 {
     VirtConsole *vcon = opaque;
 
+    vcon->watch = 0;
     virtio_serial_throttle_port(&vcon->port, false);
     return FALSE;
 }
@@ -61,8 +63,10 @@ static ssize_t flush_buf(VirtIOSerialPort *port,
             ret = 0;
         if (!k->is_console) {
             virtio_serial_throttle_port(port, true);
-            qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT, chr_write_unblocked,
-                                  vcon);
+            if (!vcon->watch) {
+                vcon->watch = qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT,
+                                                    chr_write_unblocked, vcon);
+            }
         }
     }
     return ret;
@@ -106,6 +110,10 @@ static void chr_event(void *opaque, int event)
         virtio_serial_open(&vcon->port);
         break;
     case CHR_EVENT_CLOSED:
+        if (vcon->watch) {
+            g_source_remove(vcon->watch);
+            vcon->watch = 0;
+        }
         virtio_serial_close(&vcon->port);
         break;
     }
@@ -130,6 +138,17 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
     return 0;
 }
 
+static int virtconsole_exitfn(VirtIOSerialPort *port)
+{
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    if (vcon->watch) {
+        g_source_remove(vcon->watch);
+    }
+
+    return 0;
+}
+
 static Property virtconsole_properties[] = {
     DEFINE_PROP_CHR("chardev", VirtConsole, chr),
     DEFINE_PROP_END_OF_LIST(),
@@ -142,6 +161,7 @@ static void virtconsole_class_init(ObjectClass *klass, void *data)
 
     k->is_console = true;
     k->init = virtconsole_initfn;
+    k->exit = virtconsole_exitfn;
     k->have_data = flush_buf;
     k->set_guest_connected = set_guest_connected;
     dc->props = virtconsole_properties;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/7] virtio-console: Remove any pending watches on close

[Hans de Goede]

Hi,

On 03/14/2013 08:35 AM, Amit Shah wrote:
> On (Wed) 13 Mar 2013 [14:59:42], Hans de Goede wrote:
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   hw/virtio-console.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
>> index 1d87c5b..ec0f91b 100644
>> --- a/hw/virtio-console.c
>> +++ b/hw/virtio-console.c
>> @@ -18,6 +18,7 @@
>>   typedef struct VirtConsole {
>>       VirtIOSerialPort port;
>>       CharDriverState *chr;
>> +    guint watch;
>>   } VirtConsole;
>>
>>   /*
>> @@ -29,6 +30,7 @@ static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
>>   {
>>       VirtConsole *vcon = opaque;
>>
>> +    vcon->watch = 0;
>>       virtio_serial_throttle_port(&vcon->port, false);
>>       return FALSE;
>>   }
>> @@ -60,8 +62,10 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
>>               ret = 0;
>>           if (!k->is_console) {
>>               virtio_serial_throttle_port(port, true);
>> -            qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT, chr_write_unblocked,
>> -                                  vcon);
>> +            if (!vcon->watch) {
>> +                vcon->watch = qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT,
>> +                                                    chr_write_unblocked, vcon);
>> +            }
>>           }
>>       }
>>       return ret;
>> @@ -86,6 +90,10 @@ static void guest_close(VirtIOSerialPort *port)
>>       if (!vcon->chr) {
>>           return;
>>       }
>> +    if (vcon->watch) {
>> +        g_source_remove(vcon->watch);
>> +        vcon->watch = 0;
>> +    }
>>       qemu_chr_fe_close(vcon->chr);
>>   }
>
> Even if a guest closed the connection, we want the data the guest sent
> before the close to go out.
>
> What's the reason to not do that?

There is no reason, this was a bad attempt by me to deal with closing
the watch on virtio-port hot-unplug. I've removed this in my
local version of the patchset and instead added an exit callback
to deal with virtio-port hot-unplug the proper way.

I still have a few other patches for spice-qemu-char.c I'm working on,
once those are finished, I'll resend the series with this fixed and
rebased on top of Gerd Hoffman's chardev.v5 tree.

Regards,

Hans

Re: [Qemu-devel] [PATCH 2/7] virtio-console: Remove any pending watches on close

[Amit Shah]

On (Wed) 13 Mar 2013 [14:59:42], Hans de Goede wrote:
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  hw/virtio-console.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index 1d87c5b..ec0f91b 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -18,6 +18,7 @@
>  typedef struct VirtConsole {
>      VirtIOSerialPort port;
>      CharDriverState *chr;
> +    guint watch;
>  } VirtConsole;
>  
>  /*
> @@ -29,6 +30,7 @@ static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
>  {
>      VirtConsole *vcon = opaque;
>  
> +    vcon->watch = 0;
>      virtio_serial_throttle_port(&vcon->port, false);
>      return FALSE;
>  }
> @@ -60,8 +62,10 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
>              ret = 0;
>          if (!k->is_console) {
>              virtio_serial_throttle_port(port, true);
> -            qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT, chr_write_unblocked,
> -                                  vcon);
> +            if (!vcon->watch) {
> +                vcon->watch = qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT,
> +                                                    chr_write_unblocked, vcon);
> +            }
>          }
>      }
>      return ret;
> @@ -86,6 +90,10 @@ static void guest_close(VirtIOSerialPort *port)
>      if (!vcon->chr) {
>          return;
>      }
> +    if (vcon->watch) {
> +        g_source_remove(vcon->watch);
> +        vcon->watch = 0;
> +    }
>      qemu_chr_fe_close(vcon->chr);
>  }

Even if a guest closed the connection, we want the data the guest sent
before the close to go out.

What's the reason to not do that?

> @@ -116,6 +124,10 @@ static void chr_event(void *opaque, int event)
>          virtio_serial_open(&vcon->port);
>          break;
>      case CHR_EVENT_CLOSED:
> +        if (vcon->watch) {
> +            g_source_remove(vcon->watch);
> +            vcon->watch = 0;
> +        }
>          virtio_serial_close(&vcon->port);
>          break;
>      }

Agreed with the rest.


		Amit

[Qemu-devel] [PATCH 2/7] virtio-console: Remove any pending watches on close

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/virtio-console.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 1d87c5b..ec0f91b 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -18,6 +18,7 @@
 typedef struct VirtConsole {
     VirtIOSerialPort port;
     CharDriverState *chr;
+    guint watch;
 } VirtConsole;
 
 /*
@@ -29,6 +30,7 @@ static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
 {
     VirtConsole *vcon = opaque;
 
+    vcon->watch = 0;
     virtio_serial_throttle_port(&vcon->port, false);
     return FALSE;
 }
@@ -60,8 +62,10 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
             ret = 0;
         if (!k->is_console) {
             virtio_serial_throttle_port(port, true);
-            qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT, chr_write_unblocked,
-                                  vcon);
+            if (!vcon->watch) {
+                vcon->watch = qemu_chr_fe_add_watch(vcon->chr, G_IO_OUT,
+                                                    chr_write_unblocked, vcon);
+            }
         }
     }
     return ret;
@@ -86,6 +90,10 @@ static void guest_close(VirtIOSerialPort *port)
     if (!vcon->chr) {
         return;
     }
+    if (vcon->watch) {
+        g_source_remove(vcon->watch);
+        vcon->watch = 0;
+    }
     qemu_chr_fe_close(vcon->chr);
 }
 
@@ -116,6 +124,10 @@ static void chr_event(void *opaque, int event)
         virtio_serial_open(&vcon->port);
         break;
     case CHR_EVENT_CLOSED:
+        if (vcon->watch) {
+            g_source_remove(vcon->watch);
+            vcon->watch = 0;
+        }
         virtio_serial_close(&vcon->port);
         break;
     }
-- 
1.8.1.4