[Qemu-devel] [PATCH 0/6] fix w32 sockets

[Qemu-devel] [PATCH 0/6] fix w32 sockets

[Paolo Bonzini]

The w32 main loop has been mostly broken by the introduction of the
glib main loop.  glib's g_poll does not use sockets on w32, so we
need a separate approach.

Patch 1 is a simple cleanup that is needed later in the series.

Patch 2 and patch 3 completely separate the way the main loop waits
on POSIX and w32 systems, and drop glib source handling from the w32
main loop.

Patch 4 fixes a longstanding bug in how sockets are handled, also
simplifying the code in the process.  On top of this simplification,
patch 5 starts using g_poll in the w32 main loop and patch 6 adds
back glib source handling.

I didn't test this in the conditions explained in bug 916720, but I
tested both a TCP monitor and an stdio monitor and both work (under
Wine that is).

Stefan, can you please take care of shepherding the patches in
(pinging etc.)?

Paolo Bonzini (6):
  slirp: use socket_set_nonblock
  main loop: use msec-based timeout in glib_select_fill
  main-loop: disable fd_set-based glib integration under w32
  main-loop: interrupt wait when data arrives on a socket
  main-loop: replace WaitForMultipleObjects with g_poll
  main-loop: integrate glib sources for w32

 main-loop.c      |  147 +++++++++++++++++++++++++++++++-----------------------
 main-loop.h      |    1 +
 oslib-win32.c    |    3 +
 slirp/misc.c     |   46 +----------------
 slirp/tcp_subr.c |    4 +-
 5 files changed, 92 insertions(+), 109 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/6] slirp: use socket_set_nonblock

[Paolo Bonzini]

Cc: Jan Kiszka <jan.kiszka@siemens.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 slirp/misc.c     |   46 +---------------------------------------------
 slirp/tcp_subr.c |    4 ++--
 2 files changed, 3 insertions(+), 47 deletions(-)

diff --git a/slirp/misc.c b/slirp/misc.c
index 0308a62..0bee864 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -215,7 +215,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                 setsockopt(so->s, SOL_SOCKET, SO_REUSEADDR, (char *)&opt, sizeof(int));
                 opt = 1;
                 setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, (char *)&opt, sizeof(int));
-		fd_nonblock(so->s);
+                socket_set_nonblock(so->s);
 
 		/* Append the telnet options now */
                 if (so->so_m != NULL && do_pty == 1)  {
@@ -267,50 +267,6 @@ u_sleep(int usec)
 	select(0, &fdset, &fdset, &fdset, &t);
 }
 
-/*
- * Set fd blocking and non-blocking
- */
-
-void
-fd_nonblock(int fd)
-{
-#ifdef FIONBIO
-#ifdef _WIN32
-        unsigned long opt = 1;
-#else
-        int opt = 1;
-#endif
-
-	ioctlsocket(fd, FIONBIO, &opt);
-#else
-	int opt;
-
-	opt = fcntl(fd, F_GETFL, 0);
-	opt |= O_NONBLOCK;
-	fcntl(fd, F_SETFL, opt);
-#endif
-}
-
-void
-fd_block(int fd)
-{
-#ifdef FIONBIO
-#ifdef _WIN32
-        unsigned long opt = 0;
-#else
-	int opt = 0;
-#endif
-
-	ioctlsocket(fd, FIONBIO, &opt);
-#else
-	int opt;
-
-	opt = fcntl(fd, F_GETFL, 0);
-	opt &= ~O_NONBLOCK;
-	fcntl(fd, F_SETFL, opt);
-#endif
-}
-
 void slirp_connection_info(Slirp *slirp, Monitor *mon)
 {
     const char * const tcpstates[] = {
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 143a238..5a3d7c2 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -329,7 +329,7 @@ int tcp_fconnect(struct socket *so)
     int opt, s=so->s;
     struct sockaddr_in addr;
 
-    fd_nonblock(s);
+    socket_set_nonblock(s);
     opt = 1;
     setsockopt(s,SOL_SOCKET,SO_REUSEADDR,(char *)&opt,sizeof(opt ));
     opt = 1;
@@ -417,7 +417,7 @@ tcp_connect(struct socket *inso)
 		tcp_close(sototcpcb(so)); /* This will sofree() as well */
 		return;
 	}
-	fd_nonblock(s);
+	socket_set_nonblock(s);
 	opt = 1;
 	setsockopt(s,SOL_SOCKET,SO_REUSEADDR,(char *)&opt,sizeof(int));
 	opt = 1;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 1/6] slirp: use socket_set_nonblock

[Stefan Weil]

Am 20.03.2012 10:49, schrieb Paolo Bonzini:
> Cc: Jan Kiszka<jan.kiszka@siemens.de>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   slirp/misc.c     |   46 +---------------------------------------------
>   slirp/tcp_subr.c |    4 ++--
>   2 files changed, 3 insertions(+), 47 deletions(-)
>    

