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

[Markus Armbruster]

Amit Shah <address@hidden> 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 <address@hidden>
> ---
>  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?