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

[Daniel P. Berrange]

On Thu, Feb 02, 2017 at 11:59:22AM +0300, Denis V. Lunev wrote:
> On 02/01/2017 04:53 PM, Daniel P. Berrange wrote:
> > 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.
> can we return errno directly from this call and pass exit code to the upper
> layer? This will be fair and much more straightforward.

No, the qio APIs explicitly do *not* use errno because their implementations
may be calling APIs which in turn do not provide errnos. errno is only a
useful concept if your always calling into system calls. As soon as you need
to deal with higher level libraries, errno is woefully inadequate as a
concept, hence using Error ** instead.

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