[PATCH] build-sys: fix win32 compilation with --target-list=''

[PATCH] build-sys: fix win32 compilation with --target-list=''

[marcandre.lureau]

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

Re: [PATCH] build-sys: fix win32 compilation with --target-list=''

[Claudio Fontana]

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

Re: [PATCH] build-sys: fix win32 compilation with --target-list=''

[Marc-André Lureau]

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

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.

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

Re: [PATCH] build-sys: fix win32 compilation with --target-list=''

[Paolo Bonzini]

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

Re: [PATCH] build-sys: fix win32 compilation with --target-list=''

[Marc-André Lureau]

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

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?

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

Re: [PATCH] build-sys: fix win32 compilation with --target-list=''

[Paolo Bonzini]

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?

Re: [PATCH] build-sys: fix win32 compilation with --target-list=''

[Marc-André Lureau]

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

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

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

Re: [PATCH] build-sys: fix win32 compilation with --target-list=''

[Marc-André Lureau]

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

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

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