[Qemu-devel] [PATCH v2 1/1] virtio-console: Prevent abort()s in case of host chardev close

[Qemu-devel] [PATCH v2 1/1] virtio-console: Prevent abort()s in case of host chardev close

[Amit Shah]

A host chardev could close just before the guest sends some data to be
written.  This will cause an -EPIPE error.  This shouldn't be propagated
to virtio-serial-bus.

Ideally we should close the port once -EPIPE is received, but since the
chardev interface doesn't return such meaningful values to its users,
all we get is -1 for any kind of error.  Just return 0 for now and wait
for chardevs to return better error messages to act better on the return
messages.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index b076331..9ca0dc6 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -24,8 +24,24 @@ typedef struct VirtConsole {
 static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
-
-    return qemu_chr_write(vcon->chr, buf, len);
+    ssize_t ret;
+
+    ret = qemu_chr_write(vcon->chr, buf, len);
+    if (ret < 0) {
+        /*
+         * Ideally we'd get a better error code than just -1, but
+         * that's what the chardev interface gives us right now.  If
+         * we had a finer-grained message, like -EPIPE, we could close
+         * this connection.  Absent such error messages, the most we
+         * can do is to return 0 here.
+         *
+         * This will prevent stray -1 values to go to
+         * virtio-serial-bus.c and cause abort()s in
+         * do_flush_queued_data().
+         */
+        ret = 0;
+    }
+    return ret;
 }
 
 /* Callback function that's called when the guest opens the port */
-- 
1.7.6

Re: [Qemu-devel] [PATCH v2 1/1] virtio-console: Prevent abort()s in case of host chardev close

[Amit Shah]

On (Tue) 12 Jul 2011 [14:58:57], Amit Shah wrote:
> A host chardev could close just before the guest sends some data to be
> written.  This will cause an -EPIPE error.  This shouldn't be propagated
> to virtio-serial-bus.
> 
> Ideally we should close the port once -EPIPE is received, but since the
> chardev interface doesn't return such meaningful values to its users,
> all we get is -1 for any kind of error.  Just return 0 for now and wait
> for chardevs to return better error messages to act better on the return
> messages.

For v2, removed the check for -EAGAIN as qemu-char.c doesn't return
anything other than -1 for error conditions, as Markus and Juan noted.

		Amit

Re: [Qemu-devel] [PATCH v2 1/1] virtio-console: Prevent abort()s in case of host chardev close

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> A host chardev could close just before the guest sends some data to be
> written.  This will cause an -EPIPE error.  This shouldn't be propagated
> to virtio-serial-bus.
>
> Ideally we should close the port once -EPIPE is received, but since the
> chardev interface doesn't return such meaningful values to its users,
> all we get is -1 for any kind of error.  Just return 0 for now and wait
> for chardevs to return better error messages to act better on the return
> messages.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Besides qemu_chr_write() returning meaningful errors, it would be nice
to have less harsh error handing in do_flush_queued_data(), wouldn't it?

Short of that, we can either suppress real write errors, or turn a
perfectly normal condition into an error.  This patch does the latter,
because it's a much lesser evil.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/1] virtio-console: Prevent abort()s in case of host chardev close

[Amit Shah]

On (Wed) 20 Jul 2011 [09:10:41], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > A host chardev could close just before the guest sends some data to be
> > written.  This will cause an -EPIPE error.  This shouldn't be propagated
> > to virtio-serial-bus.
> >
> > Ideally we should close the port once -EPIPE is received, but since the
> > chardev interface doesn't return such meaningful values to its users,
> > all we get is -1 for any kind of error.  Just return 0 for now and wait
> > for chardevs to return better error messages to act better on the return
> > messages.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> Besides qemu_chr_write() returning meaningful errors, it would be nice
> to have less harsh error handing in do_flush_queued_data(), wouldn't it?

I wanted to keep the interfaces cleanly separated: virtio-console.c
deals with errors from chardevs.  virtio-serial-bus is only a
transport between the host and guest; and shouldn't have to deal with
chardev errors.  It can at most throttle the guest if the host can't
consume any more data (which is what it now does on errors).

> Short of that, we can either suppress real write errors, or turn a
> perfectly normal condition into an error.  This patch does the latter,
> because it's a much lesser evil.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks; Anthony merged this yesterday.

		Amit