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

[Marc-André Lureau]

Hi
On Thu, Nov 15, 2018 at 5:09 PM Paolo Bonzini <address@hidden> 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 <address@hidden>
>
> 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:

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));
> >
>
>


-- 
Marc-André Lureau