On Fri, Feb 03, 2017 at 12:52:29PM +0300, Michael Tokarev wrote:
> When qemu vnc server is trying to send large update to clients,
> there might be a situation when system responds with something
> like EAGAIN, indicating that there's no system memory to send
> that much data (depending on the network speed, client and server
> and what is happening). In this case, something like this happens
> on qemu side (from strace):
>
> sendmsg(16, {msg_name(0)=NULL,
> msg_iov(1)=[{"\244\"..., 729186}],
> msg_controllen=0, msg_flags=0}, 0) = 103950
> sendmsg(16, {msg_name(0)=NULL,
> msg_iov(1)=[{"lz\346"..., 1559618}],
> msg_controllen=0, msg_flags=0}, 0) = -1 EAGAIN
> sendmsg(-1, {msg_name(0)=NULL,
> msg_iov(1)=[{"lz\346"..., 1559618}],
> msg_controllen=0, msg_flags=0}, 0) = -1 EBADF
>
> qemu closes the socket before the retry, and obviously it gets EBADF
> when trying to send to -1.
>
> This is because there WAS a special handling for EAGAIN, but now it doesn't
> work anymore, after commit 04d2529da27db512dcbd5e99d0e26d333f16efcc, because
> now in all error-like cases we initiate vnc disconnect.
>
> This change were introduced in qemu 2.6, and caused numerous grief for many
> people, resulting in their vnc clients reporting sporadic random disconnects
> from vnc server.
>
> Fix that by doing the disconnect only when necessary, i.e. omitting this
> very case of EAGAIN.
>
> Hopefully the existing condition (comparing with QIO_CHANNEL_ERR_BLOCK)
> is sufficient, as the original code (before the above commit) were
> checking for other errno values too.
There's no need to check WSAEWOULDBLOCK anymore - our win32 portability
code already remaps it to EAGAIN.
Checking EINTR is not required because QIOChannelSocket already
restarts the recvmsg/sendmsg when seeing EINTR
> Apparently there's another (semi?)bug exist somewhere here, since the
> code tries to write to fd# -1, it probably should check if the connection
> is open before. But this isn't important.
>
> Signed-off-by: Michael Tokarev <m...@tls.msk.ru>
> Fixes: 04d2529da27db512dcbd5e99d0e26d333f16efcc
> Cc: Daniel P. Berrange <berra...@redhat.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: qemu-sta...@nongnu.org
> ---
> v2: previous patch was tab/space-damaged, fixing this now
>
> ui/vnc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index cdeb79c..f2701e5 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1256,12 +1256,13 @@ ssize_t vnc_client_io_error(VncState *vs, ssize_t
> ret, Error **errp)
> if (ret <= 0) {
> if (ret == 0) {
> VNC_DEBUG("Closing down client sock: EOF\n");
> + vnc_disconnect_start(vs);
> } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
> VNC_DEBUG("Closing down client sock: ret %zd (%s)\n",
> ret, errp ? error_get_pretty(*errp) : "Unknown");
> + vnc_disconnect_start(vs);
> }
>
> - vnc_disconnect_start(vs);
> if (errp) {
> error_free(*errp);
> *errp = NULL;
Reviewed-by: Daniel P. Berrange <berra...@redhat.com>
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/ :|