Hi Paolo,

the compiler complains about the missing declaration of
socket_set_nonblock in slirp/tcp_subr.c: this file should
include qemu_socket.h.

Regards,
Stefan

Re: [Qemu-devel] [PATCH 1/6] slirp: use socket_set_nonblock

[Paolo Bonzini]

Il 21/03/2012 00:14, Stefan Weil ha scritto:
> Am 20.03.2012 10:49, schrieb Paolo Bonzini:
>> Cc: Jan Kiszka<jan.kiszka@siemens.de>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>> ---
>>   slirp/misc.c     |   46 +---------------------------------------------
>>   slirp/tcp_subr.c |    4 ++--
>>   2 files changed, 3 insertions(+), 47 deletions(-)
>>    
> 
> Hi Paolo,
> 
> the compiler complains about the missing declaration of
> socket_set_nonblock in slirp/tcp_subr.c: this file should
> include qemu_socket.h.

Ok, I'll send this patch and a prerequisite separately to Jan, then we
can tackle the main loop.

Paolo

[Qemu-devel] [PATCH 2/6] main loop: use msec-based timeout in glib_select_fill

[Paolo Bonzini]

The timeval-based timeout is not needed until we actually invoke select,
so compute it only then.  Also group the two calls that modify the
timeout, glib_select_fill and os_host_main_loop_wait.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 main-loop.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index db23de0..a3fd993 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -224,11 +224,11 @@ static int n_poll_fds;
 static int max_priority;
 
 static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
-                             fd_set *xfds, struct timeval *tv)
+                             fd_set *xfds, int *cur_timeout)
 {
     GMainContext *context = g_main_context_default();
     int i;
-    int timeout = 0, cur_timeout;
+    int timeout = 0;
 
     g_main_context_prepare(context, &max_priority);
 
@@ -253,10 +253,8 @@ static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
         }
     }
 
-    cur_timeout = (tv->tv_sec * 1000) + ((tv->tv_usec + 500) / 1000);
-    if (timeout >= 0 && timeout < cur_timeout) {
-        tv->tv_sec = timeout / 1000;
-        tv->tv_usec = (timeout % 1000) * 1000;
+    if (timeout >= 0 && timeout < *cur_timeout) {
+        *cur_timeout = timeout;
     }
 }
 
@@ -432,11 +430,6 @@ int main_loop_wait(int nonblocking)
         qemu_bh_update_timeout(&timeout);
     }
 
-    os_host_main_loop_wait(&timeout);
-
-    tv.tv_sec = timeout / 1000;
-    tv.tv_usec = (timeout % 1000) * 1000;
-
     /* poll any events */
     /* XXX: separate device handlers from system ones */
     nfds = -1;
@@ -448,7 +441,12 @@ int main_loop_wait(int nonblocking)
     slirp_select_fill(&nfds, &rfds, &wfds, &xfds);
 #endif
     qemu_iohandler_fill(&nfds, &rfds, &wfds, &xfds);
-    glib_select_fill(&nfds, &rfds, &wfds, &xfds, &tv);
+
+    glib_select_fill(&nfds, &rfds, &wfds, &xfds, &timeout);
+    os_host_main_loop_wait(&timeout);
+
+    tv.tv_sec = timeout / 1000;
+    tv.tv_usec = (timeout % 1000) * 1000;
 
     if (timeout > 0) {
         qemu_mutex_unlock_iothread();
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/6] main-loop: disable fd_set-based glib integration under w32

[Paolo Bonzini]

Using select with glib pollfds is wrong under w32.  Restrict
the code to the POSIX case.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 main-loop.c |   63 ++++++++++++++++++++++++++++++----------------------------
 1 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index a3fd993..dc6bdb5 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -218,7 +218,10 @@ int main_loop_init(void)
     return 0;
 }
 
+static fd_set rfds, wfds, xfds;
+static int nfds;
 
+#ifndef _WIN32
 static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
 static int n_poll_fds;
 static int max_priority;
@@ -286,7 +289,29 @@ static void glib_select_poll(fd_set *rfds, fd_set *wfds, fd_set *xfds,
     }
 }
 
-#ifdef _WIN32
+static int os_host_main_loop_wait(int timeout)
+{
+    struct timeval tv;
+    int ret;
+
+    glib_select_fill(&nfds, &rfds, &wfds, &xfds, &timeout);
+
+    if (timeout > 0) {
+        qemu_mutex_unlock_iothread();
+    }
+
+    tv.tv_sec = timeout / 1000;
+    tv.tv_usec = (timeout % 1000) * 1000;
+    ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
+
+    if (timeout > 0) {
+        qemu_mutex_lock_iothread();
+    }
+
+    glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));
+    return ret;
+}
+#else
 /***********************************************************/
 /* Polling handling */
 
