[caveat - this is my first time visiting the netcf list. Sorry if I'm
breaking any list protocols :)]
On 09/03/2010, Adam Stokes wrote:
General impressions - thanks for taking on this porting job! It's
interesting that the first contributed non-Fedora port is for Windows,
of all platforms.
diff --git a/bootstrap b/bootstrap
index f0653ef..351d58c 100755
--- a/bootstrap
+++ b/bootstrap
@@ -62,6 +62,25 @@ c-ctype
read-file
safe-alloc
warnings
+sys_wait
+sigaction
+pthread
+unistd
+getdtablesize
+signal
+arpa_inet
+sys_ioctl
+getopt-posix
+string
+sys_socket
+socket
+netinet_in
+vasprintf
+fcntl
+inet_ntop
+inet_pton
+close
+setenv
'
Can we get this list sorted alphabetically?
+++ b/configure.ac
@@ -16,7 +16,27 @@ AC_PROG_LIBTOOL
dnl The backend driver. Right now hardcoded to initscripts, but
dnl eventually needs to be configurable at buildtime
-AM_CONDITIONAL([NETCF_DRIVER_INITSCRIPTS], [true])
+dnl temporary, if win32 then dont build this.
+AC_CANONICAL_HOST
+
+dnl WIN32
+AC_MSG_CHECKING([for win32 platform])
+PLATFORM_WIN32="no"
+case "$host" in
+ *-*-mingw*)
+ PLATFORM_WIN32="yes"
+ NO_UNDEFINED="-Wl,--no-undefined"
+ ;;
+ *-*-cygwin*)
+ NO_UNDEFINED="-Wl,--no-undefined"
Not sure why you need this with a -Wl, prefix, since netcf uses libtool.
Shouldn't including '-no-undefined' in the appriate AM_LDFLAGS of
Makefile.am cover this?
-AC_CHECK_HEADER([pthread.h],
- [AC_CHECK_LIB([pthread],[pthread_join],[
- AC_DEFINE([HAVE_LIBPTHREAD],[],[Define if pthread (-lpthread)])
- AC_DEFINE([HAVE_PTHREAD_H],[],[Define if<pthread.h>])
- LIBS="-lpthread $LIBS"
- ])])
+dnl check pthread functions
+dnl pulled from:
+dnl
http://www.mail-archive.com/libvir-list@redhat.com/msg24187.html
+old_LIBS=$LIBS
+LIBS="$LIBS $LIB_PTHREAD"
+AC_CHECK_FUNCS([pthread_sigmask pthread_mutexattr_init])
+LIBS=$old_libs
This part looks sane to be broken into an independent patch.
+++ b/mingw32-netcf.spec.in
@@ -0,0 +1,95 @@
+%global __strip %{_mingw32_strip}
+%global __objdump %{_mingw32_objdump}
+%global _use_internal_dependency_generator 0
+%global __find_requires %{_mingw32_findrequires}
+%global __find_provides %{_mingw32_findprovides}
To match mingw32-libvirt's example, this needs another line
%define __debug_install_post %{_mingw32_debug_install_post}
+
+%changelog
+* Mon Sep 1 2010 Adam Stokes<ajs(a)redhat.com> - 0.1.6-2
+- Update spec to not require .dll.a or *.la
+- Fixed building of DLL
Technically, you should start mingw32-netcf at 0.1.6-1 (always match the
package version of the current non-mingw rpm, but track the -n of the
mingw builds independently, which means starting at -1).
+
+* Thu Apr 16 2010 Laine Stump<laine(a)redhat.com> - 0.1.6-1
+- New version
No need to include changelog details for builds that did not exist for
mingw32-netcf.
new file mode 100755
index 0000000..d0d1687
--- /dev/null
+++ b/netcf-wininst.nsi
Sorry, I've never worked with .nsi files to be much of a help on this
part of the review.
@@ -29,9 +32,11 @@ libnetcf_la_SOURCES = netcf.h netcf.c internal.h
\
ref.h list.h \
xslt_ext.c $(DRIVER_SOURCES)
libnetcf_la_LDFLAGS = -Wl,--version-script=netcf.syms \
- -version-info $(LIBNETCF_VERSION_INFO)
+ -version-info $(LIBNETCF_VERSION_INFO) @NO_UNDEFINED@
In general, you should use $(NO_UNDEFINED) for AC_SUBST'd data. But
back to my earlier question, why not just blindly use:
-no-undefined
here, as it does not hurt on Linux, and since libtool can handle it a
lot better than manually going through -Wl, linker flags?
datadir.h: $(top_builddir)/config.status
- echo '#define DATADIR "$(datadir)"'> datadir.h
+ echo '#define NETCF_DATADIR "$(datadir)"'> datadir.h
Hmm, this renaming should probably be split into a separate patch.
+++ b/src/ncftool.c
@@ -784,7 +784,10 @@ int main(int argc, char **argv) {
fprintf(stderr, "Failed to initialize netcf\n");
if (ncf != NULL)
print_netcf_error();
+#ifndef WIN32
+ // FIXME: Need to find out why ncf_init is croaking
exit(EXIT_FAILURE);
+#endif
Does this really work for WIN32 but fail otherwise?
}
readline_init();
if (optind< argc) {
diff --git a/src/netcf.c b/src/netcf.c
index 1a95332..96b6be7 100644
--- a/src/netcf.c
+++ b/src/netcf.c
@@ -35,7 +35,11 @@
#include "internal.h"
#include "netcf.h"
+#ifdef WIN32
+#include "netcf_win.h"
Unrelated to your patch, but should netcf adopt the same cppi rules as
libvirt to make it easier to follow nested #ifdefs?
+++ b/src/netcf_win.c
@@ -0,0 +1,565 @@
+#include<config.h>
+#include<internal.h>
+
+#include<stdio.h>
+#include<stdlib.h>
+#include "netcf_win.h"
+
+#define MAX_TRIES 3
+#define GAA_FLAGS ( GAA_FLAG_SKIP_DNS_SERVER | GAA_FLAG_SKIP_MULTICAST )
+#define BUFSIZE 8192
+
+PMIB_IPADDRTABLE _get_ip_addr_table(PMIB_IPADDRTABLE ipAddrTable) {
+ DWORD r = 0;
+ DWORD buf = 0;
+
+ ipAddrTable = (PMIB_IPADDRTABLE) malloc(sizeof (MIB_IPADDRTABLE));
Since this is C, the cast is probably redundant (I'm not a fan of
casting malloc results, except when trying to satisfy C++).
+PIP_ADAPTER_ADDRESSES _get_ip_adapter_info(PIP_ADAPTER_ADDRESSES
addrList) {
+ ULONG bufferLength = 0;
+ DWORD r;
+ int i;
+
+ for(i = 0; i< 5; i++) {
Why 5? And why is this magic number not a #define?
+/* All undefined routines in windows */
+int drv_get_aug(struct netcf *ncf, const char *ncf_xml, char **aug_xml) {
+#ifdef WIN32
+ return -1;
+#endif
+}
Hmm - this will cause compilation warnings or failure if this file is
compiled on any non-WIN32 platform (missing return value). And I think
it would be better to have these stubs next to the declaration of the
working versions in netcf.c (and not here in an independent file), as in:
#ifdef WIN32
/* No way to implement these on mingw for now... */
int drv_get_aug() { return -1; }
#else
int drv_get_aug() {
...
}
#endif
That also has the advantage of moving the #ifdefs outside of function
bodies, which aids in readability.
+
+int xasprintf(char **strp, const char *format, ...) {
+ va_list args;
+ int result;
+
+ va_start (args, format);
+ result = vasprintf (strp, format, args);
+ va_end (args);
+ if (result< 0)
+ *strp = NULL;
+ return result;
+}
Why are you reinventing a printf interface in a win-specific file?
Shouldn't this be in a file used by the entire project, if it is necessary?
+static int list_interface_ids(struct netcf *ncf ATTRIBUTE_UNUSED,
+ int maxnames ATTRIBUTE_UNUSED,
+ char **names, unsigned int flags ATTRIBUTE_UNUSED,
+ const char *id_attr ATTRIBUTE_UNUSED) {
+ unsigned int nint = 0;
+
+ PIP_ADAPTER_ADDRESSES addrList = NULL;
+ PIP_ADAPTER_ADDRESSES adapterp = NULL;
+ adapterp = _get_ip_adapter_info(addrList);
+ for (nint = 0; adapterp != NULL; nint++) {
+ if (names) {
+ char name[8192];
Shouldn't this use BUFSIZE, which you defined earlier? Also,
stack-allocating anything larger than 4k is dangerous, since it can lead
to silent program termination on stack overflow conditions where you end
up overshooting the 4k guard page.
+const char *drv_mac_string(struct netcf_if *nif) {
+ // struct netcf *ncf = nif->ncf;
+ char mac[256];
Magic number? And given the name, it sounds like you are over-allocating.
+ sprintf(mac, "%02X:%02X:%02X:%02X:%02X:%02X",
+ adapterp->PhysicalAddress[0],
+ adapterp->PhysicalAddress[1],
+ adapterp->PhysicalAddress[2],
+ adapterp->PhysicalAddress[3],
+ adapterp->PhysicalAddress[4],
+ adapterp->PhysicalAddress[5]);
+ nif->mac = strdup(mac);
Rather than risking a code audit question of whether this can overflow,
why not combine sprintf/strdup into a single asprintf from the get-go?
At which point, you don't even need the temporary mac[256].
+int drv_if_ipaddresses(struct netcf_if *nif, char *ipBuf) {
+ PIP_ADAPTER_ADDRESSES addrList = NULL;
+ PIP_ADAPTER_ADDRESSES adapterp = NULL;
+ PMIB_IPADDRTABLE ipAddrTable = NULL;
+ PMIB_IPADDRTABLE ipAddrTableDup = NULL;
+ IN_ADDR ipAddr;
+ DWORD r = 0;
+ int i;
+
+ if ((ipAddrTableDup = _get_ip_addr_table(ipAddrTable)) == NULL)
+ return -1;
+
+ adapterp = _get_ip_adapter_info(addrList);
+ if (adapterp != NULL) {
+ while(adapterp) {
+ if (adapterp->OperStatus != 1)
+ continue;
This is a 100%-processor busy loop. Should you be sleeping to at least
allow some other thread to make progress?
+++ b/src/netcf_win.h
@@ -0,0 +1,93 @@
+/*
+ * netcf-win.h: windows functions
+ *
+ * Copyright (C) 2009-2010 Red Hat Inc.
Is 2009 appropriate, if this is a new file this year?
+
+static int aug_put_xml(struct netcf *ncf, xmlDocPtr xml);
Why are you declaring static functions in a public header? Just declare
them in the .c file where they are used if they are truly static.
+struct netcf_if *make_netcf_if(struct netcf *ncf, char *name);
+int xasprintf(char **strp, const char *format, ...);
Leaving only the real exports, like these, in the header. Again, why is
xasprintf windows-specific? If it is useful to netcf in general, it
should be moved to a better file, and made namespace clean.
I have not tried a compile test, as there were enough surface issues to
be worth a patch respin first.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library
http://libvirt.org