| Message ID | 5444cc7ab1e9654fa953e626da1a50ce54a72ac7.1310462934.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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>
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
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 */
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(-)