Re: [Qemu-devel] [PATCH 1/1] qemu_char: socket backend: disconnect on EPIPE

[Daniel P. Berrange]

On Wed, Feb 01, 2017 at 04:15:28PM +0300, Denis V. Lunev wrote:
> From: Anton Nefedov <address@hidden>
> 
> Socket backend read handler should normally perform a disconnect, however
> the read handler may not get a chance to run if the frontend is not ready
> (qemu_chr_be_can_write == 0).
> 
> This means that in virtio-serial frontend case if
>  - the host has disconnected (giving EPIPE on socket write)
>  - and the guest has disconnected (-> frontend not ready -> backend
>    will not read)
>  - and there is still data (frontend->backend) to flush (has to be a really
>    tricky timing but nevertheless, we have observed the case in production)
> 
> This results in virtio-serial trying to flush this data continiously forming
> a busy loop.
> 
> Solution: react on EPIPE in the socket write handler. We must not disconnect
> right away though, there still may be data to read (see 4bf1cb0).
> 
> Signed-off-by: Anton Nefedov <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> CC: Daniel P. Berrange <address@hidden>
> ---
>  qemu-char.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 6b4a299..f1f7a07 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1202,7 +1202,7 @@ static int io_channel_send_full(QIOChannel *ioc,
>              errno = EAGAIN;
>              return -1;
>          } else if (ret < 0) {
> -            errno = EINVAL;
> +            errno = errno == EPIPE ? EPIPE : EINVAL;

You can't rely on 'errno' being valid after qio_channel_writev_full().

The 'Error **errp' arg to that function is the only error reporting
mechanism that's supported. In particular since the TCP channel can
be wrapped in TLS, the errno can easily have been clobbered by time
you get here.

> @@ -2854,6 +2854,9 @@ static gboolean tcp_chr_accept(QIOChannel *chan,
>                                 GIOCondition cond,
>                                 void *opaque);
>  
> +static int tcp_chr_read_poll(void *opaque);
> +static void tcp_chr_disconnect(Chardev *chr);
> +
>  /* Called with chr_write_lock held.  */
>  static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>  {
> @@ -2871,6 +2874,13 @@ static int tcp_chr_write(Chardev *chr, const uint8_t 
> *buf, int len)
>              s->write_msgfds_num = 0;
>          }
>  
> +        if (ret < 0 && errno == EPIPE) {

IMHO you should just remove "&& errno == EPIPE" and it'd be fine.

> +            if (tcp_chr_read_poll(chr) <= 0) {
> +                tcp_chr_disconnect(chr);
> +                return len;
> +            } /* else let the read handler finish it properly */
> +        }
> +
>          return ret;
>      } else {
>          /* XXX: indicate an error ? */

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|