Re: [Qemu-devel] [PATCH for-3.2 09/41] slirp: add a set_nonblock() callback

[Paolo Bonzini <pbonzini@redhat.com>]

On 21/11/18 22:02, Marc-André Lureau wrote:
> Hi
> On Thu, Nov 15, 2018 at 5:09 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 14/11/2018 13:36, Marc-André Lureau wrote:
>>> qemu_set_nonblock() does some event registration with the main loop on
>>> win32, let's have a callback.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Perhaps a better interface would be register_poll_fd, which is called
>> before a file descriptor can be returned to slirp_pollfds_fill?  And
>> perhaps a dual unregister_poll_fd, which QEMU would leave empty, to be
>> called before closing the file descriptor.
> 
> That sounds like a good idea, but I think it will bring more issues as
> qemu_fd_register() doing WSAEventSelect will put the socket in
> nonblocking mode anyway, and we don't have/need qemu_fd_unregister()
> yet:

Right, I was more thinking of other possible future clients of SLIRP.
But what you have now surely is fine.

(One possible way to work around the problem could be to put the socket
in non-blocking mode in SLIRP, since that is OS-dependent but not
client-dependent.  Then we can document that the register/unregister_fd
API is called with sockets that are already in non-blocking mode.  That
complicates the code a bit in order to have a nicer API).

Paolo

> https://msdn.microsoft.com/de-de/library/windows/desktop/ms738573(v=vs.85).aspx
> "The WSAAsyncSelect and WSAEventSelect functions automatically set a
> socket to nonblocking mode. If WSAAsyncSelect or WSAEventSelect has
> been issued on a socket, then any attempt to use ioctlsocket to set
> the socket back to blocking mode will fail with WSAEINVAL.
> To set the socket back to blocking mode, an application must first
> disable WSAAsyncSelect by calling WSAAsyncSelect with the lEvent
> parameter equal to zero, or disable WSAEventSelect by calling
> WSAEventSelect with the lNetworkEvents parameter equal to zero."
> 
> I will stick to the set_nonblock() callback for now.
> 
> thanks
> 
> 
> 
>> Paolo
>>
>>> ---
>>>  slirp/libslirp.h | 1 +
>>>  net/slirp.c      | 1 +
>>>  slirp/misc.c     | 2 +-
>>>  slirp/tcp_subr.c | 4 ++--
>>>  4 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/slirp/libslirp.h b/slirp/libslirp.h
>>> index 88185e6c33..f2e7f94ebb 100644
>>> --- a/slirp/libslirp.h
>>> +++ b/slirp/libslirp.h
>>> @@ -20,6 +20,7 @@ typedef struct SlirpCb {
>>>                        SlirpTimerCb cb, void *opaque);
>>>      void (*timer_free)(void *timer);
>>>      void (*timer_mod)(void *timer, int64_t expire_timer);
>>> +    void (*set_nonblock)(int fd);
>>>  } SlirpCb;
>>>
>>>
>>> diff --git a/net/slirp.c b/net/slirp.c
>>> index 7b28886802..5ea8c255f6 100644
>>> --- a/net/slirp.c
>>> +++ b/net/slirp.c
>>> @@ -190,6 +190,7 @@ static SlirpCb slirp_cb = {
>>>      .timer_new = net_slirp_timer_new,
>>>      .timer_free = net_slirp_timer_free,
>>>      .timer_mod = net_slirp_timer_mod,
>>> +    .set_nonblock = qemu_set_nonblock,
>>>  };
>>>
>>>  static int net_slirp_init(NetClientState *peer, const char *model,
>>> diff --git a/slirp/misc.c b/slirp/misc.c
>>> index 7972b9b05b..dd2b3512a8 100644
>>> --- a/slirp/misc.c
>>> +++ b/slirp/misc.c
>>> @@ -174,7 +174,7 @@ fork_exec(struct socket *so, const char *ex)
>>>      socket_set_fast_reuse(so->s);
>>>      opt = 1;
>>>      qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
>>> -    qemu_set_nonblock(so->s);
>>> +    so->slirp->cb->set_nonblock(so->s);
>>>      return 1;
>>>  }
>>>  #endif
>>> diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
>>> index 4b40850c7a..8d97f1f54e 100644
>>> --- a/slirp/tcp_subr.c
>>> +++ b/slirp/tcp_subr.c
>>> @@ -412,7 +412,7 @@ int tcp_fconnect(struct socket *so, unsigned short af)
>>>      int opt, s=so->s;
>>>      struct sockaddr_storage addr;
>>>
>>> -    qemu_set_nonblock(s);
>>> +    so->slirp->cb->set_nonblock(s);
>>>      socket_set_fast_reuse(s);
>>>      opt = 1;
>>>      qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(opt));
>>> @@ -484,7 +484,7 @@ void tcp_connect(struct socket *inso)
>>>          tcp_close(sototcpcb(so)); /* This will sofree() as well */
>>>          return;
>>>      }
>>> -    qemu_set_nonblock(s);
>>> +    so->slirp->cb->set_nonblock(s);
>>>      socket_set_fast_reuse(s);
>>>      opt = 1;
>>>      qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
>>>
>>
>>
> 
>