Message ID | 20130130114409.GA2990@amit.redhat.com |
---|---|
State | New |
Headers | show |
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
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
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
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 --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_);