[PATCH 1/2] win32: drop fd registration to the main-loop on setting non-block

[PATCH 1/2] win32: drop fd registration to the main-loop on setting non-block

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Low-level fd users from QEMU use aio_set_fd_handler(), which handles
event registration with the main-loop.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/oslib-win32.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 23a7c7320b..01787df74c 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -221,7 +221,6 @@ int qemu_try_set_nonblock(int fd)
     if (ioctlsocket(fd, FIONBIO, &opt) != NO_ERROR) {
         return -socket_error();
     }
-    qemu_fd_register(fd);
     return 0;
 }
 
-- 
2.29.0

[PATCH 2/2] win32: make qemu_fd_register() specific to windows

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Move the declaration of the function to a windows-specific header.

The only user left now is SLIRP, which needs special treatement since
it doesn't provide event objects itself for the socket/fds.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/main-loop.h  | 2 --
 include/sysemu/os-win32.h | 2 ++
 net/slirp.c               | 2 ++
 util/main-loop.c          | 4 ----
 4 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index d6892fd208..bf93fd691d 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -310,8 +310,6 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
 
 /* internal interfaces */
 
-void qemu_fd_register(int fd);
-
 QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque);
 void qemu_bh_schedule_idle(QEMUBH *bh);
 
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 5346d51e89..aa462e3ef6 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -117,6 +117,8 @@ static inline void qemu_funlockfile(FILE *f)
 {
 }
 
+void qemu_fd_register(int fd);
+
 /* We wrap all the sockets functions so that we can
  * set errno based on WSAGetLastError()
  */
