util/main-loop-stub.c | 26 ++++++++++++++++++++++++++ util/meson.build | 2 ++ 2 files changed, 28 insertions(+) create mode 100644 util/main-loop-stub.c
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Fixes linking:
x86_64-w64-mingw32-gcc -o tests/test-qapi-util.exe version.rc_version.o tests/test-qapi-util.exe.p/test-qapi-util.c.obj -Wl,--allow-shlib-undefined -Wl,--nxcompat -Wl,--no-seh -Wl,--dynamicbase -Wl,--warn-common -m64 -fstack-protector-strong -Wl,--start-group libqemuutil.a -pthread -L/usr/x86_64-w64-mingw32/sys-root/mingw/lib -lgnutls -lwinmm -lm -L/usr/x86_64-w64-mingw32/sys-root/mingw/lib -lgthread-2.0 -lglib-2.0 -lintl -lws2_32 -mconsole -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 -Wl,--end-group
/usr/lib/gcc/x86_64-w64-mingw32/10.2.1/../../../../x86_64-w64-mingw32/bin/ld: libqemuutil.a(util_oslib-win32.c.obj): in function `qemu_try_set_nonblock':
/home/elmarco/src/qemu/buildw/../util/oslib-win32.c:224: undefined reference to `qemu_fd_register'
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
util/main-loop-stub.c | 26 ++++++++++++++++++++++++++
util/meson.build | 2 ++
2 files changed, 28 insertions(+)
create mode 100644 util/main-loop-stub.c
diff --git a/util/main-loop-stub.c b/util/main-loop-stub.c
new file mode 100644
index 0000000000..b3e175ade5
--- /dev/null
+++ b/util/main-loop-stub.c
@@ -0,0 +1,26 @@
+/*
+ * QEMU main loop stub impl
+ *
+ * Copyright (c) 2020 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+
+void qemu_fd_register(int fd)
+{
+}
diff --git a/util/meson.build b/util/meson.build
index f359af0d46..462b79a61a 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -79,4 +79,6 @@ if have_block
util_ss.add(when: 'CONFIG_INOTIFY1', if_true: files('filemonitor-inotify.c'),
if_false: files('filemonitor-stub.c'))
util_ss.add(when: 'CONFIG_LINUX', if_true: files('vfio-helpers.c'))
+else
+ util_ss.add(files('main-loop-stub.c'))
endif
--
2.29.0
On 12/17/20 11:44 AM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Fixes linking: > x86_64-w64-mingw32-gcc -o tests/test-qapi-util.exe version.rc_version.o tests/test-qapi-util.exe.p/test-qapi-util.c.obj -Wl,--allow-shlib-undefined -Wl,--nxcompat -Wl,--no-seh -Wl,--dynamicbase -Wl,--warn-common -m64 -fstack-protector-strong -Wl,--start-group libqemuutil.a -pthread -L/usr/x86_64-w64-mingw32/sys-root/mingw/lib -lgnutls -lwinmm -lm -L/usr/x86_64-w64-mingw32/sys-root/mingw/lib -lgthread-2.0 -lglib-2.0 -lintl -lws2_32 -mconsole -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 -Wl,--end-group > /usr/lib/gcc/x86_64-w64-mingw32/10.2.1/../../../../x86_64-w64-mingw32/bin/ld: libqemuutil.a(util_oslib-win32.c.obj): in function `qemu_try_set_nonblock': > /home/elmarco/src/qemu/buildw/../util/oslib-win32.c:224: undefined reference to `qemu_fd_register' > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > util/main-loop-stub.c | 26 ++++++++++++++++++++++++++ > util/meson.build | 2 ++ > 2 files changed, 28 insertions(+) > create mode 100644 util/main-loop-stub.c > > diff --git a/util/main-loop-stub.c b/util/main-loop-stub.c > new file mode 100644 > index 0000000000..b3e175ade5 > --- /dev/null > +++ b/util/main-loop-stub.c > @@ -0,0 +1,26 @@ > +/* > + * QEMU main loop stub impl > + * > + * Copyright (c) 2020 Red Hat, Inc. > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, see <http://www.gnu.org/licenses/>. > + * > + */ > + > +#include "qemu/osdep.h" > +#include "qemu/main-loop.h" > + > +void qemu_fd_register(int fd) > +{ > +} > diff --git a/util/meson.build b/util/meson.build > index f359af0d46..462b79a61a 100644 > --- a/util/meson.build > +++ b/util/meson.build > @@ -79,4 +79,6 @@ if have_block > util_ss.add(when: 'CONFIG_INOTIFY1', if_true: files('filemonitor-inotify.c'), > if_false: files('filemonitor-stub.c')) > util_ss.add(when: 'CONFIG_LINUX', if_true: files('vfio-helpers.c')) > +else > + util_ss.add(files('main-loop-stub.c')) > endif > Is the root cause elsewhere though? I don't like stubs very much, because often they are introduced as the easy way out of a problem instead of doing the necessary refactoring, and they end up confusing the hell out of someone trying to understand what is actually used where, never mind trying to debug the linker errors. There is already an bunch of #ifndef _WIN32, #else , ... in util/main-loop.c (quite a bunch of them really), is that what actually needs reworking, and putting the pieces together in the build system in a way that makes sense? Ciao, thanks, Claudio
Hi On Thu, Dec 17, 2020 at 3:33 PM Claudio Fontana <cfontana@suse.de> wrote: > On 12/17/20 11:44 AM, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Fixes linking: > > x86_64-w64-mingw32-gcc -o tests/test-qapi-util.exe version.rc_version.o > tests/test-qapi-util.exe.p/test-qapi-util.c.obj -Wl,--allow-shlib-undefined > -Wl,--nxcompat -Wl,--no-seh -Wl,--dynamicbase -Wl,--warn-common -m64 > -fstack-protector-strong -Wl,--start-group libqemuutil.a -pthread > -L/usr/x86_64-w64-mingw32/sys-root/mingw/lib -lgnutls -lwinmm -lm > -L/usr/x86_64-w64-mingw32/sys-root/mingw/lib -lgthread-2.0 -lglib-2.0 > -lintl -lws2_32 -mconsole -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 > -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 -Wl,--end-group > > > /usr/lib/gcc/x86_64-w64-mingw32/10.2.1/../../../../x86_64-w64-mingw32/bin/ld: > libqemuutil.a(util_oslib-win32.c.obj): in function `qemu_try_set_nonblock': > > /home/elmarco/src/qemu/buildw/../util/oslib-win32.c:224: undefined > reference to `qemu_fd_register' > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > util/main-loop-stub.c | 26 ++++++++++++++++++++++++++ > > util/meson.build | 2 ++ > > 2 files changed, 28 insertions(+) > > create mode 100644 util/main-loop-stub.c > > > > diff --git a/util/main-loop-stub.c b/util/main-loop-stub.c > > new file mode 100644 > > index 0000000000..b3e175ade5 > > --- /dev/null > > +++ b/util/main-loop-stub.c > > @@ -0,0 +1,26 @@ > > +/* > > + * QEMU main loop stub impl > > + * > > + * Copyright (c) 2020 Red Hat, Inc. > > + * > > + * This library is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU Lesser General Public > > + * License as published by the Free Software Foundation; either > > + * version 2.1 of the License, or (at your option) any later version. > > + * > > + * This library is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * Lesser General Public License for more details. > > + * > > + * You should have received a copy of the GNU Lesser General Public > > + * License along with this library; if not, see < > http://www.gnu.org/licenses/>. > > + * > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "qemu/main-loop.h" > > + > > +void qemu_fd_register(int fd) > > +{ > > +} > > diff --git a/util/meson.build b/util/meson.build > > index f359af0d46..462b79a61a 100644 > > --- a/util/meson.build > > +++ b/util/meson.build > > @@ -79,4 +79,6 @@ if have_block > > util_ss.add(when: 'CONFIG_INOTIFY1', if_true: > files('filemonitor-inotify.c'), > > if_false: > files('filemonitor-stub.c')) > > util_ss.add(when: 'CONFIG_LINUX', if_true: files('vfio-helpers.c')) > > +else > > + util_ss.add(files('main-loop-stub.c')) > > endif > > > > Is the root cause elsewhere though? > > I don't like stubs very much, because often they are introduced as the > easy way out of a problem instead of doing the necessary refactoring, > and they end up confusing the hell out of someone trying to understand > what is actually used where, never mind trying to debug the linker errors. > > There is already an bunch of #ifndef _WIN32, #else , ... in > util/main-loop.c (quite a bunch of them really), > is that what actually needs reworking, and putting the pieces together in > the build system in a way that makes sense? > > I am not sure we can improve it so much. qemu_fd_register() is called from qemu_set_nonblock() and net/slirp.c. It is necessary for win32 util/main-loop.c. Eventually, we could move qemu_fd_register() in its own unit, but then we will probably need more stubs or configure knobs for the main-loop symbols. I don't think it's worth, but maybe I am missing something and I should try it.
On 17/12/20 12:32, Claudio Fontana wrote: > Is the root cause elsewhere though? > > I don't like stubs very much, because often they are introduced as the easy way out of a problem instead of doing the necessary refactoring, > and they end up confusing the hell out of someone trying to understand what is actually used where, never mind trying to debug the linker errors. > > There is already an bunch of #ifndef _WIN32, #else , ... in util/main-loop.c (quite a bunch of them really), > is that what actually needs reworking, and putting the pieces together in the build system in a way that makes sense? qemu_fd_register is almost not needed at all, since we have WSAEventSelect(node->pfd.fd, event, bitmask); in aio_set_fd_handler. I think we can remove the call to qemu_fd_register from qemu_try_set_nonblock, and that should fix the issue as well. Paolo
Hi On Thu, Dec 17, 2020 at 3:47 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 17/12/20 12:32, Claudio Fontana wrote: > > Is the root cause elsewhere though? > > > > I don't like stubs very much, because often they are introduced as the > easy way out of a problem instead of doing the necessary refactoring, > > and they end up confusing the hell out of someone trying to understand > what is actually used where, never mind trying to debug the linker errors. > > > > There is already an bunch of #ifndef _WIN32, #else , ... in > util/main-loop.c (quite a bunch of them really), > > is that what actually needs reworking, and putting the pieces together > in the build system in a way that makes sense? > > qemu_fd_register is almost not needed at all, since we have > > WSAEventSelect(node->pfd.fd, event, bitmask); > > in aio_set_fd_handler. I think we can remove the call to > qemu_fd_register from qemu_try_set_nonblock, and that should fix the > issue as well. > That's tricky to say whether this won't introduce regression. For most fds from qemu, if they use aio_set_fd_handler, that should be ok. But what about other fds? For examples, the ones from slirp? In fact, I don't understand how it could work today. We are passing socket() fd directly to g_poll(). But according to the documentation: * On Win32, the fd in a GPollFD should be Win32 HANDLE (*not* a file * descriptor as provided by the C runtime) that can be used by * MsgWaitForMultipleObjects. This does *not* include file handles * from CreateFile, SOCKETs, nor pipe handles. (But you can use * WSAEventSelect to signal events when a SOCKET is readable). And MsgWaitForMultipleObjects doesn't mention SOCKET as being valid handles to wait for. But when I run qemu with slirp, with or without qemu_fd_register, I don't see any error or regression. Am I missing something?
On 18/12/20 14:01, Marc-André Lureau wrote: >> in aio_set_fd_handler. I think we can remove the call to >> qemu_fd_register from qemu_try_set_nonblock, and that should fix the >> issue as well. > > That's tricky to say whether this won't introduce regression. For most > fds from qemu, if they use aio_set_fd_handler, that should be ok. > > But what about other fds? For examples, the ones from slirp? slirp already calls qemu_fd_register, see net_slirp_register_poll_fd > In fact, I > don't understand how it could work today. We are passing socket() fd > directly to g_poll(). But according to the documentation: > > * On Win32, the fd in a GPollFD should be Win32 HANDLE (*not* a file > * descriptor as provided by the C runtime) that can be used by > * MsgWaitForMultipleObjects. This does *not* include file handles > * from CreateFile, SOCKETs, nor pipe handles. (But you can use > * WSAEventSelect to signal events when a SOCKET is readable). > > And MsgWaitForMultipleObjects doesn't mention SOCKET as being valid > handles to wait for. No, it's more complicated. On Win32, gpollfds is only used for sockets (despite the name!), while poll_fds is used for prepare/query/g_poll/check. What we do is basically the same that QIOChannel and aio-win32.c already do, just with more indirection to fit the SLIRP callback API: - main_loop_wait calls net_slirp_poll_notify, which asks SLIRP to send back the list of file descriptors through the net_slirp_add_poll callback. - the file descriptors are stored in the gpollfds global. - os_host_main_loop_wait does a select on the sockets with 0 timeout - if no socket is ready, g_poll is done with the original timeout (otherwise the timeout is zeroed) - the sockets were registered with WSAEventSelect in net_slirp_register_poll_fd, so they interrupt the subsequent g_poll if data comes in. I don't see any other use of MainLoopPoll, so all non-SLIRP sockets should be going through {qemu,aio}_set_fd_handler. In particular this is the case for all of chardev/ (which uses QIOChannel), io/ and net/. These are all the other users of qemu_set_nonblock and qemu_try_set_nonblock. Paolo > But when I run qemu with slirp, with or without qemu_fd_register, I > don't see any error or regression. > > Am I missing something?
Hi On Fri, Dec 18, 2020 at 5:24 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 18/12/20 14:01, Marc-André Lureau wrote: > >> in aio_set_fd_handler. I think we can remove the call to > >> qemu_fd_register from qemu_try_set_nonblock, and that should fix the > >> issue as well. > > > > That's tricky to say whether this won't introduce regression. For most > > fds from qemu, if they use aio_set_fd_handler, that should be ok. > > > > But what about other fds? For examples, the ones from slirp? > > slirp already calls qemu_fd_register, see net_slirp_register_poll_fd > > > In fact, I > > don't understand how it could work today. We are passing socket() fd > > directly to g_poll(). But according to the documentation: > > > > * On Win32, the fd in a GPollFD should be Win32 HANDLE (*not* a file > > * descriptor as provided by the C runtime) that can be used by > > * MsgWaitForMultipleObjects. This does *not* include file handles > > * from CreateFile, SOCKETs, nor pipe handles. (But you can use > > * WSAEventSelect to signal events when a SOCKET is readable). > > > > And MsgWaitForMultipleObjects doesn't mention SOCKET as being valid > > handles to wait for. > > No, it's more complicated. On Win32, gpollfds is only used for sockets > (despite the name!), while poll_fds is used for prepare/query/g_poll/check. > > What we do is basically the same that QIOChannel and aio-win32.c already > do, just with more indirection to fit the SLIRP callback API: > > - main_loop_wait calls net_slirp_poll_notify, which asks SLIRP to send > back the list of file descriptors through the net_slirp_add_poll callback. > > - the file descriptors are stored in the gpollfds global. > > - os_host_main_loop_wait does a select on the sockets with 0 timeout > > - if no socket is ready, g_poll is done with the original timeout > (otherwise the timeout is zeroed) > > - the sockets were registered with WSAEventSelect in > net_slirp_register_poll_fd, so they interrupt the subsequent g_poll if > data comes in. > > Ah thanks, I mixed the unix and the win32 versions. > I don't see any other use of MainLoopPoll, so all non-SLIRP sockets > should be going through {qemu,aio}_set_fd_handler. In particular this > is the case for all of chardev/ (which uses QIOChannel), io/ and net/. > These are all the other users of qemu_set_nonblock and > qemu_try_set_nonblock. > > Ok, I guess we can simply register fd to be a win32-specific call for slirp then. > Paolo > > > But when I run qemu with slirp, with or without qemu_fd_register, I > > don't see any error or regression. > > > > Am I missing something? > > > -- Marc-André Lureau
Hi On Fri, Dec 18, 2020 at 5:03 PM Marc-André Lureau < marcandre.lureau@redhat.com> wrote: > Hi > > On Thu, Dec 17, 2020 at 3:47 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 17/12/20 12:32, Claudio Fontana wrote: >> > Is the root cause elsewhere though? >> > >> > I don't like stubs very much, because often they are introduced as the >> easy way out of a problem instead of doing the necessary refactoring, >> > and they end up confusing the hell out of someone trying to understand >> what is actually used where, never mind trying to debug the linker errors. >> > >> > There is already an bunch of #ifndef _WIN32, #else , ... in >> util/main-loop.c (quite a bunch of them really), >> > is that what actually needs reworking, and putting the pieces together >> in the build system in a way that makes sense? >> >> qemu_fd_register is almost not needed at all, since we have >> >> WSAEventSelect(node->pfd.fd, event, bitmask); >> >> in aio_set_fd_handler. I think we can remove the call to >> qemu_fd_register from qemu_try_set_nonblock, and that should fix the >> issue as well. >> > > That's tricky to say whether this won't introduce regression. For most fds > from qemu, if they use aio_set_fd_handler, that should be ok. > > But what about other fds? For examples, the ones from slirp? In fact, I > don't understand how it could work today. We are passing socket() fd > directly to g_poll(). But according to the documentation: > > * On Win32, the fd in a GPollFD should be Win32 HANDLE (*not* a file > * descriptor as provided by the C runtime) that can be used by > * MsgWaitForMultipleObjects. This does *not* include file handles > * from CreateFile, SOCKETs, nor pipe handles. (But you can use > * WSAEventSelect to signal events when a SOCKET is readable). > > And MsgWaitForMultipleObjects doesn't mention SOCKET as being valid > handles to wait for. > > But when I run qemu with slirp, with or without qemu_fd_register, I don't > see any error or regression. > > Am I missing something? > I wrote a simple program to check the behaviour of WaitForMultipleObjects: #include <winsock2.h> #include <glib.h> int main(int argc, char *argv[]) { WSADATA Data; WSAStartup(MAKEWORD(2, 0), &Data); int fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); g_print("sock fd %d\n", fd); HANDLE handles[1] = { (HANDLE)fd }; DWORD res = WaitForMultipleObjects(1, handles, FALSE, 1000); g_print("wait res %x\n", res); return 0; } Indeed, it doesn not complain about SOCKET as HANDLE here. But it will return immediately indicating that the socket has events. We probably want to fix this. slirp should only fill pollfd with handles that are acceptable for WaitFor API I suppose. Correct? -- Marc-André Lureau
© 2016 - 2024 Red Hat, Inc.