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

login
register
mail settings
Submitter Amit Shah
Date July 12, 2011, 9:28 a.m.
Message ID <5444cc7ab1e9654fa953e626da1a50ce54a72ac7.1310462934.git.amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/104331/
State New
Headers show

Comments

Amit Shah - July 12, 2011, 9:28 a.m.
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(-)
Amit Shah - July 12, 2011, 12:34 p.m.
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
Markus Armbruster - July 20, 2011, 7:10 a.m.
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>
Amit Shah - July 20, 2011, 8:39 a.m.
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

Patch

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 */