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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list