diff mbox

Char flow control patches

Message ID 20130130114409.GA2990@amit.redhat.com
State New
Headers show

Commit Message

Amit Shah Jan. 30, 2013, 11:44 a.m. UTC 
Hi Anthony,

I did some basic testing of the char flow control patches from your
char-flow.2 branch.  With the following patch applied, things seem to
be working fine.  I tested the isa-serial and virtio-serial devices.

I haven't yet tested with virtio-serial flow control, but at least no
regressions as of now.



In addition to this, we should also have the following, but the above
masks these:

@@ -1067,7 +1067,7 @@ static gboolean pty_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     if (len > s->read_bytes)
         len = s->read_bytes;
     if (len == 0)
-        return TRUE;
+        return FALSE;
     status = g_io_channel_read_chars(s->fd, (gchar *)buf, len, &size, NULL);
     if (status != G_IO_STATUS_NORMAL) {
         pty_chr_state(chr, 0);
@@ -2237,7 +2237,7 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     GIOStatus status;
 
     if (s->max_size == 0)
-        return TRUE;
+        return FALSE;
     status = g_io_channel_read_chars(s->chan, (gchar *)s->buf, sizeof(s->buf),
                                      &bytes_read, NULL);
     s->bufcnt = bytes_read;
@@ -2492,7 +2492,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     int len, size;
 
     if (!s->connected || s->max_size <= 0) {
-        return TRUE;
+        return FALSE;
     }
     len = sizeof(buf);
     if (len > s->max_size)



		Amit

Comments

Anthony Liguori Jan. 30, 2013, 2:38 p.m. UTC | #1 
Amit Shah <amit.shah@redhat.com> writes:

> Hi Anthony,
>
> I did some basic testing of the char flow control patches from your
> char-flow.2 branch.  With the following patch applied, things seem to
> be working fine.  I tested the isa-serial and virtio-serial devices.
>
> I haven't yet tested with virtio-serial flow control, but at least no
> regressions as of now.

Awesome!

It's obviously too late for 1.4 but I'd like to submit a proper series
during the hard freeze so we can take it early in 1.5.

Can you add a Signed-off-by and I'll include it in my queue.

Regards,

Anthony Liguori

>
> diff --git a/qemu-char.c b/qemu-char.c
> index 2b714cf..5731d02 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -569,7 +569,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_)
>  
>      iwp->max_size = iwp->fd_can_read(iwp->opaque);
>      if (iwp->max_size == 0) {
> -        return TRUE;
> +        return FALSE;
>      }
>  
>      return g_io_watch_funcs.prepare(source, timeout_);
>
>
> In addition to this, we should also have the following, but the above
> masks these:
>
> @@ -1067,7 +1067,7 @@ static gboolean pty_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>      if (len > s->read_bytes)
>          len = s->read_bytes;
>      if (len == 0)
> -        return TRUE;
> +        return FALSE;
>      status = g_io_channel_read_chars(s->fd, (gchar *)buf, len, &size, NULL);
>      if (status != G_IO_STATUS_NORMAL) {
>          pty_chr_state(chr, 0);
> @@ -2237,7 +2237,7 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>      GIOStatus status;
>  
>      if (s->max_size == 0)
> -        return TRUE;
> +        return FALSE;
>      status = g_io_channel_read_chars(s->chan, (gchar *)s->buf, sizeof(s->buf),
>                                       &bytes_read, NULL);
>      s->bufcnt = bytes_read;
> @@ -2492,7 +2492,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>      int len, size;
>  
>      if (!s->connected || s->max_size <= 0) {
> -        return TRUE;
> +        return FALSE;
>      }
>      len = sizeof(buf);
>      if (len > s->max_size)
>
>
>
> 		Amit
Amit Shah Feb. 1, 2013, 8:15 a.m. UTC | #2 
On (Wed) 30 Jan 2013 [08:38:50], Anthony Liguori wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Hi Anthony,
> >
> > I did some basic testing of the char flow control patches from your
> > char-flow.2 branch.  With the following patch applied, things seem to
> > be working fine.  I tested the isa-serial and virtio-serial devices.
> >
> > I haven't yet tested with virtio-serial flow control, but at least no
> > regressions as of now.
> 
> Awesome!
> 
> It's obviously too late for 1.4 but I'd like to submit a proper series
> during the hard freeze so we can take it early in 1.5.

Agreed, late for 1.4.

> Can you add a Signed-off-by and I'll include it in my queue.

Sure, but it's a small thing, and I'd prefer if you folded it in the
patch that introduces that hunk.

BTW I'm only sure of the first diff here, do the rest look OK?  I'm
not too familiar with the gio interface...

> > diff --git a/qemu-char.c b/qemu-char.c
> > index 2b714cf..5731d02 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -569,7 +569,7 @@ static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_)
> >  
> >      iwp->max_size = iwp->fd_can_read(iwp->opaque);
> >      if (iwp->max_size == 0) {
> > -        return TRUE;
> > +        return FALSE;
> >      }
> >  
> >      return g_io_watch_funcs.prepare(source, timeout_);
> >
> >
> > In addition to this, we should also have the following, but the above
> > masks these:
> >
> > @@ -1067,7 +1067,7 @@ static gboolean pty_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
> >      if (len > s->read_bytes)
> >          len = s->read_bytes;
> >      if (len == 0)
> > -        return TRUE;
> > +        return FALSE;
> >      status = g_io_channel_read_chars(s->fd, (gchar *)buf, len, &size, NULL);
> >      if (status != G_IO_STATUS_NORMAL) {
> >          pty_chr_state(chr, 0);
> > @@ -2237,7 +2237,7 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
> >      GIOStatus status;
> >  
> >      if (s->max_size == 0)
> > -        return TRUE;
> > +        return FALSE;
> >      status = g_io_channel_read_chars(s->chan, (gchar *)s->buf, sizeof(s->buf),
> >                                       &bytes_read, NULL);
> >      s->bufcnt = bytes_read;
> > @@ -2492,7 +2492,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
> >      int len, size;
> >  
> >      if (!s->connected || s->max_size <= 0) {
> > -        return TRUE;
> > +        return FALSE;
> >      }
> >      len = sizeof(buf);
> >      if (len > s->max_size)

		Amit
Amit Shah Feb. 11, 2013, 7:07 a.m. UTC | #3 
On (Wed) 30 Jan 2013 [08:38:50], Anthony Liguori wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Hi Anthony,
> >
> > I did some basic testing of the char flow control patches from your
> > char-flow.2 branch.  With the following patch applied, things seem to
> > be working fine.  I tested the isa-serial and virtio-serial devices.
> >
> > I haven't yet tested with virtio-serial flow control, but at least no
> > regressions as of now.
> 
> Awesome!

Well -- just tested starting sockets in non-server mode, and that
fails with:

(process:922): GLib-CRITICAL **: g_io_channel_write_chars: assertion `channel != NULL' failed

cmdline:

./x86_64-softmmu/qemu-system-x86_64 -m 512 /guests/f14-nolvm.qcow2 \
  -snapshot -enable-kvm   -chardev socket,path=/tmp/foo,id=foo \
  -device virtio-serial -device virtserialport,chardev=foo,nr=2 \
  -monitor stdio

on the host, also start a listening process, like:

$ socat UNIX-LISTEN:/tmp/foo  -

and doing 'echo foo > /dev/vport0p2' in the guest results in the error
message above.

		Amit
Amit Shah Feb. 12, 2013, 6:30 a.m. UTC | #4 
On (Mon) 11 Feb 2013 [12:37:34], Amit Shah wrote:
> On (Wed) 30 Jan 2013 [08:38:50], Anthony Liguori wrote:
> > Amit Shah <amit.shah@redhat.com> writes:
> > 
> > > Hi Anthony,
> > >
> > > I did some basic testing of the char flow control patches from your
> > > char-flow.2 branch.  With the following patch applied, things seem to
> > > be working fine.  I tested the isa-serial and virtio-serial devices.
> > >
> > > I haven't yet tested with virtio-serial flow control, but at least no
> > > regressions as of now.
> > 
> > Awesome!
> 
> Well -- just tested starting sockets in non-server mode, and that
> fails with:

Found another one, though it's an old one (so not really a regression):

QEMU doesn't catch -EPIPE for short-lived guest processes and
disconnected host chardevs. This is also described in
https://bugzilla.redhat.com/show_bug.cgi?id=621484. To reproduce this
issue:

    start qemu, boot guest
    connect netcat to the unix socket
    echo some chars to /dev/virtio-ports/$name
    watch them appear in netcat
    stop netcat, restart it
    echo some chars again
    watch them *not* appear 

BTW I've just put up this feature page on the wiki with status,
testing and known bugs sections, which describe the two current known
bugs.

http://wiki.qemu.org/Features/ChardevFlowControl

		Amit
diff mbox

Patch

diff --git a/qemu-char.c b/qemu-char.c
index 2b714cf..5731d02 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -569,7 +569,7 @@  static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_)
 
     iwp->max_size = iwp->fd_can_read(iwp->opaque);
     if (iwp->max_size == 0) {
-        return TRUE;
+        return FALSE;
     }
 
     return g_io_watch_funcs.prepare(source, timeout_);