| Message ID | 18c8e8e9a6b40015a3cb0a6217667a7254b73353.1310046798.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> > --- > 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..a25c29e 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 && ret != -EAGAIN) { > + /* > + * 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 */ qemu_chr_write() is the obvious wrapper around the chr_write() method. What's the contract for that method? Specifically, is the error value -1, -errno, or any negative value? Unless it's -errno, we should *not* test for -EAGAIN here. What's the impact of silently ignoring errors other than EPIPE?
diff --git a/hw/virtio-console.c b/hw/virtio-console.c index b076331..a25c29e 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 && ret != -EAGAIN) { + /* + * 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(-)