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

[Denis V. Lunev]

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?

Den