@@ -367,10 +392,11 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
     }
 }
 
-static void os_host_main_loop_wait(int *timeout)
+static int os_host_main_loop_wait(int timeout)
 {
     int ret, ret2, i;
     PollingEntry *pe;
+    static struct timeval tv0;
 
     /* XXX: need to suppress polling by better using win32 events */
     ret = 0;
@@ -382,7 +408,7 @@ static void os_host_main_loop_wait(int *timeout)
         WaitObjects *w = &wait_objects;
 
         qemu_mutex_unlock_iothread();
-        ret = WaitForMultipleObjects(w->num, w->events, FALSE, *timeout);
+        ret = WaitForMultipleObjects(w->num, w->events, FALSE, timeout);
         qemu_mutex_lock_iothread();
         if (WAIT_OBJECT_0 + 0 <= ret && ret <= WAIT_OBJECT_0 + w->num - 1) {
             if (w->func[ret - WAIT_OBJECT_0]) {
@@ -408,20 +434,14 @@ static void os_host_main_loop_wait(int *timeout)
         }
     }
 
-    *timeout = 0;
-}
-#else
-static inline void os_host_main_loop_wait(int *timeout)
-{
+    ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv0);
+    return ret;
 }
 #endif
 
 int main_loop_wait(int nonblocking)
 {
-    fd_set rfds, wfds, xfds;
-    int ret, nfds;
-    struct timeval tv;
-    int timeout;
+    int ret, timeout;
 
     if (nonblocking) {
         timeout = 0;
@@ -441,24 +461,7 @@ int main_loop_wait(int nonblocking)
     slirp_select_fill(&nfds, &rfds, &wfds, &xfds);
 #endif
     qemu_iohandler_fill(&nfds, &rfds, &wfds, &xfds);
-
-    glib_select_fill(&nfds, &rfds, &wfds, &xfds, &timeout);
-    os_host_main_loop_wait(&timeout);
-
-    tv.tv_sec = timeout / 1000;
-    tv.tv_usec = (timeout % 1000) * 1000;
-
-    if (timeout > 0) {
-        qemu_mutex_unlock_iothread();
-    }
-
-    ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
-
-    if (timeout > 0) {
-        qemu_mutex_lock_iothread();
-    }
-
-    glib_select_poll(&rfds, &wfds, &xfds, (ret < 0));
+    ret = os_host_main_loop_wait(timeout);
     qemu_iohandler_poll(&rfds, &wfds, &xfds, ret);
 #ifdef CONFIG_SLIRP
     slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
-- 
1.7.7.6

[Qemu-devel] [PATCH 4/6] main-loop: interrupt wait when data arrives on a socket

[Paolo Bonzini]

Right now, the main loop is not interrupted when data arrives on a
socket.  To fix this, register each socket to interrupt the main loop
with WSAEventSelect.  This does not replace select, it only communicates
a change in socket state that requires a select call.

Since the interrupt fires only once per recv call, or only once
after a send call returns EWOULDBLOCK we can activate it on all events
unconditionally.  If QEMU is momentarily uninterested on some condition,
the main loop will not busy wait.  Instead, it may get one extra wakeup,
but then it will ignore the condition until progress occurs and/or
qemu_set_fd_handler is called to set a callback.  At this point the
condition will be tested via select and the callback will be invoked
even if it is still disabled on the event.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 main-loop.c   |   69 ++++++++++++++++++++++++++++++++++++--------------------
 main-loop.h   |    1 +
 oslib-win32.c |    3 ++
 3 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index dc6bdb5..7364074 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -392,10 +392,18 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
     }
 }
 
