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

[Daniel P. Berrange]

On Thu, Feb 02, 2017 at 02:43:18PM +0300, Denis V. Lunev wrote:
> On 02/02/2017 12:47 PM, Daniel P. Berrange wrote:
> > 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
> But the problem is that Error does not have error code field inside.
> Should we play with ErrorClass enum?

No, don't try to distinguish different types of errors. As mentioned
elsewhere just use the same logic for all errors, except when seeing
QIO_ERR_WOULD_BLOCK

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