[libvirt] [PATCH] Fix Win32 portability problems
The network filter / snapshot / hooks code introduced some non-portable pices that broke the win32 build * configure.ac: Check for net/ethernet.h required by nwfile config parsing code * src/conf/nwfilter_conf.c: Define ethernet protocol constants if net/ethernet.h is missing * src/util/hooks.c: Disable hooks build on Win32 since it lacks fork/exec/pipe * src/util/threads-win32.c: Fix unchecked return value * tools/virsh.c: Disable SIGPIPE on Win32 since it doesn't exist. Fix non-portable strftime() formats --- configure.ac | 2 +- src/conf/nwfilter_conf.c | 20 ++++++++++++++++++++ src/util/hooks.c | 16 ++++++++++++++++ src/util/threads-win32.c | 2 +- tools/virsh.c | 7 ++++++- 5 files changed, 44 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index e13961e..9a133e4 100644 --- a/configure.ac +++ b/configure.ac @@ -111,7 +111,7 @@ dnl Availability of various not common threadsafe functions AC_CHECK_FUNCS([strerror_r strtok_r getmntent_r getgrnam_r getpwuid_r]) dnl Availability of various common headers (non-fatal if missing). -AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/syslimits.h sys/utsname.h sys/wait.h winsock2.h sched.h termios.h sys/poll.h syslog.h mntent.h]) +AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/syslimits.h sys/utsname.h sys/wait.h winsock2.h sched.h termios.h sys/poll.h syslog.h mntent.h net/ethernet.h]) dnl Where are the XDR functions? dnl If portablexdr is installed, prefer that. diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 16c1a25..0fe51e4 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -26,9 +26,13 @@ #include <config.h> +#include <sys/types.h> +#include <sys/stat.h> #include <fcntl.h> #include <dirent.h> +#if HAVE_NET_ETHERNET_H #include <net/ethernet.h> +#endif #include "internal.h" @@ -41,6 +45,22 @@ #include "domain_conf.h" +/* XXX + * The config parser/struts should not be using platform specific + * constants. Win32 lacks these constants, breaking the parser, + * so temporarily define them until this can be re-written to use + * locally defined enums for all consants + */ +#ifndef ETHERTYPE_IP +#define ETHERTYPE_IP 0x0800 +#endif +#ifndef ETHERTYPE_ARP +#define ETHERTYPE_ARP 0x0806 +#endif +#ifndef ETHERTYPE_IPV6 +#define ETHERTYPE_IPV6 0x86dd +#endif + #define VIR_FROM_THIS VIR_FROM_NWFILTER diff --git a/src/util/hooks.c b/src/util/hooks.c index 755679d..bcab4eb 100644 --- a/src/util/hooks.c +++ b/src/util/hooks.c @@ -24,7 +24,9 @@ #include <config.h> #include <sys/types.h> +#if HAVE_SYS_WAIT_H #include <sys/wait.h> +#endif #include <sys/stat.h> #include <unistd.h> #include <stdlib.h> @@ -188,6 +190,19 @@ virHookPresent(int driver) { * Returns: 0 if the execution succeeded, 1 if the script was not found or * invalid parameters, and -1 if script returned an error */ +#ifdef WIN32 +int +virHookCall(int driver ATTRIBUTE_UNUSED, + const char *id ATTRIBUTE_UNUSED, + int op ATTRIBUTE_UNUSED, + int sub_op ATTRIBUTE_UNUSED, + const char *extra ATTRIBUTE_UNUSED, + const char *input ATTRIBUTE_UNUSED) { + virReportSystemError(ENOSYS, "%s", + _("spawning hooks not supported on this platform")); + return -1; +} +#else int virHookCall(int driver, const char *id, int op, int sub_op, const char *extra, const char *input) { @@ -447,3 +462,4 @@ no_memory: #undef ADD_ENV_LIT #undef ADD_ENV_SPACE } +#endif diff --git a/src/util/threads-win32.c b/src/util/threads-win32.c index b1d1571..a30bccf 100644 --- a/src/util/threads-win32.c +++ b/src/util/threads-win32.c @@ -69,7 +69,7 @@ void virThreadOnExit(void) int virMutexInit(virMutexPtr m) { - virMutexInitRecursive(m); + return virMutexInitRecursive(m); } int virMutexInitRecursive(virMutexPtr m) diff --git a/tools/virsh.c b/tools/virsh.c index aaae7ca..8017beb 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -414,6 +414,7 @@ out: */ static int disconnected = 0; /* we may have been disconnected */ +#ifdef SIGPIPE /* * vshCatchDisconnect: * @@ -442,6 +443,10 @@ vshSetupSignals(void) { sigaction(SIGPIPE, &sig_action, NULL); } +#else +static void +vshSetupSignals(void) {} +#endif /* * vshReconnect: @@ -8425,7 +8430,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) &creation) < 0) continue; localtime_r(&creation, &time_info); - strftime(timestr, sizeof(timestr), "%F %T %z", &time_info); + strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", &time_info); vshPrint(ctl, " %-20s %-25s %s\n", names[i], timestr, state); } -- 1.6.6.1
On 04/07/2010 11:38 AM, Daniel P. Berrange wrote:
The network filter / snapshot / hooks code introduced some non-portable pices that broke the win32 build
* configure.ac: Check for net/ethernet.h required by nwfile config parsing code * src/conf/nwfilter_conf.c: Define ethernet protocol constants if net/ethernet.h is missing * src/util/hooks.c: Disable hooks build on Win32 since it lacks fork/exec/pipe * src/util/threads-win32.c: Fix unchecked return value * tools/virsh.c: Disable SIGPIPE on Win32 since it doesn't exist.
Gnulib can emulate SIGPIPE on Win32, but that can be a separate patch for later, after 0.8.0 is out the door.
dnl Availability of various common headers (non-fatal if missing). -AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/syslimits.h sys/utsname.h sys/wait.h winsock2.h sched.h termios.h sys/poll.h syslog.h mntent.h]) +AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/syslimits.h sys/utsname.h sys/wait.h winsock2.h sched.h termios.h sys/poll.h syslog.h mntent.h net/ethernet.h])
I'm wondering if it's worth using AC_CHECK_HEADERS_ONCE here, instead. Probably not. Hmm, gnulib guarantees regex.h, sys/utsname.h, sys/wait.h, and sched.h, given the correct modules. We could shrink this line with followup patches to use that part of gnulib. Also, this line is long; AC_CHECK_HEADERS allows newlines in its whitespace-separated list, if you'd like to keep things under 80 columns.
+#if HAVE_NET_ETHERNET_H #include <net/ethernet.h> +#endif
Will fail 'make syntax-check' if you install cppi. s/#include/# include/
+/* XXX + * The config parser/struts should not be using platform specific
s/struts/structs/
+ * constants. Win32 lacks these constants, breaking the parser, + * so temporarily define them until this can be re-written to use + * locally defined enums for all consants
s/consants/constants/
+ */ +#ifndef ETHERTYPE_IP +#define ETHERTYPE_IP 0x0800 +#endif +#ifndef ETHERTYPE_ARP +#define ETHERTYPE_ARP 0x0806 +#endif +#ifndef ETHERTYPE_IPV6 +#define ETHERTYPE_IPV6 0x86dd +#endif
ETHERTYPE_* constants are mandated by RFC, so they are not platform-specific. I'm not sure we need the overhead of wrapping these with a locally-defined enum, so the comment wording may be a bit misleading; oh well. Also needs cppi indentation.
+#ifdef WIN32 +int +virHookCall(int driver ATTRIBUTE_UNUSED, + const char *id ATTRIBUTE_UNUSED, + int op ATTRIBUTE_UNUSED, + int sub_op ATTRIBUTE_UNUSED, + const char *extra ATTRIBUTE_UNUSED, + const char *input ATTRIBUTE_UNUSED) { + virReportSystemError(ENOSYS, "%s", + _("spawning hooks not supported on this platform")); + return -1;
Good enough for now. But gnulib supports posix_spawn ported to mingw (currently LGPLv3, so we'd have to get it relaxed to LPGLv2 first); perhaps if we rewrite hooks to use posix_spawn() instead of fork()/exec(), then we can support hooks on mingw.
@@ -8425,7 +8430,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) &creation) < 0) continue; localtime_r(&creation, &time_info); - strftime(timestr, sizeof(timestr), "%F %T %z", &time_info); + strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", &time_info);
Is this a case where we want localized output? Or is switching to fixed format a good move independently of mingw lacking localization? Gnulib provides strftime (but it is currently LGPLv3, and would need relaxing), if we want to go with localized output. OK, in spite of me sounding so negative, my comments were mostly notes to self on ways that we can improve things in the future, after the release. Your patch, as-is, _does_ fix the build for mingw, and I didn't find any technical flaws. Therefore, I'm perfectly fine giving this: ACK for 0.8.0 after the typo and syntax-check fixes. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Apr 07, 2010 at 12:12:10PM -0600, Eric Blake wrote:
+#ifdef WIN32 +int +virHookCall(int driver ATTRIBUTE_UNUSED, + const char *id ATTRIBUTE_UNUSED, + int op ATTRIBUTE_UNUSED, + int sub_op ATTRIBUTE_UNUSED, + const char *extra ATTRIBUTE_UNUSED, + const char *input ATTRIBUTE_UNUSED) { + virReportSystemError(ENOSYS, "%s", + _("spawning hooks not supported on this platform")); + return -1;
Good enough for now. But gnulib supports posix_spawn ported to mingw (currently LGPLv3, so we'd have to get it relaxed to LPGLv2 first); perhaps if we rewrite hooks to use posix_spawn() instead of fork()/exec(), then we can support hooks on mingw.
posix_spawn() isn't really flexible enough to replace the virExec() functionality
@@ -8425,7 +8430,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) &creation) < 0) continue; localtime_r(&creation, &time_info); - strftime(timestr, sizeof(timestr), "%F %T %z", &time_info); + strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", &time_info);
Is this a case where we want localized output? Or is switching to fixed format a good move independently of mingw lacking localization? Gnulib provides strftime (but it is currently LGPLv3, and would need relaxing), if we want to go with localized output.
%F & %T are not localized formats anyway, so this isn't impacting that. THis is just a straight substitution expanding the shortcuts to the full syntax Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange -
Eric Blake