[Spice-devel] Treat EAGAIN similarly to EINTR and try again.

Submitted by Jeremy White on May 2, 2012, 7:24 p.m.

Details

Message ID 4FA189EA.8050508@codeweavers.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Jeremy White May 2, 2012, 7:24 p.m.
I had a puzzling bug where I could not establish a connection
with a spice server across an openvpn connection onto a Debian
stable box.  It turns out that the network stack in question
apparently is very careful on non blocking sockets, and was
returning EAGAIN on the network ping test transmission.

The channel code appears to be clearly broken; on the EAGAIN,
it should clearly try the write again later, just as it does
on an EINTR return.  This patch makes that change.

Cheers,

Jeremy

---
 server/red_channel.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/server/red_channel.c b/server/red_channel.c
index 4858bb5..612459c 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -278,7 +278,7 @@  static void red_peer_handle_outgoing(RedsStream *stream, OutgoingHandler *handle
             switch (errno) {
             case EAGAIN:
                 handler->cb->on_block(handler->opaque);
-                return;
+                continue;
             case EINTR:
                 continue;
             case EPIPE:


Comments

Hi,
On 05/02/2012 10:24 PM, Jeremy White wrote:
> I had a puzzling bug where I could not establish a connection
> with a spice server across an openvpn connection onto a Debian
> stable box.  It turns out that the network stack in question
> apparently is very careful on non blocking sockets, and was
> returning EAGAIN on the network ping test transmission.
>
> The channel code appears to be clearly broken; on the EAGAIN,
> it should clearly try the write again later, just as it does
> on an EINTR return.  This patch makes that change.
>
Why do you think it is broken? EAGAIN and EINTR have different meaning. 
The EAGAIN means that the write would have blocked if the socket was 
blocking. So we use watch_update_mask to watch the socket, and we make 
the thread available for other tasks and try again to write only when 
the socket is available again.

Yonit.
> Cheers,
>
> Jeremy
>
> ---
>   server/red_channel.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
>
>
>
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On 05/03/2012 03:38 AM, Yonit Halperin wrote:
> Hi,
> On 05/02/2012 10:24 PM, Jeremy White wrote:
>> I had a puzzling bug where I could not establish a connection
>> with a spice server across an openvpn connection onto a Debian
>> stable box.  It turns out that the network stack in question
>> apparently is very careful on non blocking sockets, and was
>> returning EAGAIN on the network ping test transmission.
>>
>> The channel code appears to be clearly broken; on the EAGAIN,
>> it should clearly try the write again later, just as it does
>> on an EINTR return.  This patch makes that change.
>>
> Why do you think it is broken? EAGAIN and EINTR have different meaning. The EAGAIN means that the write would have blocked if the socket was blocking. So we use watch_update_mask to watch the socket, and we make the thread available for other tasks and try again to write only when the socket is available again.

Ah, sure; I jumped to a wrong conclusion. The for (;;) gave me
the impression that red_peer_handle_outgoing() was not supposed
to exit until it had sent the data.

So then the question is - what mechanism is supposed
to cause red_peer_handle_outgoing() to be revisited,
and why didn't it?

Cheers,

Jeremy