+void qemu_fd_register(int fd)
+{
+    WSAEventSelect(fd, qemu_event_handle, FD_READ | FD_ACCEPT | FD_CLOSE |
+                   FD_CONNECT | FD_WRITE | FD_OOB);
+}
+
 static int os_host_main_loop_wait(int timeout)
 {
     int ret, ret2, i;
     PollingEntry *pe;
+    int err;
+    WaitObjects *w = &wait_objects;
     static struct timeval tv0;
 
     /* XXX: need to suppress polling by better using win32 events */
@@ -403,38 +411,49 @@ static int os_host_main_loop_wait(int timeout)
     for (pe = first_polling_entry; pe != NULL; pe = pe->next) {
         ret |= pe->func(pe->opaque);
     }
-    if (ret == 0) {
-        int err;
-        WaitObjects *w = &wait_objects;
+    if (ret != 0) {
+        return ret;
+    }
 
-        qemu_mutex_unlock_iothread();
-        ret = WaitForMultipleObjects(w->num, w->events, FALSE, timeout);
-        qemu_mutex_lock_iothread();
-        if (WAIT_OBJECT_0 + 0 <= ret && ret <= WAIT_OBJECT_0 + w->num - 1) {
-            if (w->func[ret - WAIT_OBJECT_0]) {
-                w->func[ret - WAIT_OBJECT_0](w->opaque[ret - WAIT_OBJECT_0]);
-            }
+    if (nfds >= 0) {
+        ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv0);
+        if (ret != 0) {
+            timeout = 0;
+        }
+    }
 
-            /* Check for additional signaled events */
-            for (i = (ret - WAIT_OBJECT_0 + 1); i < w->num; i++) {
-                /* Check if event is signaled */
-                ret2 = WaitForSingleObject(w->events[i], 0);
-                if (ret2 == WAIT_OBJECT_0) {
-                    if (w->func[i]) {
-                        w->func[i](w->opaque[i]);
-                    }
-                } else if (ret2 != WAIT_TIMEOUT) {
-                    err = GetLastError();
-                    fprintf(stderr, "WaitForSingleObject error %d %d\n", i, err);
+    qemu_mutex_unlock_iothread();
+    ret = WaitForMultipleObjects(w->num, w->events, FALSE, timeout);
+    qemu_mutex_lock_iothread();
+    if (WAIT_OBJECT_0 + 0 <= ret && ret <= WAIT_OBJECT_0 + w->num - 1) {
+        if (w->func[ret - WAIT_OBJECT_0]) {
+            w->func[ret - WAIT_OBJECT_0](w->opaque[ret - WAIT_OBJECT_0]);
+        }
+
+        /* Check for additional signaled events */
+        for (i = (ret - WAIT_OBJECT_0 + 1); i < w->num; i++) {
+            /* Check if event is signaled */
+            ret2 = WaitForSingleObject(w->events[i], 0);
+            if (ret2 == WAIT_OBJECT_0) {
+                if (w->func[i]) {
+                    w->func[i](w->opaque[i]);
                 }
+            } else if (ret2 != WAIT_TIMEOUT) {
+                err = GetLastError();
+                fprintf(stderr, "WaitForSingleObject error %d %d\n", i, err);
             }
-        } else if (ret != WAIT_TIMEOUT) {
-            err = GetLastError();
-            fprintf(stderr, "WaitForMultipleObjects error %d %d\n", ret, err);
         }
+    } else if (ret != WAIT_TIMEOUT) {
+        err = GetLastError();
+        fprintf(stderr, "WaitForMultipleObjects error %d %d\n", ret, err);
     }
 
-    ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv0);
+
+    /* If an edge-triggered socket event occurred, select will return a
+     * positive result on the next iteration.  We do not need to do anything
+     * here.
+     */
+
     return ret;
 }
 #endif
diff --git a/main-loop.h b/main-loop.h
index 4987041..e743aa0 100644
--- a/main-loop.h
+++ b/main-loop.h
@@ -359,6 +359,7 @@ void qemu_mutex_unlock_iothread(void);
 
 /* internal interfaces */
 
+void qemu_fd_register(int fd);
 void qemu_iohandler_fill(int *pnfds, fd_set *readfds, fd_set *writefds, fd_set *xfds);
 void qemu_iohandler_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, int rc);
 
diff --git a/oslib-win32.c b/oslib-win32.c
index ce3021e..ffbc6d0 100644
--- a/oslib-win32.c
+++ b/oslib-win32.c
@@ -28,6 +28,7 @@
 #include <windows.h>
 #include "config-host.h"
 #include "sysemu.h"
+#include "main-loop.h"
 #include "trace.h"
 #include "qemu_socket.h"
 
@@ -76,6 +77,7 @@ void qemu_vfree(void *ptr)
 void socket_set_block(int fd)
 {
     unsigned long opt = 0;
+    WSAEventSelect(fd, NULL, 0);
     ioctlsocket(fd, FIONBIO, &opt);
 }
 
@@ -83,6 +85,7 @@ void socket_set_nonblock(int fd)
 {
     unsigned long opt = 1;
     ioctlsocket(fd, FIONBIO, &opt);
+    qemu_fd_register(fd);
 }
 
 int inet_aton(const char *cp, struct in_addr *ia)
-- 
1.7.7.6

[Qemu-devel] [PATCH 5/6] main-loop: replace WaitForMultipleObjects with g_poll

[Paolo Bonzini]

On w32, glib implements g_poll using WaitForMultipleObjects
or MsgWaitForMultipleObjects.  This means that we can simplify
our code by switching to g_poll, and at the same time prepare for
adding back glib sources.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 main-loop.c |   40 +++++++++++++++++-----------------------
 1 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index 7364074..4d02568 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -220,9 +220,9 @@ int main_loop_init(void)
 
 static fd_set rfds, wfds, xfds;
 static int nfds;
+static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
 
 #ifndef _WIN32
-static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
 static int n_poll_fds;
 static int max_priority;
 
@@ -351,6 +351,7 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque)
 /* Wait objects support */
 typedef struct WaitObjects {
     int num;
+    int revents[MAXIMUM_WAIT_OBJECTS + 1];
     HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
     WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1];
     void *opaque[MAXIMUM_WAIT_OBJECTS + 1];
