[libvirt] [PATCH 1/3] Don't use virSetCloseExec in event loop on Win32
The virSetCloseExec API returns -1 on Win32 since it cannot possibly work. Avoid calling it from the event loop since is not required in this case. * src/util/event_poll.c: Remove virSetCloseExec --- src/util/event_poll.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/util/event_poll.c b/src/util/event_poll.c index 91000e2..90788a4 100644 --- a/src/util/event_poll.c +++ b/src/util/event_poll.c @@ -658,10 +658,12 @@ int virEventPollInit(void) } if (pipe(eventLoop.wakeupfd) < 0 || - virSetNonBlock(eventLoop.wakeupfd[0]) < 0 || - virSetNonBlock(eventLoop.wakeupfd[1]) < 0 || +#ifndef WIN32 virSetCloseExec(eventLoop.wakeupfd[0]) < 0 || - virSetCloseExec(eventLoop.wakeupfd[1]) < 0) { + virSetCloseExec(eventLoop.wakeupfd[1]) < 0 || +#endif + virSetNonBlock(eventLoop.wakeupfd[0]) < 0 || + virSetNonBlock(eventLoop.wakeupfd[1]) < 0) { virReportSystemError(errno, "%s", _("Unable to setup wakeup pipe")); return -1; -- 1.7.4
GCC is a little confused about the cast of beginthread/beginthreadex from unsigned long -> void *. Go via an intermediate variable avoids the bogus warning, and makes the code a little cleaner * src/util/threads-win32.c: Avoid compiler warning in cast --- src/util/threads-win32.c | 18 +++++++++++------- 1 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/util/threads-win32.c b/src/util/threads-win32.c index ddb4737..5661437 100644 --- a/src/util/threads-win32.c +++ b/src/util/threads-win32.c @@ -262,6 +262,7 @@ int virThreadCreate(virThreadPtr thread, void *opaque) { struct virThreadArgs *args; + uintptr_t ret; if (VIR_ALLOC(args) < 0) return -1; @@ -271,17 +272,20 @@ int virThreadCreate(virThreadPtr thread, thread->joinable = joinable; if (joinable) { - thread->thread = (HANDLE)_beginthreadex(NULL, 0, - virThreadHelperJoinable, - args, 0, NULL); - if (thread->thread == 0) + ret = _beginthreadex(NULL, 0, + virThreadHelperJoinable, + args, 0, NULL); + if (ret == 0) return -1; } else { - thread->thread = (HANDLE)_beginthread(virThreadHelperDaemon, - 0, args); - if (thread->thread == (HANDLE)-1L) + ret = _beginthread(virThreadHelperDaemon, + 0, args); + if (ret == -1L) return -1; } + + thread->thread = (HANDLE)ret; + return 0; } -- 1.7.4
On 04/05/2011 09:03 AM, Daniel P. Berrange wrote:
GCC is a little confused about the cast of beginthread/beginthreadex from unsigned long -> void *. Go via an intermediate variable avoids the bogus warning, and makes the code a little cleaner
* src/util/threads-win32.c: Avoid compiler warning in cast --- src/util/threads-win32.c | 18 +++++++++++------- 1 files changed, 11 insertions(+), 7 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The GCC Win32 compiler will claim to support -fstack-protector, but if it actually gets triggered by a suitable code pattern, linking will fail. Other non-Linux OS likely suffer the same way with gcc. * m4/virt-compile-warnings.m4: Only use stack protector when the build target is Linux. --- m4/virt-compile-warnings.m4 | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-) diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index 7e252d5..819da8f 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -98,11 +98,17 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ # Extra special flags gl_WARN_ADD([-Wp,-D_FORTIFY_SOURCE=2]) - dnl Fedora only uses -fstack-protector, but doesn't seem to - dnl be great overhead in adding -fstack-protector-all instead - dnl gl_WARN_ADD([-fstack-protector]) - gl_WARN_ADD([-fstack-protector-all]) - gl_WARN_ADD([--param=ssp-buffer-size=4]) + dnl -fstack-protector stuff passes gl_WARN_ADD with gcc + dnl on Mingw32, but fails when actually used + case $host in + *-*-linux*) + dnl Fedora only uses -fstack-protector, but doesn't seem to + dnl be great overhead in adding -fstack-protector-all instead + dnl gl_WARN_ADD([-fstack-protector]) + gl_WARN_ADD([-fstack-protector-all]) + gl_WARN_ADD([--param=ssp-buffer-size=4]) + ;; + esac gl_WARN_ADD([-fexceptions]) gl_WARN_ADD([-fasynchronous-unwind-tables]) gl_WARN_ADD([-fdiagnostics-show-option]) -- 1.7.4
On 04/05/2011 09:03 AM, Daniel P. Berrange wrote:
The GCC Win32 compiler will claim to support -fstack-protector, but if it actually gets triggered by a suitable code pattern, linking will fail. Other non-Linux OS likely suffer the same way with gcc.
* m4/virt-compile-warnings.m4: Only use stack protector when the build target is Linux. --- m4/virt-compile-warnings.m4 | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/05/2011 09:03 AM, Daniel P. Berrange wrote:
The virSetCloseExec API returns -1 on Win32 since it cannot possibly work. Avoid calling it from the event loop since is not required in this case.
Well, it _could_ work if gnulib would relax a couple modules to LGPLv2+ instead of their current LGPLv3+. Meanwhile, mingw _does_ let us create a pipe already non-inheritible (that is, pipe2(fds, O_CLOEXEC) _does_ work on mingw as an LGPLv2+ gnulib solution). If you don't mind, I'd rather conditionally NAK this patch and try the pipe2 approach instead, since the added in-function #ifdefs detract from readability. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/05/2011 09:31 AM, Eric Blake wrote:
On 04/05/2011 09:03 AM, Daniel P. Berrange wrote:
The virSetCloseExec API returns -1 on Win32 since it cannot possibly work. Avoid calling it from the event loop since is not required in this case.
Well, it _could_ work if gnulib would relax a couple modules to LGPLv2+ instead of their current LGPLv3+.
Meanwhile, mingw _does_ let us create a pipe already non-inheritible (that is, pipe2(fds, O_CLOEXEC) _does_ work on mingw as an LGPLv2+ gnulib solution).
If you don't mind, I'd rather conditionally NAK this patch and try the pipe2 approach instead, since the added in-function #ifdefs detract from readability.
Like so, except that this is pending a gnulib license change first (I was wrong, gnulib's pipe2 is still LGPLv3+ at the moment). diff --git i/bootstrap.conf w/bootstrap.conf index 11d2199..95136ac 100644 --- i/bootstrap.conf +++ w/bootstrap.conf @@ -54,6 +54,7 @@ nonblocking perror physmem pipe-posix +pipe2 poll posix-shell pthread diff --git i/src/util/event_poll.c w/src/util/event_poll.c index 91000e2..cd1ff4a 100644 --- i/src/util/event_poll.c +++ w/src/util/event_poll.c @@ -29,6 +29,7 @@ #include <sys/time.h> #include <errno.h> #include <unistd.h> +#include <fcntl.h> #include "threads.h" #include "logging.h" @@ -657,11 +658,7 @@ int virEventPollInit(void) return -1; } - if (pipe(eventLoop.wakeupfd) < 0 || - virSetNonBlock(eventLoop.wakeupfd[0]) < 0 || - virSetNonBlock(eventLoop.wakeupfd[1]) < 0 || - virSetCloseExec(eventLoop.wakeupfd[0]) < 0 || - virSetCloseExec(eventLoop.wakeupfd[1]) < 0) { + if (pipe2(eventLoop.wakeupfd, O_CLOEXEC | O_NONBLOCK) < 0) { virReportSystemError(errno, "%s", _("Unable to setup wakeup pipe")); return -1; -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* .gnulib: Update to latest, for pipe2. * bootstrap.conf (gnulib_modules): Add pipe2. * src/util/event_poll.c (virEventPollInit): Use it, to avoid problematic virSetCloseExec on mingw. --- The gnulib changes to pipe2 are now in. * .gnulib f796520...2255b86 (4):
verify: use _Static_assert if available Remove leftover generated .h files after config.status changed. Ensure to rebuild generated .h files when config.status has changed. pipe2: Relicense under LGPLv2+.
This assumes that my earlier .gnulib modification patch is also in: https://www.redhat.com/archives/libvir-list/2011-April/msg00297.html although I can rebase as needed. .gnulib | 2 +- bootstrap.conf | 3 ++- src/util/event_poll.c | 7 ++----- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/.gnulib b/.gnulib index f796520..2255b86 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit f79652003534e496bca1e49922ca521f12ca8051 +Subproject commit 2255b865130df1feadea60779babd6889285186e diff --git a/bootstrap.conf b/bootstrap.conf index 11d2199..95136ac 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -54,6 +54,7 @@ nonblocking perror physmem pipe-posix +pipe2 poll posix-shell pthread diff --git a/src/util/event_poll.c b/src/util/event_poll.c index 91000e2..cd1ff4a 100644 --- a/src/util/event_poll.c +++ b/src/util/event_poll.c @@ -29,6 +29,7 @@ #include <sys/time.h> #include <errno.h> #include <unistd.h> +#include <fcntl.h> #include "threads.h" #include "logging.h" @@ -657,11 +658,7 @@ int virEventPollInit(void) return -1; } - if (pipe(eventLoop.wakeupfd) < 0 || - virSetNonBlock(eventLoop.wakeupfd[0]) < 0 || - virSetNonBlock(eventLoop.wakeupfd[1]) < 0 || - virSetCloseExec(eventLoop.wakeupfd[0]) < 0 || - virSetCloseExec(eventLoop.wakeupfd[1]) < 0) { + if (pipe2(eventLoop.wakeupfd, O_CLOEXEC | O_NONBLOCK) < 0) { virReportSystemError(errno, "%s", _("Unable to setup wakeup pipe")); return -1; -- 1.7.4
On 04/06/2011 03:47 PM, Eric Blake wrote:
* .gnulib: Update to latest, for pipe2. * bootstrap.conf (gnulib_modules): Add pipe2. * src/util/event_poll.c (virEventPollInit): Use it, to avoid problematic virSetCloseExec on mingw. ---
The gnulib changes to pipe2 are now in.
* .gnulib f796520...2255b86 (4):
verify: use _Static_assert if available Remove leftover generated .h files after config.status changed. Ensure to rebuild generated .h files when config.status has changed. pipe2: Relicense under LGPLv2+.
This assumes that my earlier .gnulib modification patch is also in: https://www.redhat.com/archives/libvir-list/2011-April/msg00297.html although I can rebase as needed.
.gnulib | 2 +- bootstrap.conf | 3 ++- src/util/event_poll.c | 7 ++----- 3 files changed, 5 insertions(+), 7 deletions(-)
ACK.
On 04/07/2011 09:02 AM, Laine Stump wrote:
On 04/06/2011 03:47 PM, Eric Blake wrote:
* .gnulib: Update to latest, for pipe2. * bootstrap.conf (gnulib_modules): Add pipe2. * src/util/event_poll.c (virEventPollInit): Use it, to avoid problematic virSetCloseExec on mingw. ---
The gnulib changes to pipe2 are now in.
* .gnulib f796520...2255b86 (4):
verify: use _Static_assert if available Remove leftover generated .h files after config.status changed. Ensure to rebuild generated .h files when config.status has changed. pipe2: Relicense under LGPLv2+.
This assumes that my earlier .gnulib modification patch is also in: https://www.redhat.com/archives/libvir-list/2011-April/msg00297.html although I can rebase as needed.
.gnulib | 2 +- bootstrap.conf | 3 ++- src/util/event_poll.c | 7 ++----- 3 files changed, 5 insertions(+), 7 deletions(-)
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Aargh; commit 8ae5dfd still didn't fix the mingw problem, because gnulib defined O_NONBLOCK to 0 for just mingw. I've now fixed that in gnulib, but we need the latest pipe2 for libvirt to work. * .gnulib: Update to latest, for pipe2 fixes. --- Pushing under the trivial rule (it's a one-liner, right? :) * .gnulib 9cc9910...4a1579d (5):
nonblocking: reduce dependency pipe2: fix O_NONBLOCK support on mingw fcntl-h: fix O_ACCMODE on cygwin pipe-filter: drop O_NONBLOCK workarounds nonblocking: provide O_NONBLOCK for mingw
.gnulib | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/.gnulib b/.gnulib index 9cc9910..4a1579d 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 9cc991025d6139a3a8f3e0f1570bf39a66f3aafa +Subproject commit 4a1579d7560659ef5723325726eda14490a967f6 -- 1.7.4.2
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump