diff mbox

[03/20] char: add IOWatchPoll support

Message ID 9b59ac17b9d0bb3972a73fed04d415f07b391936.1362505276.git.amit.shah@redhat.com
State New
Headers show

Commit Message

Amit Shah March 5, 2013, 5:51 p.m. UTC 
From: Anthony Liguori <aliguori@us.ibm.com>

This is a special GSource that supports CharDriverState style
poll callbacks.

For reviewability and bisectability, this code is #if 0'd out in this
patch to avoid unused warnings since all of the functions are static.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)

Comments

Amit Shah March 29, 2013, 9:53 a.m. UTC | #1 
On (Tue) 05 Mar 2013 [23:21:18], Amit Shah wrote:
> From: Anthony Liguori <aliguori@us.ibm.com>
> 
> This is a special GSource that supports CharDriverState style
> poll callbacks.
> 
> For reviewability and bisectability, this code is #if 0'd out in this
> patch to avoid unused warnings since all of the functions are static.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>


> +static int io_channel_send_all(GIOChannel *fd, const void *_buf, int len1)
> +{
> +    GIOStatus status;
> +    gsize bytes_written;
> +    int len;
> +    const uint8_t *buf = _buf;
> +
> +    len = len1;
> +    while (len > 0) {
> +        status = g_io_channel_write_chars(fd, (const gchar *)buf, len,
> +                                          &bytes_written, NULL);
> +        if (status != G_IO_STATUS_NORMAL) {
> +            if (status != G_IO_STATUS_AGAIN) {
> +                return -1;
> +            }

It's not quite right to return -1 here; previous iterations of the
while loop could have successfully written data, and (len1 - len)
could be +ve.

How to approach this?  Convert all callers of qemu_chr_fe_write() to
also pass a bytes_written param to handle this case?

> +        } else if (status == G_IO_STATUS_EOF) {
> +            break;
> +        } else {
> +            buf += bytes_written;
> +            len -= bytes_written;
> +        }
> +    }
> +    return len1 - len;
> +}
> +#endif
> +
>  typedef struct {
>      int fd_in, fd_out;
>      int max_size;

		Amit
Anthony Liguori March 29, 2013, 12:24 p.m. UTC | #2 
Amit Shah <amit.shah@redhat.com> writes:

> On (Tue) 05 Mar 2013 [23:21:18], Amit Shah wrote:
>> From: Anthony Liguori <aliguori@us.ibm.com>
>> 
>> This is a special GSource that supports CharDriverState style
>> poll callbacks.
>> 
>> For reviewability and bisectability, this code is #if 0'd out in this
>> patch to avoid unused warnings since all of the functions are static.
>> 
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>
>
>> +static int io_channel_send_all(GIOChannel *fd, const void *_buf, int len1)
>> +{
>> +    GIOStatus status;
>> +    gsize bytes_written;
>> +    int len;
>> +    const uint8_t *buf = _buf;
>> +
>> +    len = len1;
>> +    while (len > 0) {
>> +        status = g_io_channel_write_chars(fd, (const gchar *)buf, len,
>> +                                          &bytes_written, NULL);
>> +        if (status != G_IO_STATUS_NORMAL) {
>> +            if (status != G_IO_STATUS_AGAIN) {
>> +                return -1;
>> +            }
>
> It's not quite right to return -1 here; previous iterations of the
> while loop could have successfully written data, and (len1 - len)
> could be +ve.

Once -1 is returned, it's a terminal error.  It doesn't matter that we
may have written some data.

Regards,

Anthony Liguori

>
> How to approach this?  Convert all callers of qemu_chr_fe_write() to
> also pass a bytes_written param to handle this case?
>
>> +        } else if (status == G_IO_STATUS_EOF) {
>> +            break;
>> +        } else {
>> +            buf += bytes_written;
>> +            len -= bytes_written;
>> +        }
>> +    }
>> +    return len1 - len;
>> +}
>> +#endif
>> +
>>  typedef struct {
>>      int fd_in, fd_out;
>>      int max_size;
>
> 		Amit
Amit Shah March 29, 2013, 12:42 p.m. UTC | #3 
On (Fri) 29 Mar 2013 [07:24:07], Anthony Liguori wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Tue) 05 Mar 2013 [23:21:18], Amit Shah wrote:
> >> From: Anthony Liguori <aliguori@us.ibm.com>
> >> 
> >> This is a special GSource that supports CharDriverState style
> >> poll callbacks.
> >> 
> >> For reviewability and bisectability, this code is #if 0'd out in this
> >> patch to avoid unused warnings since all of the functions are static.
> >> 
> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >
> >
> >> +static int io_channel_send_all(GIOChannel *fd, const void *_buf, int len1)
> >> +{
> >> +    GIOStatus status;
> >> +    gsize bytes_written;
> >> +    int len;
> >> +    const uint8_t *buf = _buf;
> >> +
> >> +    len = len1;
> >> +    while (len > 0) {
> >> +        status = g_io_channel_write_chars(fd, (const gchar *)buf, len,
> >> +                                          &bytes_written, NULL);
> >> +        if (status != G_IO_STATUS_NORMAL) {
> >> +            if (status != G_IO_STATUS_AGAIN) {
> >> +                return -1;
> >> +            }
> >
> > It's not quite right to return -1 here; previous iterations of the
> > while loop could have successfully written data, and (len1 - len)
> > could be +ve.
> 
> Once -1 is returned, it's a terminal error.  It doesn't matter that we
> may have written some data.

Why do you say that?

Try this:

Start a guest with a virtio-serial channel.

In the guest, start writing something to the channel, say a 1M file.

In the host, open the chardev but don't read from it.

This will make the guest send some data, fill the vq and the char
buffers, and then make it stop.

The flow control code in virtio-console.c and virtio-serial-bus.c will
be triggered, and a watch will be registered for the pending data to
be sent once the chardev becomes writable.

If one starts reading from the chardev, we'll get duplicate data in
the current case (16 bytes were written, 20 not, but a retry for all
the 36 bytes will be tried in this case).

Instead, we only want to continue writing from the offset that was
last written.

Having a chardev open but not reading from it causing data to stop
flowing isn't a terminal error.

		Amit
Anthony Liguori March 29, 2013, 2:03 p.m. UTC | #4 
Amit Shah <amit.shah@redhat.com> writes:

> On (Fri) 29 Mar 2013 [07:24:07], Anthony Liguori wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>> 
>> > On (Tue) 05 Mar 2013 [23:21:18], Amit Shah wrote:
>> >> From: Anthony Liguori <aliguori@us.ibm.com>
>> >> 
>> >> This is a special GSource that supports CharDriverState style
>> >> poll callbacks.
>> >> 
>> >> For reviewability and bisectability, this code is #if 0'd out in this
>> >> patch to avoid unused warnings since all of the functions are static.
>> >> 
>> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> >> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>> >
>> >
>> >> +static int io_channel_send_all(GIOChannel *fd, const void *_buf, int len1)
>> >> +{
>> >> +    GIOStatus status;
>> >> +    gsize bytes_written;
>> >> +    int len;
>> >> +    const uint8_t *buf = _buf;
>> >> +
>> >> +    len = len1;
>> >> +    while (len > 0) {
>> >> +        status = g_io_channel_write_chars(fd, (const gchar *)buf, len,
>> >> +                                          &bytes_written, NULL);
>> >> +        if (status != G_IO_STATUS_NORMAL) {
>> >> +            if (status != G_IO_STATUS_AGAIN) {
>> >> +                return -1;
>> >> +            }
>> >
>> > It's not quite right to return -1 here; previous iterations of the
>> > while loop could have successfully written data, and (len1 - len)
>> > could be +ve.
>> 
>> Once -1 is returned, it's a terminal error.  It doesn't matter that we
>> may have written some data.
>
> Why do you say that?

Because you're quoting the wrong patch :-)  This bit is rewritten by a
later patch which is the source of your problem below.  In the patch you
quote, we busy spin until all data is written.  However, with:

    commit 23673ca740e0eda66901ca801a5a901df378b063
    Author: Anthony Liguori <aliguori@us.ibm.com>
    Date:   Tue Mar 5 23:21:23 2013 +0530
    
        qemu-char: add watch support

We started to return EAGAIN even if we have a partially successful
write.  I'm running a patch through testing right now that rewrites this
function to have sane semantics (return bytes written on partial write).

I'll post as soon as testing completes.

Regards,

Anthony Liguori
Amit Shah March 29, 2013, 4:08 p.m. UTC | #5 
On (Fri) 29 Mar 2013 [09:03:10], Anthony Liguori wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Fri) 29 Mar 2013 [07:24:07], Anthony Liguori wrote:
> >> Amit Shah <amit.shah@redhat.com> writes:
> >> 
> >> > On (Tue) 05 Mar 2013 [23:21:18], Amit Shah wrote:
> >> >> From: Anthony Liguori <aliguori@us.ibm.com>
> >> >> 
> >> >> This is a special GSource that supports CharDriverState style
> >> >> poll callbacks.
> >> >> 
> >> >> For reviewability and bisectability, this code is #if 0'd out in this
> >> >> patch to avoid unused warnings since all of the functions are static.
> >> >> 
> >> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> >> >> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >> >
> >> >
> >> >> +static int io_channel_send_all(GIOChannel *fd, const void *_buf, int len1)
> >> >> +{
> >> >> +    GIOStatus status;
> >> >> +    gsize bytes_written;
> >> >> +    int len;
> >> >> +    const uint8_t *buf = _buf;
> >> >> +
> >> >> +    len = len1;
> >> >> +    while (len > 0) {
> >> >> +        status = g_io_channel_write_chars(fd, (const gchar *)buf, len,
> >> >> +                                          &bytes_written, NULL);
> >> >> +        if (status != G_IO_STATUS_NORMAL) {
> >> >> +            if (status != G_IO_STATUS_AGAIN) {
> >> >> +                return -1;
> >> >> +            }
> >> >
> >> > It's not quite right to return -1 here; previous iterations of the
> >> > while loop could have successfully written data, and (len1 - len)
> >> > could be +ve.
> >> 
> >> Once -1 is returned, it's a terminal error.  It doesn't matter that we
> >> may have written some data.
> >
> > Why do you say that?
> 
> Because you're quoting the wrong patch :-)

Indeed.

>  This bit is rewritten by a
> later patch which is the source of your problem below.  In the patch you
> quote, we busy spin until all data is written.  However, with:
> 
>     commit 23673ca740e0eda66901ca801a5a901df378b063
>     Author: Anthony Liguori <aliguori@us.ibm.com>
>     Date:   Tue Mar 5 23:21:23 2013 +0530
>     
>         qemu-char: add watch support
> 
> We started to return EAGAIN even if we have a partially successful
> write.  I'm running a patch through testing right now that rewrites this
> function to have sane semantics (return bytes written on partial write).

Yes, that's where the problem is: EINTR and EAGAIN returns.

> I'll post as soon as testing completes.

Thanks!

		Amit
diff mbox

Patch

diff --git a/qemu-char.c b/qemu-char.c
index 0a74c10..8aa0b41 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -541,6 +541,146 @@  int send_all(int fd, const void *_buf, int len1)
 
 #ifndef _WIN32
 
+#if 0
+typedef struct IOWatchPoll
+{
+    GSource *src;
+    int max_size;
+
+    IOCanReadHandler *fd_can_read;
+    void *opaque;
+
+    QTAILQ_ENTRY(IOWatchPoll) node;
+} IOWatchPoll;
+
+static QTAILQ_HEAD(, IOWatchPoll) io_watch_poll_list =
+    QTAILQ_HEAD_INITIALIZER(io_watch_poll_list);
+
+static IOWatchPoll *io_watch_poll_from_source(GSource *source)
+{
+    IOWatchPoll *i;
+
+    QTAILQ_FOREACH(i, &io_watch_poll_list, node) {
+        if (i->src == source) {
+            return i;
+        }
+    }
+
+    return NULL;
+}
+
+static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_)
+{
+    IOWatchPoll *iwp = io_watch_poll_from_source(source);
+
+    iwp->max_size = iwp->fd_can_read(iwp->opaque);
+    if (iwp->max_size == 0) {
+        return FALSE;
+    }
+
+    return g_io_watch_funcs.prepare(source, timeout_);
+}
+
+static gboolean io_watch_poll_check(GSource *source)
+{
+    IOWatchPoll *iwp = io_watch_poll_from_source(source);
+
+    if (iwp->max_size == 0) {
+        return FALSE;
+    }
+
+    return g_io_watch_funcs.check(source);
+}
+
+static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
+                                       gpointer user_data)
+{
+    return g_io_watch_funcs.dispatch(source, callback, user_data);
+}
+
+static void io_watch_poll_finalize(GSource *source)
+{
+    IOWatchPoll *iwp = io_watch_poll_from_source(source);
+    QTAILQ_REMOVE(&io_watch_poll_list, iwp, node);
+    g_io_watch_funcs.finalize(source);
+}
+
+static GSourceFuncs io_watch_poll_funcs = {
+    .prepare = io_watch_poll_prepare,
+    .check = io_watch_poll_check,
+    .dispatch = io_watch_poll_dispatch,
+    .finalize = io_watch_poll_finalize,
+};
+
+/* Can only be used for read */
+static guint io_add_watch_poll(GIOChannel *channel,
+                               IOCanReadHandler *fd_can_read,
+                               GIOFunc fd_read,
+                               gpointer user_data)
+{
+    IOWatchPoll *iwp;
+    GSource *src;
+    guint tag;
+
+    src = g_io_create_watch(channel, G_IO_IN | G_IO_ERR | G_IO_HUP);
+    g_source_set_funcs(src, &io_watch_poll_funcs);
+    g_source_set_callback(src, (GSourceFunc)fd_read, user_data, NULL);
+    tag = g_source_attach(src, NULL);
+    g_source_unref(src);
+
+    iwp = g_malloc0(sizeof(*iwp));
+    iwp->src = src;
+    iwp->max_size = 0;
+    iwp->fd_can_read = fd_can_read;
+    iwp->opaque = user_data;
+
+    QTAILQ_INSERT_HEAD(&io_watch_poll_list, iwp, node);
+
+    return tag;
+}
+
+static GIOChannel *io_channel_from_fd(int fd)
+{
+    GIOChannel *chan;
+
+    if (fd == -1) {
+        return NULL;
+    }
+
+    chan = g_io_channel_unix_new(fd);
+
+    g_io_channel_set_encoding(chan, NULL, NULL);
+    g_io_channel_set_buffered(chan, FALSE);
+
+    return chan;
+}
+
+static int io_channel_send_all(GIOChannel *fd, const void *_buf, int len1)
+{
+    GIOStatus status;
+    gsize bytes_written;
+    int len;
+    const uint8_t *buf = _buf;
+
+    len = len1;
+    while (len > 0) {
+        status = g_io_channel_write_chars(fd, (const gchar *)buf, len,
+                                          &bytes_written, NULL);
+        if (status != G_IO_STATUS_NORMAL) {
+            if (status != G_IO_STATUS_AGAIN) {
+                return -1;
+            }
+        } else if (status == G_IO_STATUS_EOF) {
+            break;
+        } else {
+            buf += bytes_written;
+            len -= bytes_written;
+        }
+    }
+    return len1 - len;
+}
+#endif
+
 typedef struct {
     int fd_in, fd_out;
     int max_size;