@@ -367,6 +368,7 @@ int qemu_add_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
     w->events[w->num] = handle;
     w->func[w->num] = func;
     w->opaque[w->num] = opaque;
+    w->revents[w->num] = 0;
     w->num++;
     return 0;
 }
@@ -385,6 +387,7 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
             w->events[i] = w->events[i + 1];
             w->func[i] = w->func[i + 1];
             w->opaque[i] = w->opaque[i + 1];
+            w->revents[i] = w->revents[i + 1];
         }
     }
     if (found) {
@@ -400,9 +403,8 @@ void qemu_fd_register(int fd)
 
 static int os_host_main_loop_wait(int timeout)
 {
-    int ret, ret2, i;
+    int ret, i;
     PollingEntry *pe;
-    int err;
     WaitObjects *w = &wait_objects;
     static struct timeval tv0;
 
@@ -422,33 +424,25 @@ static int os_host_main_loop_wait(int timeout)
         }
     }
 
+    for (i = 0; i < w->num; i++) {
+        poll_fds[i].fd = (DWORD) w->events[i];
+        poll_fds[i].events = G_IO_IN;
+    }
+
     qemu_mutex_unlock_iothread();
-    ret = WaitForMultipleObjects(w->num, w->events, FALSE, timeout);
+    ret = g_poll(poll_fds, w->num, timeout);
     qemu_mutex_lock_iothread();