diff --git a/net/slirp.c b/net/slirp.c
index 77042e6df7..b54c2882dc 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -196,7 +196,9 @@ static void net_slirp_timer_mod(void *timer, int64_t expire_timer,
 
 static void net_slirp_register_poll_fd(int fd, void *opaque)
 {
+#ifdef WIN32
     qemu_fd_register(fd);
+#endif
 }
 
 static void net_slirp_unregister_poll_fd(int fd, void *opaque)
diff --git a/util/main-loop.c b/util/main-loop.c
index 6470f8eae3..744b42fc54 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -179,10 +179,6 @@ static int max_priority;
 static int glib_pollfds_idx;
 static int glib_n_poll_fds;
 
-void qemu_fd_register(int fd)
-{
-}
-
 static void glib_pollfds_fill(int64_t *cur_timeout)
 {
     GMainContext *context = g_main_context_default();
-- 
2.29.0

Re: [PATCH 2/2] win32: make qemu_fd_register() specific to windows

[Paolo Bonzini]

On 18/12/20 14:57, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Move the declaration of the function to a windows-specific header.
> 
> The only user left now is SLIRP, which needs special treatement since
> it doesn't provide event objects itself for the socket/fds.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

This patch is not needed to fix the build, right?  (I don't disagree 
with it, but I wanted to understand why _you_ thought it was an 
improvement).

Paolo

> ---
>   include/qemu/main-loop.h  | 2 --
>   include/sysemu/os-win32.h | 2 ++
>   net/slirp.c               | 2 ++
>   util/main-loop.c          | 4 ----
>   4 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> index d6892fd208..bf93fd691d 100644
> --- a/include/qemu/main-loop.h
> +++ b/include/qemu/main-loop.h
> @@ -310,8 +310,6 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
>   
>   /* internal interfaces */
>   
> -void qemu_fd_register(int fd);
> -
>   QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque);
>   void qemu_bh_schedule_idle(QEMUBH *bh);
>   
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index 5346d51e89..aa462e3ef6 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -117,6 +117,8 @@ static inline void qemu_funlockfile(FILE *f)
>   {
>   }
>   
> +void qemu_fd_register(int fd);
> +
>   /* We wrap all the sockets functions so that we can
>    * set errno based on WSAGetLastError()
>    */
> diff --git a/net/slirp.c b/net/slirp.c
> index 77042e6df7..b54c2882dc 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -196,7 +196,9 @@ static void net_slirp_timer_mod(void *timer, int64_t expire_timer,
>   
>   static void net_slirp_register_poll_fd(int fd, void *opaque)
>   {
> +#ifdef WIN32
>       qemu_fd_register(fd);
> +#endif
>   }
>   
>   static void net_slirp_unregister_poll_fd(int fd, void *opaque)
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 6470f8eae3..744b42fc54 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -179,10 +179,6 @@ static int max_priority;
>   static int glib_pollfds_idx;
>   static int glib_n_poll_fds;
>   
> -void qemu_fd_register(int fd)
> -{
> -}
> -
>   static void glib_pollfds_fill(int64_t *cur_timeout)
>   {
>       GMainContext *context = g_main_context_default();
> 

Re: [PATCH 2/2] win32: make qemu_fd_register() specific to windows

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3196 bytes --]

Hi

On Sat, Dec 19, 2020 at 4:19 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 18/12/20 14:57, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Move the declaration of the function to a windows-specific header.
> >
> > The only user left now is SLIRP, which needs special treatement since
> > it doesn't provide event objects itself for the socket/fds.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This patch is not needed to fix the build, right?  (I don't disagree
> with it, but I wanted to understand why _you_ thought it was an
> improvement).
>
>
My point is that this is corenercase-ish and specific to Windows, I don't
really think it's worth it to be in main-loop.h, with stubs to take care of
the non-windows case (and possibly error prone, if you link against it by
mistake).

If we were to be using that call consistently in every part of qemu using
an fd and the main-loop, it would make sense to keep it "generic". But it's
not being used that way.

Paolo
>
> > ---
> >   include/qemu/main-loop.h  | 2 --
> >   include/sysemu/os-win32.h | 2 ++
> >   net/slirp.c               | 2 ++
> >   util/main-loop.c          | 4 ----
> >   4 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
> > index d6892fd208..bf93fd691d 100644
> > --- a/include/qemu/main-loop.h
> > +++ b/include/qemu/main-loop.h
> > @@ -310,8 +310,6 @@ void qemu_cond_timedwait_iothread(QemuCond *cond,
> int ms);
> >
> >   /* internal interfaces */
> >
> > -void qemu_fd_register(int fd);
> > -
> >   QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque);
> >   void qemu_bh_schedule_idle(QEMUBH *bh);
> >
> > diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> > index 5346d51e89..aa462e3ef6 100644
> > --- a/include/sysemu/os-win32.h
> > +++ b/include/sysemu/os-win32.h
> > @@ -117,6 +117,8 @@ static inline void qemu_funlockfile(FILE *f)
> >   {
> >   }
> >
> > +void qemu_fd_register(int fd);
> > +
> >   /* We wrap all the sockets functions so that we can
> >    * set errno based on WSAGetLastError()
> >    */
> > diff --git a/net/slirp.c b/net/slirp.c
> > index 77042e6df7..b54c2882dc 100644
> > --- a/net/slirp.c
> > +++ b/net/slirp.c
> > @@ -196,7 +196,9 @@ static void net_slirp_timer_mod(void *timer, int64_t
> expire_timer,
> >
> >   static void net_slirp_register_poll_fd(int fd, void *opaque)
> >   {
> > +#ifdef WIN32
> >       qemu_fd_register(fd);
> > +#endif
> >   }
> >
> >   static void net_slirp_unregister_poll_fd(int fd, void *opaque)
> > diff --git a/util/main-loop.c b/util/main-loop.c
> > index 6470f8eae3..744b42fc54 100644
> > --- a/util/main-loop.c
> > +++ b/util/main-loop.c
> > @@ -179,10 +179,6 @@ static int max_priority;
> >   static int glib_pollfds_idx;
> >   static int glib_n_poll_fds;
> >
> > -void qemu_fd_register(int fd)
> > -{
> > -}
> > -
> >   static void glib_pollfds_fill(int64_t *cur_timeout)
> >   {
> >       GMainContext *context = g_main_context_default();
> >
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4470 bytes --]