-    if (WAIT_OBJECT_0 + 0 <= ret && ret <= WAIT_OBJECT_0 + w->num - 1) {
-        if (w->func[ret - WAIT_OBJECT_0]) {
-            w->func[ret - WAIT_OBJECT_0](w->opaque[ret - WAIT_OBJECT_0]);
+    if (ret > 0) {
+        for (i = 0; i < w->num; i++) {
+            w->revents[i] = poll_fds[i].revents;
         }
-
-        /* Check for additional signaled events */
-        for (i = (ret - WAIT_OBJECT_0 + 1); i < w->num; i++) {
-            /* Check if event is signaled */
-            ret2 = WaitForSingleObject(w->events[i], 0);
-            if (ret2 == WAIT_OBJECT_0) {
-                if (w->func[i]) {
-                    w->func[i](w->opaque[i]);
-                }
-            } else if (ret2 != WAIT_TIMEOUT) {
-                err = GetLastError();
-                fprintf(stderr, "WaitForSingleObject error %d %d\n", i, err);
+        for (i = 0; i < w->num; i++) {
+            if (w->revents[i] && w->func[i]) {
+                w->func[i](w->opaque[i]);
             }
         }
-    } else if (ret != WAIT_TIMEOUT) {
-        err = GetLastError();
-        fprintf(stderr, "WaitForMultipleObjects error %d %d\n", ret, err);
     }
 
-
     /* If an edge-triggered socket event occurred, select will return a
      * positive result on the next iteration.  We do not need to do anything
      * here.
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 5/6] main-loop: replace WaitForMultipleObjects with g_poll

[Blue Swirl]

On Tue, Mar 20, 2012 at 09:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On w32, glib implements g_poll using WaitForMultipleObjects
> or MsgWaitForMultipleObjects.  This means that we can simplify
> our code by switching to g_poll, and at the same time prepare for
> adding back glib sources.

Unfortunately g_poll does not seem to be available in glib-2.0:
  CC    main-loop.o
/src/qemu/main-loop.c: In function 'os_host_main_loop_wait':
/src/qemu/main-loop.c:438: warning: implicit declaration of function 'g_poll'
/src/qemu/main-loop.c:438: warning: nested extern declaration of 'g_poll'
  LINK  qemu-img.exe
main-loop.o: In function `os_host_main_loop_wait':
/src/qemu/main-loop.c:438: undefined reference to `_g_poll'

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  main-loop.c |   40 +++++++++++++++++-----------------------
>  1 files changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/main-loop.c b/main-loop.c
> index 7364074..4d02568 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -220,9 +220,9 @@ int main_loop_init(void)
>
>  static fd_set rfds, wfds, xfds;
>  static int nfds;
> +static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
>
>  #ifndef _WIN32
> -static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
>  static int n_poll_fds;
>  static int max_priority;
>
> @@ -351,6 +351,7 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque)
>  /* Wait objects support */
>  typedef struct WaitObjects {
>     int num;
> +    int revents[MAXIMUM_WAIT_OBJECTS + 1];
>     HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
>     WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1];
>     void *opaque[MAXIMUM_WAIT_OBJECTS + 1];
> @@ -367,6 +368,7 @@ int qemu_add_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
>     w->events[w->num] = handle;
>     w->func[w->num] = func;
>     w->opaque[w->num] = opaque;
> +    w->revents[w->num] = 0;
>     w->num++;
>     return 0;
>  }
> @@ -385,6 +387,7 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
>             w->events[i] = w->events[i + 1];
>             w->func[i] = w->func[i + 1];
>             w->opaque[i] = w->opaque[i + 1];
> +            w->revents[i] = w->revents[i + 1];
>         }
>     }
>     if (found) {
> @@ -400,9 +403,8 @@ void qemu_fd_register(int fd)
>
>  static int os_host_main_loop_wait(int timeout)
>  {
> -    int ret, ret2, i;
> +    int ret, i;
>     PollingEntry *pe;
> -    int err;
>     WaitObjects *w = &wait_objects;
>     static struct timeval tv0;
>
> @@ -422,33 +424,25 @@ static int os_host_main_loop_wait(int timeout)
>         }
>     }
>
> +    for (i = 0; i < w->num; i++) {
> +        poll_fds[i].fd = (DWORD) w->events[i];
> +        poll_fds[i].events = G_IO_IN;
> +    }
> +
>     qemu_mutex_unlock_iothread();
> -    ret = WaitForMultipleObjects(w->num, w->events, FALSE, timeout);
> +    ret = g_poll(poll_fds, w->num, timeout);
>     qemu_mutex_lock_iothread();
> -    if (WAIT_OBJECT_0 + 0 <= ret && ret <= WAIT_OBJECT_0 + w->num - 1) {
> -        if (w->func[ret - WAIT_OBJECT_0]) {
> -            w->func[ret - WAIT_OBJECT_0](w->opaque[ret - WAIT_OBJECT_0]);
> +    if (ret > 0) {
> +        for (i = 0; i < w->num; i++) {
> +            w->revents[i] = poll_fds[i].revents;
>         }
> -
> -        /* Check for additional signaled events */
> -        for (i = (ret - WAIT_OBJECT_0 + 1); i < w->num; i++) {
> -            /* Check if event is signaled */
> -            ret2 = WaitForSingleObject(w->events[i], 0);
> -            if (ret2 == WAIT_OBJECT_0) {
> -                if (w->func[i]) {
> -                    w->func[i](w->opaque[i]);
> -                }
> -            } else if (ret2 != WAIT_TIMEOUT) {
> -                err = GetLastError();
> -                fprintf(stderr, "WaitForSingleObject error %d %d\n", i, err);
> +        for (i = 0; i < w->num; i++) {
> +            if (w->revents[i] && w->func[i]) {
> +                w->func[i](w->opaque[i]);
>             }
>         }
> -    } else if (ret != WAIT_TIMEOUT) {
> -        err = GetLastError();
> -        fprintf(stderr, "WaitForMultipleObjects error %d %d\n", ret, err);
>     }
>
> -
>     /* If an edge-triggered socket event occurred, select will return a
>      * positive result on the next iteration.  We do not need to do anything
>      * here.
> --
> 1.7.7.6
>
>
>

Re: [Qemu-devel] [PATCH 5/6] main-loop: replace WaitForMultipleObjects with g_poll

[Peter Maydell]

On 4 April 2012 21:44, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Mar 20, 2012 at 09:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On w32, glib implements g_poll using WaitForMultipleObjects
>> or MsgWaitForMultipleObjects.  This means that we can simplify
>> our code by switching to g_poll, and at the same time prepare for
>> adding back glib sources.
>
> Unfortunately g_poll does not seem to be available in glib-2.0:

Looks like it appeared in glib 2.19. We could maybe use an
autobuilder configured for whatever the lowest glib version
we support is, given how high the API churn seems to be :-/

-- PMM

Re: [Qemu-devel] [PATCH 5/6] main-loop: replace WaitForMultipleObjects with g_poll

[Paolo Bonzini]

----- Messaggio originale -----
> Da: "Blue Swirl" <blauwirbel@gmail.com>
> A: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, sw@weilnetz.de
> Inviato: Mercoledì, 4 aprile 2012 22:44:12
> Oggetto: Re: [Qemu-devel] [PATCH 5/6] main-loop: replace WaitForMultipleObjects with g_poll
>
> On Tue, Mar 20, 2012 at 09:49, Paolo Bonzini <pbonzini@redhat.com>
> wrote:
> > On w32, glib implements g_poll using WaitForMultipleObjects
> > or MsgWaitForMultipleObjects.  This means that we can simplify
> > our code by switching to g_poll, and at the same time prepare for
> > adding back glib sources.
>
> Unfortunately g_poll does not seem to be available in glib-2.0:
>   CC    main-loop.o
> /src/qemu/main-loop.c: In function 'os_host_main_loop_wait':
> /src/qemu/main-loop.c:438: warning: implicit declaration of function
> 'g_poll'
> /src/qemu/main-loop.c:438: warning: nested extern declaration of
> 'g_poll'
>   LINK  qemu-img.exe
> main-loop.o: In function `os_host_main_loop_wait':
> /src/qemu/main-loop.c:438: undefined reference to `_g_poll'

Supporting old glibs makes sense on Linux where we have enterprise
distributions to support, but not on Windows where glib is going to
be bundled with the executable anyway.  If anyone is interested they
can simply lift g_poll from glib and put it somewhere in the QEMU
sources.  I don't care enough; until we decide what to do, applying
patches 2-4 will fix some of the breakage.

(Also, the lowest common denominator for Linux would be 2.12, not 2.0.
Win32 g_poll was not available until 2.20).

Paolo

Re: [Qemu-devel] [PATCH 5/6] main-loop: replace WaitForMultipleObjects with g_poll

[Peter Maydell]

On 5 April 2012 09:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
> (Also, the lowest common denominator for Linux would be 2.12, not 2.0.

...this doesn't seem to be what configure checks for? AFAICT it
just wants something providing gthread-2.0.

-- PMM

Re: [Qemu-devel] [PATCH 5/6] main-loop: replace WaitForMultipleObjects with g_poll

[Paolo Bonzini]

> On 5 April 2012 09:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > (Also, the lowest common denominator for Linux would be 2.12, not
> > 2.0.
> 
> ...this doesn't seem to be what configure checks for? AFAICT it
> just wants something providing gthread-2.0.

Yes, that's correct.  Our checks are way too strict.

Paolo

[Qemu-devel] [PATCH 6/6] main-loop: integrate glib sources for w32

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 main-loop.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/main-loop.c b/main-loop.c
index 4d02568..7e163f9 100644
--- a/main-loop.c
+++ b/main-loop.c
@@ -221,11 +221,10 @@ int main_loop_init(void)
 static fd_set rfds, wfds, xfds;
 static int nfds;
 static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
-
-#ifndef _WIN32
 static int n_poll_fds;
 static int max_priority;
 
+#ifndef _WIN32
 static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
                              fd_set *xfds, int *cur_timeout)
 {
@@ -403,6 +402,7 @@ void qemu_fd_register(int fd)
 
 static int os_host_main_loop_wait(int timeout)
 {
+    GMainContext *context = g_main_context_default();
     int ret, i;
     PollingEntry *pe;
     WaitObjects *w = &wait_objects;
@@ -424,17 +424,22 @@ static int os_host_main_loop_wait(int timeout)
         }
     }
 
+    g_main_context_prepare(context, &max_priority);
+    n_poll_fds = g_main_context_query(context, max_priority, &timeout,
+                                      poll_fds, ARRAY_SIZE(poll_fds));
+    g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
+
     for (i = 0; i < w->num; i++) {
-        poll_fds[i].fd = (DWORD) w->events[i];
-        poll_fds[i].events = G_IO_IN;
+        poll_fds[n_poll_fds + i].fd = (DWORD) w->events[i];
+        poll_fds[n_poll_fds + i].events = G_IO_IN;
     }
 
     qemu_mutex_unlock_iothread();
-    ret = g_poll(poll_fds, w->num, timeout);
+    ret = g_poll(poll_fds, n_poll_fds + w->num, timeout);
     qemu_mutex_lock_iothread();
     if (ret > 0) {
         for (i = 0; i < w->num; i++) {
-            w->revents[i] = poll_fds[i].revents;
+            w->revents[i] = poll_fds[n_poll_fds + i].revents;
         }
         for (i = 0; i < w->num; i++) {
             if (w->revents[i] && w->func[i]) {
@@ -443,6 +448,10 @@ static int os_host_main_loop_wait(int timeout)
         }
     }
 
+    if (g_main_context_check(context, max_priority, poll_fds, n_poll_fds)) {
+        g_main_context_dispatch(context);
+    }
+
     /* If an edge-triggered socket event occurred, select will return a
      * positive result on the next iteration.  We do not need to do anything
      * here.
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 0/6] fix w32 sockets

[Stefan Weil]

Am 20.03.2012 10:49, schrieb Paolo Bonzini:
> The w32 main loop has been mostly broken by the introduction of the
> glib main loop. glib's g_poll does not use sockets on w32, so we
> need a separate approach.
>
> Patch 1 is a simple cleanup that is needed later in the series.
>
> Patch 2 and patch 3 completely separate the way the main loop waits
> on POSIX and w32 systems, and drop glib source handling from the w32
> main loop.
>
> Patch 4 fixes a longstanding bug in how sockets are handled, also
> simplifying the code in the process. On top of this simplification,
> patch 5 starts using g_poll in the w32 main loop and patch 6 adds
> back glib source handling.
>
> I didn't test this in the conditions explained in bug 916720, but I
> tested both a TCP monitor and an stdio monitor and both work (under
> Wine that is).
>
> Stefan, can you please take care of shepherding the patches in
> (pinging etc.)?

Hi Paolo,

it's really great that you addressed this main loop issue.

I tried to run an ARM system emulation for Raspberry Pi recently
and had much problems because QEMU was freezing very soon.

My host is Windows 7 (64 bit) running 32 and 64 bit versions
of QEMU with SDL. See https://bugs.launchpad.net/qemu/+bug/954099
for the command line and images.

A few seconds after start, QEMU freezes when I select screen 1 ("vga")
with the mouse (no more screen updates, no reaction on keyboard
input or window events).

If a switch to screen 3 (serial console 1) very fast after, I get
the message that the Linux kernel is uncompressed. It is also possible
to switch to screen 1 and watch the kernel boot messages, but it
freezes later.

Your patch series does not fix this, although I had expected that
it would, because I already noticed the problem with select. :-(

So we still have a problem to find.

Regards,

Stefan

Re: [Qemu-devel] [PATCH 0/6] fix w32 sockets

[Paolo Bonzini]

Il 20/03/2012 10:49, Paolo Bonzini ha scritto:
> The w32 main loop has been mostly broken by the introduction of the
> glib main loop.  glib's g_poll does not use sockets on w32, so we
> need a separate approach.
> 
> Patch 1 is a simple cleanup that is needed later in the series.
> 
> Patch 2 and patch 3 completely separate the way the main loop waits
> on POSIX and w32 systems, and drop glib source handling from the w32
> main loop.
> 
> Patch 4 fixes a longstanding bug in how sockets are handled, also
> simplifying the code in the process.  On top of this simplification,
> patch 5 starts using g_poll in the w32 main loop and patch 6 adds
> back glib source handling.
> 
> I didn't test this in the conditions explained in bug 916720, but I
> tested both a TCP monitor and an stdio monitor and both work (under
> Wine that is).
> 
> Stefan, can you please take care of shepherding the patches in
> (pinging etc.)?
> 
> Paolo Bonzini (6):
>   slirp: use socket_set_nonblock
>   main loop: use msec-based timeout in glib_select_fill
>   main-loop: disable fd_set-based glib integration under w32
>   main-loop: interrupt wait when data arrives on a socket
>   main-loop: replace WaitForMultipleObjects with g_poll
>   main-loop: integrate glib sources for w32

Patch 1 is now in separately through the slirp tree, so the other 5 can
now be applied.  Blue, can you commit them?

Paolo

Re: [Qemu-devel] [PATCH 0/6] fix w32 sockets

[Stefan Weil]

Am 03.04.2012 13:06, schrieb Paolo Bonzini:
> Il 20/03/2012 10:49, Paolo Bonzini ha scritto:
>> The w32 main loop has been mostly broken by the introduction of the
>> glib main loop. glib's g_poll does not use sockets on w32, so we
>> need a separate approach.
>>
>> Patch 1 is a simple cleanup that is needed later in the series.
>>
>> Patch 2 and patch 3 completely separate the way the main loop waits
>> on POSIX and w32 systems, and drop glib source handling from the w32
>> main loop.
>>
>> Patch 4 fixes a longstanding bug in how sockets are handled, also
>> simplifying the code in the process. On top of this simplification,
>> patch 5 starts using g_poll in the w32 main loop and patch 6 adds
>> back glib source handling.
>>
>> I didn't test this in the conditions explained in bug 916720, but I
>> tested both a TCP monitor and an stdio monitor and both work (under
>> Wine that is).
>>
>> Stefan, can you please take care of shepherding the patches in
>> (pinging etc.)?
>>
>> Paolo Bonzini (6):
>> slirp: use socket_set_nonblock
>> main loop: use msec-based timeout in glib_select_fill
>> main-loop: disable fd_set-based glib integration under w32
>> main-loop: interrupt wait when data arrives on a socket
>> main-loop: replace WaitForMultipleObjects with g_poll
>> main-loop: integrate glib sources for w32
>
> Patch 1 is now in separately through the slirp tree, so the other 5 can
> now be applied. Blue, can you commit them?
>
> Paolo

As I wrote in my last mail, these patches don't fix the main loop
problem(s). Nevertheless I think that they improve the code in
the right direction, therefore it's ok to commit them.

Regards,
Stefan