[PATCH] - review of win32 netcf port

[Adam Stokes]

I know this is the matahari list but I was asked to post patch here for technical review.

Any suggestions highly appreciated

Thanks,

-- 
        __          __                
.-----.|  |_.-----.|  |--.-----.-----.
|__ --||   _|  _  ||    <|  -__|__ --|
|_____||____|_____||__|__|_____|_____|
 .----------------------------------.
(   ajs(a)redhat.com || 919.754.4187   )
 `----------------------------------'

[Andrew Beekhof]

On Fri, Aug 27, 2010 at 8:02 PM,  <ajs(a)redhat.com&gt; wrote:
>  I know this is the matahari list but I was asked to post patch here
> for technical review. 
Comments in-line.
Not your fault, but the Windows code made my eye's bleed.
All those caps!

>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/bootstrap
> netcf-bm-clean/bootstrap
>  --- netcf/bootstrap	2010-08-27 11:13:14.255330584 -0400
>  +++ netcf-bm-clean/bootstrap	2010-08-27 11:20:55.172330580 -0400
>  @@ -62,6 +62,24 @@
>   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
>   '
>
>   # Tell gnulib to:
>  @@ -70,7 +88,6 @@
>   #   put *.[ch] files in new gnulib/lib/ dir.
>
>   $gnulib_tool			\
>  -  --lgpl=2			\ 
This is something you'll need to call out and explain exactly which
modules cause this and what the status of getting them dual-licenced
is.

>     --with-tests			\
>     --m4-base=gnulib/m4		\
>     --source-base=gnulib/lib	\
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/build/aux/move-if-change
> netcf-bm-clean/build/aux/move-if-change
>  --- netcf/build/aux/move-if-change	2010-08-27 11:13:14.256328052 -0400
>  +++ netcf-bm-clean/build/aux/move-if-change	1969-12-31 19:00:00.000000000 -0500 
Not convinced we should be removing this file.
Probably just remove this part of the patch.

>  @@ -1,77 +0,0 @@
>  -#!/bin/sh
>  -# Like mv $1 $2, but if the files are the same, just delete $1.
>  -# Status is zero if successful, nonzero otherwise.
>  -
>  -VERSION='2007-09-28 23:10'; # UTC
>  -# The definition above must lie within the first 8 lines in order
>  -# for the Emacs time-stamp write hook (at end) to update it.
>  -# If you change this file with Emacs, please let the write hook
>  -# do its job.  Otherwise, update this string manually.
>  -
>  -# Copyright (C) 2002-2007 Free Software Foundation, Inc.
>  -
>  -# This program is free software: you can redistribute it and/or modify
>  -# it under the terms of the GNU General Public License as published by
>  -# the Free Software Foundation, either version 3 of the License, or
>  -# (at your option) any later version.
>  -
>  -# This program 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 General Public License for more details.
>  -
>  -# You should have received a copy of the GNU General Public License
>  -# along with this program.  If not, see <http://www.gnu.org/licenses/>;.
>  -
>  -usage="usage: $0 SOURCE DEST"
>  -
>  -help="$usage
>  -  or:  $0 OPTION
>  -If SOURCE is different than DEST, then move it to DEST; else remove SOURCE.
>  -
>  -  --help     display this help and exit
>  -  --version  output version information and exit
>  -
>  -Report bugs to <bug-gnulib(a)gnu.org&gt;.&quot;
>  -
>  -version=`expr "$VERSION" : '\([^ ]*\)'`
>  -version="move-if-change (gnulib) $version
>  -Copyright (C) 2007 Free Software Foundation, Inc.
>  -License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>  -This is free software: you are free to change and redistribute it.
>  -There is NO WARRANTY, to the extent permitted by law."
>  -
>  -
>  -for arg
>  -do
>  -  case $arg in
>  -    --help | --hel | --he | --h)
>  -      exec echo "$help" ;;
>  -    --version | --versio | --versi | --vers | --ver | --ve | --v)
>  -      exec echo "$version" ;;
>  -    --)
>  -      shift
>  -      break ;;
>  -    -*)
>  -      echo "$0: invalid option: $arg" >&2
>  -      exit 1 ;;
>  -    *)
>  -      break ;;
>  -  esac
>  -done
>  -
>  -test $# = 2 || { echo "$0: $usage" >&2; exit 1; }
>  -
>  -if test -r "$2" && cmp -s -- "$1" "$2"; then
>  -  rm -f -- "$1"
>  -else
>  -  mv -f -- "$1" "$2"
>  -fi
>  -
>  -## Local Variables:
>  -## eval: (add-hook 'write-file-hooks 'time-stamp)
>  -## time-stamp-start: "VERSION='"
>  -## time-stamp-format: "%:y-%02m-%02d %02H:%02M"
>  -## time-stamp-time-zone: "UTC"
>  -## time-stamp-end: "'; # UTC"
>  -## End:
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/configure.ac
> netcf-bm-clean/configure.ac
>  --- netcf/configure.ac	2010-08-27 11:13:14.256328052 -0400
>  +++ netcf-bm-clean/configure.ac	2010-08-27 11:20:55.173330423 -0400
>  @@ -1,7 +1,7 @@
>  -AC_INIT(netcf, 0.1.6)
>  +AC_INIT([netcf], [0.1.6]) 
Avoid including style issues, thats not the focus of the patch and we
don't want to include any tangent opportunities.
If you feel inclined, submit it as a separate patch.

>   AC_CONFIG_SRCDIR([src/netcf.c])
>   AC_CONFIG_AUX_DIR([build/aux])
>  -AM_CONFIG_HEADER([config.h])
>  +AC_CONFIG_HEADER([config.h]) 
Same here.  If it actually breaks something, include as a separate patch.

>   AM_INIT_AUTOMAKE([-Wno-portability 1.11 color-tests
> parallel-tests])
>   AM_SILENT_RULES([yes]) # make --enable-silent-rules the default.
>
>  @@ -16,7 +16,24 @@
>
>   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"
>  +	;;
>  +	*)
>  +		MINGW_EXTRA_LDFLAGS="-no-undefined" 
All platforms get MINGW_EXTRA_LDFLAGS?

>  +	;;
>  +esac
>  +AC_MSG_RESULT([$PLATFORM_WIN32])
>  +AM_CONDITIONAL(PLATFORM_WIN32, test "${PLATFORM_WIN32}" = "yes")
>  +AC_SUBST([PLATFORM_WIN32])
>  +AC_SUBST(MINGW_EXTRA_LDFLAGS)
>
>   dnl Compiler flags to be used everywhere
>   AC_SUBST([NETCF_CFLAGS], ['--std=gnu99 -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
> -fasynchronous-unwind-tables'])
>  @@ -27,23 +44,26 @@
>
>   dnl Need to test if pkg-config exists
>   PKG_PROG_PKG_CONFIG
>  -PKG_CHECK_MODULES([LIBAUGEAS], [augeas >= 0.5.0])
>   PKG_CHECK_MODULES([LIBXML], [libxml-2.0])
>   PKG_CHECK_MODULES([LIBXSLT], [libxslt])
>   PKG_CHECK_MODULES([LIBEXSLT], [libexslt])
>  -PKG_CHECK_MODULES([LIBNL], [libnl-1])
>  +if test "${PLATFORM_WIN32}" = "no"; then
>  +	PKG_CHECK_MODULES([LIBAUGEAS], [augeas >= 0.5.0])
>  +	PKG_CHECK_MODULES([LIBNL], [libnl-1])
>  +fi 
I would do the checks all the time so that you can use #ifdef
LIBAUGEAS rather than #ifndef WIN32.
See further down.

>   NETCF_CHECK_READLINE
>
>   NETCF_LIBDEPS=$(echo $LIBAUGEAS_LIBS $LIBEXSLT_LIBS $LIBXSLT_LIBS $LIBXML_LIBS
> $LIBNL_LIBS)
>   AC_SUBST([NETCF_LIBDEPS])
>
>  -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/[e-mail]/msg24187.html
>  +old_LIBS=$LIBS
>  +LIBS="$LIBS $LIB_PTHREAD"
>  +AC_CHECK_FUNCS([pthread_sigmask pthread_mutexattr_init])
>  +LIBS=$old_libs 
Lets see how they react to this one.

>   AC_OUTPUT(Makefile                                          \
>             gnulib/lib/Makefile                               \
>  @@ -51,4 +71,4 @@
>             src/Makefile                                      \
>             tests/Makefile                                    \
>             doc/Makefile                                      \
>  -          netcf.pc netcf.spec)
>  +          netcf.pc netcf.spec mingw32-netcf.spec)
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/Makefile.am
> netcf-bm-clean/Makefile.am
>  --- netcf/Makefile.am	2010-08-27 11:13:14.255330584 -0400
>  +++ netcf-bm-clean/Makefile.am	2010-08-27 11:20:55.170330406 -0400
>  @@ -7,6 +7,7 @@
>   pkgconfigdir = $(libdir)/pkgconfig
>   pkgconfig_DATA = netcf.pc
>
>  +if PLATFORM_WIN32_FALSE 
Here's an example of something that should depend on Augeas being
(not-)present rather than the platform.
As a general rule, its preferrable to do things conditionally based on
features rather than platforms.

>   netcfdir=$(datadir)/netcf
>   xmldir=$(netcfdir)/xml
>   lensdir=$(netcfdir)/lenses
>  @@ -16,6 +17,7 @@
>   	data/xml/initscripts-get.xsl data/xml/initscripts-put.xsl
>   dist_netcf_DATA=data/iptables-forward-bridged
>   dist_lens_DATA=data/lenses/netcf.aug data/lenses/sysconfig.aug
>  +endif
>
>   # This requires that trang is installed, but we don't want to require
>   # that, even for building, since the .rnc files are only a convenience
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/mingw32-netcf.spec.in
> netcf-bm-clean/mingw32-netcf.spec.in
>  --- netcf/mingw32-netcf.spec.in	1969-12-31 19:00:00.000000000 -0500
>  +++ netcf-bm-clean/mingw32-netcf.spec.in	2010-08-27 11:20:55.176329742 -0400
>  @@ -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}
>  +
>  +Name:           mingw32-netcf
>  +Version:        @VERSION@
>  +Release:        1%{?dist}%{?extra_release}
>  +Summary:        Cross-platform network configuration library
>  +
>  +Group:          System Environment/Libraries
>  +License:        LGPLv2+
>  +URL:            https://fedorahosted.org/netcf/
>  +Source0:        https://fedorahosted.org/released/%{name}/%{name}-%{version}.tar.gz
>  +BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>  +
>  +BuildRequires:  mingw32-readline
>  +BuildRequires:  mingw32-binutils
>  +BuildRequires:  mingw32-libxml2
>  +BuildRequires:  mingw32-libxslt
>  +BuildRequires:  mingw32-gcc
>  +BuildRequires:  mingw32-gettext
>  +BuildRequires: 	mingw32-w32api
>  +Requires:       pkgconfig
>  +
>  +%description
>  +A library for modifying the network configuration of a system. Network
>  +configurations are expresed in a platform-independent XML format, which
>  +netcf translates into changes to the system's 'native' network
>  +configuration files.
>  +
>  +MinGW cross-compiled Windows library.
>  +
>  +%prep
>  +%setup -q
>  +
>  +%build
>  +PATH=%{_mingw32_bindir}:$PATH \
>  +%{_mingw32_configure} --disable-static
>  +make %{?_smp_mflags}
>  +
>  +%install
>  +rm -rf $RPM_BUILD_ROOT
>  +make install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -p"
>  +find $RPM_BUILD_ROOT -name '*.la' -exec rm -f {} ';'
>  +find $RPM_BUILD_ROOT -name '*dll.a' -exec rm -f {} ';'
>  +
>  +%clean
>  +rm -rf $RPM_BUILD_ROOT
>  +
>  +%files
>  +%defattr(-,root,root,-)
>  +%{_mingw32_bindir}/ncftool.exe
>  +%{_mingw32_bindir}/libnetcf-1.dll
>  +%{_mingw32_includedir}/netcf.h
>  +%{_mingw32_libdir}/pkgconfig/netcf.pc
>  +
>  +%doc AUTHORS COPYING NEWS
>  +
>  +%changelog
>  +* Mon Aug 02 2010 Adam Stokes <ajs(a)redhat.com&gt; - 0.1.8-1
>  +- Update spec to not require .dll.a or *.la
>  +- Fixed building of DLL
>  +
>  +* Wed Jul 28 2010 Adam Stokes <ajs(a)redhat.com&gt; - 0.1.7-1
>  +- cross compile 
I don't think you need to include the pre-mingw history in the mingw spec file.

>  +
>  +* Thu Apr 16 2010 Laine Stump <laine(a)redhat.com&gt; - 0.1.6-1
>  +- New version
>  +
>  +* Mon Nov 30 2009 David Lutterkort <lutter(a)redhat.com&gt; - 0.1.5-1
>  +- New version
>  +
>  +* Thu Nov  5 2009 David Lutterkort <lutter(a)redhat.com&gt; - 0.1.4-1
>  +- New version
>  +
>  +* Tue Oct 27 2009 David Lutterkort <lutter(a)redhat.com&gt; - 0.1.3-1
>  +- New version
>  +
>  +* Fri Sep 25 2009 David Lutterkort <lutter(a)redhat.com&gt; - 0.1.2-1
>  +- New Version
>  +
>  +* Wed Sep 16 2009 David Lutterkort <lutter(a)redhat.com&gt; - 0.1.1-1
>  +- New Version
>  +
>  +* Mon Jul 13 2009 David Lutterkort <lutter(a)redhat.com&gt; - 0.1.0-1
>  +- BR on augeas-0.5.2
>  +- Drop explicit requires for augeas-libs
>  +
>  +* Wed Apr 15 2009 David Lutterkort <lutter(a)redhat.com&gt; - 0.0.2-1
>  +- Updates acording to Fedora review
>  +
>  +* Fri Feb 27 2009 David Lutterkort <lutter(a)redhat.com&gt; - 0.0.1-1
>  +- Initial specfile
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/drv_initscripts.c
> netcf-bm-clean/src/drv_initscripts.c
>  --- netcf/src/drv_initscripts.c	2010-08-27 11:13:14.258328785 -0400
>  +++ netcf-bm-clean/src/drv_initscripts.c	2010-08-27 11:20:55.177328537 -0400
>  @@ -23,7 +23,10 @@
>   #include <config.h>
>   #include <internal.h>
>
>  +#ifdef HAVE_LIBAUGEAS
>   #include <augeas.h>
>  +#endif
>  +
>   #include <stdlib.h>
>   #include <stdarg.h>
>   #include <stdio.h>
>  @@ -35,7 +38,9 @@
>   #include "ref.h"
>   #include "list.h"
>   #include "dutil.h"
>  +#ifndef WIN32
>   #include "dutil_linux.h"
>  +#endif
>
>   #include <libxml/parser.h>
>   #include <libxml/relaxng.h>
>  @@ -100,7 +105,7 @@
>
>   static const char *const prog_lokkit = "/usr/sbin/lokkit";
>   static const char *const lokkit_custom_rules =
>  -    "--custom-rules=ipv4:filter:" DATADIR
> "/netcf/iptables-forward-bridged";
>  +    "--custom-rules=ipv4:filter:" NETCF_DATADIR
> "/netcf/iptables-forward-bridged"; 
Why the #define name change?
Again, not crucial to the patch so leave it out.

>   static const char *const prog_rc_d_iptables = "/etc/init.d/iptables";
>
>  @@ -163,7 +168,9 @@
>       int match = -1;
>       for (int i=0; i < nhwaddr; i++) {
>           const char *addr;
>  +#ifdef HAVE_LIBAUGEAS
>           r = aug_get(aug, hwaddr[i], &addr);
>  +#endif
>           ERR_COND_BAIL(r != 1, ncf, EOTHER);
>           if (STRCASEEQ(addr, mac))
>               match = i;
>  @@ -184,16 +191,20 @@
>    * interface by checking for an entry 'DEVICE=NAME'
>    */
>   static char *find_ifcfg_path_by_device(struct netcf *ncf, const char *name) {
>  +#ifdef HAVE_LIBAUGEAS
>       struct augeas *aug = NULL;
>  +#endif
>       int ndevs = 0;
>       char **devs = NULL;
>
>  +#ifdef HAVE_LIBAUGEAS
>       aug = get_augeas(ncf);
>       ERR_BAIL(ncf);
>
>       ndevs = aug_fmt_match(ncf, &devs, "%s[DEVICE = '%s']",
>                             ifcfg_path, name);
>       ERR_COND_BAIL(ndevs < 0, ncf, EOTHER);
>  +#endif
>
>       if (ndevs == 0)
>           return NULL;
>  @@ -211,6 +222,7 @@
>       return NULL;
>   }
>
>  +#ifndef WIN32
>   /* Find the path to the ifcfg file that has the configuration for
>    * the device NAME. The logic follows the need_config function
>    * in /etc/sysconfig/network-scripts/network-functions
>  @@ -327,9 +339,11 @@
>       free_matches(ndevs, &devs);
>       return -1;
>   }
>  +#endif
>
>   static int list_interfaces(struct netcf *ncf, char ***intf) {
>       int nint = 0, result = 0;
>  +#ifdef HAVE_LIBAUGEAS
>       struct augeas *aug = NULL;
>
>       aug = get_augeas(ncf);
>  @@ -338,6 +352,7 @@
>       /* Look in augeas for all interfaces */
>       nint = list_ifcfg_paths(ncf, intf);
>       ERR_BAIL(ncf);
>  +#elif WIN32
>       result = nint;
>
>       /* Filter out the interfaces that are slaves/subordinate */
>  @@ -351,12 +366,14 @@
>               i += 1;
>           }
>       }
>  +#endif
>       return result;
>    error:
>       free_matches(nint, intf);
>       return -1;
>   }
>
>  +#ifndef WIN32
>   /* Ensure we have an iptables rule to bridge physdevs. We take care of both
>    * systems using iptables directly, and systems using lokkit (even if it's
>    * only installed, but not used)
>  @@ -477,6 +494,7 @@
>       free(p);
>       return;
>   }
>  +#endif /* WIN32 */
>
>   int drv_init(struct netcf *ncf) {
>       int r;
>  @@ -486,10 +504,13 @@
>
>       ncf->driver->ioctl_fd = -1;
>
>  +#ifdef HAVE_LIBAUGEAS
>       r = add_augeas_xfm_table(ncf, &augeas_xfm_common);
>       if (r < 0)
>           goto error;
>  +#endif /* LIBAUGEAS */
>
>  +#ifndef WIN32
>       // FIXME: Check for errors
>       xsltInit();
>       exsltStrRegister();
>  @@ -500,13 +521,16 @@
>
>       bridge_physdevs(ncf);
>       ERR_BAIL(ncf);
>  -
>  +#endif /* WIN32 */
>       /* open a socket for interface ioctls */
>       ncf->driver->ioctl_fd = init_ioctl_fd(ncf);
>       if (ncf->driver->ioctl_fd < 0)
>           goto error;
>  +
>  +#ifdef HAVE_LIBNL
>       if (netlink_init(ncf) < 0)
>           goto error;
>  +#endif /* LIBNL */
>       return 0;
>
>    error:
>  @@ -520,11 +544,15 @@
>       xsltFreeStylesheet(ncf->driver->get);
>       xsltFreeStylesheet(ncf->driver->put);
>       xmlRelaxNGFree(ncf->driver->rng);
>  +#ifdef HAVE_LIBNL
>       netlink_close(ncf);
>  +#endif /* LIBNL */
>       if (ncf->driver->ioctl_fd >= 0)
>           close(ncf->driver->ioctl_fd);
>  +#ifdef HAVE_LIBAUGEAS
>       aug_close(ncf->driver->augeas);
>       FREE(ncf->driver->augeas_xfm_tables);
>  +#endif /* LIBAUGEAS */
>       FREE(ncf->driver);
>   }
>
>  @@ -536,12 +564,14 @@
>                                 int maxnames, char **names,
>                                 unsigned int flags,
>                                 const char *id_attr) {
>  -    struct augeas *aug = NULL;
>       int nint = 0, nmatches = 0, nqualified = 0, result = 0, r;
>       char **intf = NULL, **matches = NULL;
>  +#ifdef HAVE_LIBAUGEAS
>  +    struct augeas *aug = NULL;
>
>       aug = get_augeas(ncf);
>       ERR_BAIL(ncf);
>  +#endif
>       nint = list_interfaces(ncf, &intf);
>       ERR_BAIL(ncf);
>       if (!names) {
>  @@ -556,9 +586,10 @@
>               int is_qualified = ((flags & (NETCF_IFACE_ACTIVE|NETCF_IFACE_INACTIVE))
>                                   == (NETCF_IFACE_ACTIVE|NETCF_IFACE_INACTIVE));
>
>  +#ifdef HAVE_LIBAUGEAS
>               r = aug_get(aug, matches[nmatches-1], &name);
>               ERR_COND_BAIL(r < 0, ncf, EOTHER);
>  -
>  +#endif /* LIBAUGEAS */
>               if (!is_qualified) {
>                   int is_active = if_is_active(ncf, name);
>                   if ((is_active && (flags & NETCF_IFACE_ACTIVE))
>  @@ -599,6 +630,7 @@
>       struct netcf_if *nif = NULL;
>       char *pathx = NULL;
>       char *name_dup = NULL;
>  +#ifdef HAVE_LIBAUGEAS
>       struct augeas *aug;
>
>       aug = get_augeas(ncf);
>  @@ -606,6 +638,7 @@
>
>       pathx = find_ifcfg_path(ncf, name);
>       ERR_BAIL(ncf);
>  +#endif /* LIBAUGEAS */
>
>       if (pathx == NULL || is_slave(ncf, pathx))
>           goto done;
>  @@ -625,6 +658,7 @@
>       return nif;
>   }
>
>  +#ifndef WIN32
>   /* Get an XML desription of the interfaces (just paths, really) in INTF.
>    * The format is a very simple representation of the Augeas tree (see
>    * xml/augeas.rng)
>  @@ -804,6 +838,7 @@
>       result = 0;
>       goto done;
>   }
>  +#endif /* WIN32 */
>
>   /* Report various status info about the interface as bits in
>    * "flags". Returns 0 on success, -1 on failure
>  @@ -875,6 +910,7 @@
>       return nmatches > 0;
>   }
>
>  +#ifndef WIN32
>   /* The device NAME is a bridge if it has an entry TYPE=Bridge */
>   static bool is_bridge(struct netcf *ncf, const char *name) {
>       int nmatches = 0;
>  @@ -910,13 +946,14 @@
>       free_matches(nslaves, slaves);
>       return -1;
>   }
>  -
>  +#endif /* WIN32 */
>
>   /* For an interface NAME, remove the ifcfg-* files for that interface and
>    * all its slaves. */
>   static void rm_interface(struct netcf *ncf, const char *name) {
>       int r;
>       char *path = NULL;
>  +#ifdef HAVE_LIBAUGEAS
>       struct augeas *aug = NULL;
>
>       aug = get_augeas(ncf);
>  @@ -932,6 +969,9 @@
>
>       r = aug_rm(aug, path);
>       ERR_COND_BAIL(r < 0, ncf, EOTHER);
>  +#elif WIN32
>  +    goto error;
>  +#endif
>    error:
>       FREE(path);
>   }
>  @@ -968,6 +1008,7 @@
>       xmlXPathFreeContext(context);
>   }
>
>  +#ifndef WIN32
>   /* Dig through interface NAME and all its subinterfaces for bonds
>    * and either add aliases in modprobe.conf for it (ALIAS == true), or
>    * remove such aliases (ALIAS == false)
>  @@ -1002,12 +1043,14 @@
>       free_matches(nslaves, &slaves);
>       return;
>   }
>  +#endif
>
>   struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str) {
>       xmlDocPtr ncf_xml = NULL, aug_xml = NULL;
>       char *name = NULL;
>       struct netcf_if *result = NULL;
>       int r;
>  +#ifdef HAVE_LIBAUGEAS
>       struct augeas *aug = get_augeas(ncf);
>
>       ncf_xml = parse_xml(ncf, xml_str);
>  @@ -1037,6 +1080,7 @@
>           aug_print(aug, stderr, "/augeas//error");
>       }
>       ERR_THROW(r < 0, ncf, EOTHER, "aug_save failed");
>  +#endif /* LIBAUGEAS */
>
>       result = make_netcf_if(ncf, name);
>       ERR_BAIL(ncf);
>  @@ -1051,22 +1095,24 @@
>   }
>
>   int drv_undefine(struct netcf_if *nif) {
>  -    struct augeas *aug = NULL;
>       struct netcf *ncf = nif->ncf;
>  +#ifdef HAVE_LIBAUGEAS
>       int r;
>  +    struct augeas *aug = NULL;
>
>       aug = get_augeas(ncf);
>       ERR_BAIL(ncf);
>
>       bond_setup(ncf, nif->name, false);
>       ERR_BAIL(ncf);
>  -
>  +#endif
>       rm_interface(ncf, nif->name);
>       ERR_BAIL(ncf);
>
>  +#ifdef HAVE_LIBAUGEAS
>       r = aug_save(aug);
>       ERR_COND_BAIL(r < 0, ncf, EOTHER);
>  -
>  +#endif
>       return 0;
>    error:
>       return -1;
>  @@ -1075,13 +1121,14 @@
>   int drv_lookup_by_mac_string(struct netcf *ncf, const char *mac,
>                                int maxifaces, struct netcf_if **ifaces)
>   {
>  -    struct augeas *aug = NULL;
>       char *path = NULL, *ifcfg = NULL;
>       const char **names = NULL;
>       int nmatches = 0;
>       char **matches = NULL;
>       int r;
>       int result = -1;
>  +#ifdef HAVE_LIBAUGEAS
>  +    struct augeas *aug = NULL;
>
>       MEMZERO(ifaces, maxifaces);
>
>  @@ -1116,6 +1163,9 @@
>           ERR_BAIL(ncf);
>       }
>       result = cnt;
>  +#elif WIN32
>  +    goto done;
>  +#endif
>       goto done;
>
>    error:
>  @@ -1135,8 +1185,10 @@
>       char *path = NULL;
>       int r;
>
>  +#ifdef HAVE_LIBAUGEAS
>       r = aug_get_mac(ncf, nif->name, &mac);
>       ERR_THROW(r < 0, ncf, EOTHER, "could not lookup MAC of %s",
> nif->name);
>  +#endif
>
>       if (mac != NULL) {
>           if (nif->mac == NULL || STRNEQ(nif->mac, mac)) {
>  @@ -1164,6 +1216,7 @@
>       int nslaves = 0;
>       int result = -1;
>
>  +#ifdef HAVE_LIBAUGEAS
>       if (is_bridge(ncf, nif->name)) {
>           /* Bring up bridge slaves before the bridge */
>           nslaves = bridge_slaves(ncf, nif->name, &slaves);
>  @@ -1176,6 +1229,9 @@
>       }
>       run1(ncf, ifup, nif->name);
>       ERR_BAIL(ncf);
>  +#elif WIN32
>  +    goto error;
>  +#endif
>       result = 0;
>    error:
>       free_matches(nslaves, &slaves);
>  @@ -1189,6 +1245,7 @@
>       int nslaves = 0;
>       int result = -1;
>
>  +#ifdef HAVE_LIBAUGEAS
>       run1(ncf, ifdown, nif->name);
>       ERR_BAIL(ncf);
>       if (is_bridge(ncf, nif->name)) {
>  @@ -1201,12 +1258,16 @@
>               ERR_BAIL(ncf);
>           }
>       }
>  +#elif WIN32
>  +    goto error;
>  +#endif
>       result = 0;
>    error:
>       free_matches(nslaves, &slaves);
>       return result;
>   }
>
>  +#ifdef HAVE_LIBAUGEAS
>   /*
>    * Test interface
>    */
>  @@ -1220,6 +1281,7 @@
>       /* Use utility implementation */
>       return dutil_put_aug(ncf, aug_xml, ncf_xml);
>   }
>  +#endif
>
>   /*
>    * Local variables:
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/dutil.c
> netcf-bm-clean/src/dutil.c
>  --- netcf/src/dutil.c	2010-08-27 11:13:14.259328697 -0400
>  +++ netcf-bm-clean/src/dutil.c	2010-08-27 11:20:55.177328537 -0400
>  @@ -23,7 +23,12 @@
>   #include <config.h>
>   #include <internal.h>
>
>  +#ifndef WIN32
>   #include <augeas.h> 
The above include should be conditional on HAVE_LIBAUGEAS.

>  +#include <net/if.h>
>  +#include <netinet/in.h>
>  +#endif
>  +
>   #include <stdlib.h>
>   #include <stdarg.h>
>   #include <stdio.h>
>  @@ -37,10 +42,7 @@
>   #include <sys/ioctl.h>
>   #include <sys/types.h>
>   #include <sys/stat.h>
>  -#include <net/if.h>
>  -#include <netinet/in.h>
>  -#include <arpa/inet.h> 
Did arpa/inet.h get lost in the ordering shuffle?
Or replaced by sys/socket.h?

>  -
>  +#include <sys/socket.h>
>
>   #include "safe-alloc.h"
>   #include "ref.h"
>  @@ -48,12 +50,14 @@
>   #include "netcf.h"
>   #include "dutil.h"
>
>  +#ifdef HAVE_LIBNL
>   #include <netlink/socket.h>
>   #include <netlink/cache.h>
>   #include <netlink/route/addr.h>
>   #include <netlink/route/link.h>
>   /* For some reason, the headers for libnl vlan functions aren't installed */
>   extern int rtnl_link_vlan_get_id(struct rtnl_link *link);
>  +#endif
>
>   #include <dirent.h>
>
>  @@ -78,6 +82,7 @@
>     return result;
>   }
>
>  +#ifdef HAVE_LIBAUGEAS
>   int add_augeas_xfm_table(struct netcf *ncf,
>                            const struct augeas_xfm_table *xfm) {
>       int slot, r;
>  @@ -122,6 +127,7 @@
>       return 0;
>   }
>
>  +
>   /* Get the Augeas instance; if we already initialized it, just return
>    * it. Otherwise, create a new one and return that.
>    */
>  @@ -246,6 +252,7 @@
>       free(path);
>       return -1;
>   }
>  +#endif /* LIBAUGEAS */
>
>   void free_matches(int nint, char ***intf) {
>       if (*intf != NULL) {
>  @@ -256,7 +263,7 @@
>   }
>
>   xsltStylesheetPtr parse_stylesheet(struct netcf *ncf,
>  -                                          const char *fname) {
>  +                                   const char *fname) { 
Avoid whitespace-only changes such as this one.
It probably aligns just how they like it with their editor configuration :-)

>       xsltStylesheetPtr result = NULL;
>       char *path = NULL;
>       int r;
>  @@ -457,6 +464,7 @@
>       return ret;
>   }
>
>  +
>   /* Find existing node of given name within parent, or create and link
>    * in a new one if not found.
>    */
>  @@ -485,11 +493,14 @@
>       ioctl_fd = socket(AF_INET, SOCK_STREAM, 0);
>       ERR_THROW(ioctl_fd < 0, ncf, EINTERNAL, "failed to open socket for interface
> ioctl");
>
>  +#ifndef WIN32
>  +    /* None of the needed file descriptor routines available in gnulib */
>       flags = fcntl(ioctl_fd, F_GETFD);
>       ERR_THROW(flags < 0, ncf, EINTERNAL, "failed to get flags for ioctl
> socket");
>
>       flags = fcntl(ioctl_fd, F_SETFD, flags | FD_CLOEXEC);
>       ERR_THROW(flags < 0, ncf, EINTERNAL, "failed to set FD_CLOEXEC flag on ioctl
> socket");
>  +#endif /* WIN32 */
>       return ioctl_fd;
>
>   error:
>  @@ -498,6 +509,7 @@
>       return -1;
>   }
>
>  +#ifdef HAVE_LIBNL
>   int netlink_init(struct netcf *ncf) {
>
>       ncf->driver->nl_sock = nl_handle_alloc();
>  @@ -546,7 +558,13 @@
>       }
>       return 0;
>   }
>  +#endif /* LIBNL */
>
>  +#ifdef WIN32
>  +int if_is_active(struct netcf *ncf, const char *intf) {
>  +    return 0;
>  +}
>  +#else
>   int if_is_active(struct netcf *ncf, const char *intf) { 
Try to avoid making the entire function conditional and just do:

>  +#ifdef WIN32
>  +    return 0;
>  +#endif 
at the top of the existing definition.

>       struct ifreq ifr;
>
>  @@ -558,6 +576,7 @@
>       }
>       return ((ifr.ifr_flags & IFF_UP) == IFF_UP);
>   }
>  +#endif /* WIN32 */
>
>   netcf_if_type_t if_type(struct netcf *ncf, const char *intf) {
>       char *path;
>  @@ -675,6 +694,7 @@
>       return result;
>   }
>
>  +#ifdef HAVE_LIBAUGEAS
>   /*
>    * Test interface
>    */
>  @@ -717,7 +737,7 @@
>       xmlFreeDoc(aug_doc);
>       return result;
>   }
>  -
>  +#endif /* LIBNL */
>
>   static void add_type_specific_info(struct netcf *ncf,
>                                      const char *ifname, int ifindex,
>  @@ -734,6 +754,11 @@
>       struct netcf *ncf;
>   };
>
>  +#ifdef WIN32
>  +static void add_ip_info_cb(struct nl_object *obj ATTRIBUTE_UNUSED, void *arg) {
>  +
>  +}
>  +#else 
Same as if_is_active()

>   /* add all ip addresses for the given interface to the xml document
>   */
>   static void add_ip_info_cb(struct nl_object *obj, void *arg) {
>  @@ -814,7 +839,9 @@
>   error:
>       return;
>   }
>  +#endif /* WIN32 */
>
>  +#ifdef HAVE_LIBNL
>   static void add_ip_info(struct netcf *ncf,
>                           const char *ifname ATTRIBUTE_UNUSED, int ifindex,
>                           xmlDocPtr doc, xmlNodePtr root) {
>  @@ -1141,7 +1168,7 @@
>   error:
>       return;
>   }
>  -
>  +#endif /* LIBNL */
>   /*
>    * Bringing interfaces up/down
>    */
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/dutil.h
> netcf-bm-clean/src/dutil.h
>  --- netcf/src/dutil.h	2010-08-27 11:13:14.259328697 -0400
>  +++ netcf-bm-clean/src/dutil.h	2010-08-27 11:20:55.177328537 -0400
>  @@ -25,7 +25,9 @@
>
>   #include <libxml/relaxng.h>
>   #include <libxslt/xsltInternals.h>
>  +#ifndef WIN32 
Should be #ifdef HAVE_LIBNL

>   #include <netlink/netlink.h>
>  +#endif
>
>   struct driver {
>       struct augeas     *augeas;
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/internal.h
> netcf-bm-clean/src/internal.h
>  --- netcf/src/internal.h	2010-08-27 11:13:14.259328697 -0400
>  +++ netcf-bm-clean/src/internal.h	2010-08-27 11:20:55.178328519 -0400
>  @@ -29,6 +29,7 @@
>
>   #include <string.h>
>   #include <stdarg.h>
>  +#include <stdio.h>
>
>   #include <libxslt/transform.h>
>
>  @@ -211,7 +212,6 @@
>   int ncf_put_aug(struct netcf *, const char *aug_xml, char **ncf_xml);
>   #endif
>
>  - 
Whitespace

>   /*
>    * Local variables:
>    *  indent-tabs-mode: nil
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/Makefile.am
> netcf-bm-clean/src/Makefile.am
>  --- netcf/src/Makefile.am	2010-08-27 11:13:14.258328785 -0400
>  +++ netcf-bm-clean/src/Makefile.am	2010-08-27 11:20:55.177328537 -0400
>  @@ -16,22 +16,26 @@
>   EXTRA_DIST = netcf_public.syms \
>   	netcf_private.syms
>
>  -if NETCF_DRIVER_INITSCRIPTS
>  -DRIVER_SOURCES = drv_initscripts.c dutil.h dutil.c \
>  -     dutil_linux.h dutil_linux.c
>  +DRIVER_SOURCES = dutil.h dutil.c netcf.c drv_initscripts.c 
Preserve the original order and just drop the backslash

>  +
>  +if PLATFORM_WIN32
>  +WIN32LIB= -liphlpapi
>  +DRIVER_SOURCES += netcf_win.h netcf_win.c
>  +else
>  +DRIVER_SOURCES += dutil_linux.c
>   endif
>
>   BUILT_SOURCES = datadir.h netcf.syms
>
>   DISTCLEANFILES = datadir.h
>
>  -libnetcf_la_SOURCES = netcf.h netcf.c internal.h \
>  +libnetcf_la_SOURCES = netcf.h internal.h \
>        ref.h list.h \
>        xslt_ext.c $(DRIVER_SOURCES)
>   libnetcf_la_LDFLAGS = -Wl,--version-script=netcf.syms \
>  -     -version-info $(LIBNETCF_VERSION_INFO)
>  -libnetcf_la_LIBADD = $(NETCF_LIBDEPS) $(GNULIB)
>  -libnetcf_la_DEPENDENCIES = $(libnetcf_la_LIBADD) netcf.syms
>  +     -version-info $(LIBNETCF_VERSION_INFO) @MINGW_EXTRA_LDFLAGS@
>  +libnetcf_la_LIBADD = $(NETCF_LIBDEPS) $(GNULIB) $(WIN32LIB)
>  +#libnetcf_la_DEPENDENCIES = $(libnetcf_la_LIBADD) netcf.syms 
You'll need to make this part of the patch conditional - otherwise
linux builds are going to fail.

>   ncftool_SOURCES = ncftool.c
>   ncftool_LDADD = libnetcf.la $(READLINE_LIBS) $(GNULIB)
>  @@ -55,6 +59,6 @@
>   internal.h: datadir.h
>
>   datadir.h: $(top_builddir)/config.status
>  -	echo '#define DATADIR "$(datadir)"' > datadir.h
>  +	echo '#define NETCF_DATADIR "$(datadir)"' > datadir.h 
No need to change this define.  See earlier comment.

>   DISTCLEANFILES += $(BUILT_SOURCES)
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/ncftool.c
> netcf-bm-clean/src/ncftool.c
>  --- netcf/src/ncftool.c	2010-08-27 11:13:14.259328697 -0400
>  +++ netcf-bm-clean/src/ncftool.c	2010-08-27 11:20:55.178328519 -0400
>  @@ -135,7 +135,7 @@
>       return result;
>   }
>
>  -static int cmd_list(ATTRIBUTE_UNUSED const struct command *cmd) {
>  +static int cmd_list(const struct command *cmd) { 
Not used at all or just on WIN32?

>       int nint;
>       char **intf;
>       unsigned int flags = NETCF_IFACE_ACTIVE;
>  @@ -146,7 +146,6 @@
>       if (opt_present(cmd, "all")) {
>           flags = NETCF_IFACE_ACTIVE | NETCF_IFACE_INACTIVE;
>       }
>  - 
Whitespace

>       nint = ncf_num_of_interfaces(ncf, flags);
>       if (nint < 0)
>           return CMD_RES_ERR;
>  @@ -229,6 +228,7 @@
>           goto done;
>       }
>
>  +#ifndef WIN32
>       if (opt_present(cmd, "live")) {
>           xml = ncf_if_xml_state(nif);
>       } else {
>  @@ -236,6 +236,7 @@
>       }
>       if (xml == NULL)
>           goto done;
>  +#endif /* WIN32 */
>
>       printf("%s\n", xml);
>       result= CMD_RES_OK;
>  @@ -677,7 +678,11 @@
>       while ((opt = getopt_long(argc, argv, "+dhr:", options, &idx)) != -1)
> {
>           switch(opt) {
>           case 'd':
>  +#ifdef WIN32
>  +            _putenv("NETCF_DEBUG=1");
>  +#else
>               setenv("NETCF_DEBUG", "1", 1);
>  +#endif
>               break;
>           case 'h':
>               usage();
>  @@ -768,7 +773,9 @@
>           if (ret == 0 && cmdstatus == CMD_RES_QUIT)
>               return ret;
>
>  +#ifndef WIN32 
HAVE_READLINE?

>           add_history(line);
>  +#endif
>           ret = cmdret;
>       }
>   }
>  @@ -786,7 +793,9 @@
>               print_netcf_error();
>           exit(EXIT_FAILURE);
>       }
>  +#ifndef WIN32 
HAVE_READLINE?

>       readline_init();
>  +#endif
>       if (optind < argc) {
>           /* Run a single command */
>           int i, ignore_status;
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/ncftransform.c
> netcf-bm-clean/src/ncftransform.c
>  --- netcf/src/ncftransform.c	2010-08-27 11:13:14.259328697 -0400
>  +++ netcf-bm-clean/src/ncftransform.c	2010-08-27 11:20:55.178328519 -0400
>  @@ -62,6 +62,7 @@
>       if (r < 0)
>           die("Failed to initialize netcf\n");
>
>  +#ifdef HAVE_LIBAUGEAS
>       if (STREQ(argv[1], "get")) {
>           r = ncf_get_aug(ncf, in_xml, &out_xml);
>       } else {
>  @@ -77,7 +78,7 @@
>       } else {
>           puts(out_xml);
>       }
>  -
>  +#endif /* LIBAUGEAS */
>       cleanup();
>   }
>   /*
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/netcf.c
> netcf-bm-clean/src/netcf.c
>  --- netcf/src/netcf.c	2010-08-27 11:13:14.260328610 -0400
>  +++ netcf-bm-clean/src/netcf.c	2010-08-27 11:20:55.178328519 -0400
>  @@ -28,14 +28,20 @@
>   #include <stdarg.h>
>   #include <stdio.h>
>   #include <unistd.h>
>  +#include <pthread.h>
>  + 
Whitespace

>   #include <sys/wait.h>
>  +
>   #include <signal.h>
>   #include <errno.h>
>   #include "safe-alloc.h"
>  -
>  + 
Whitespace

>   #include "internal.h"
>   #include "netcf.h"
>   #include "dutil.h"
>  +#ifdef WIN32
>  +#include "netcf_win.h"
>  +#endif
>
>   /* Clear error code and details */
>   #define API_ENTRY(ncf)                          \
>  @@ -88,18 +94,23 @@
>       if (make_ref(*ncf) < 0)
>           goto oom;
>       if (root == NULL)
>  +#ifdef WIN32
>  +        root = "c:\\"; 
Is that the kind of root its talking about?
Or more like a xpath query root?

>  +#else
>           root = "/";
>  +#endif
>       if (root[strlen(root)-1] == '/') {
>           (*ncf)->root = strdup(root);
>       } else {
>           if (xasprintf(&(*ncf)->root, "%s/", root) < 0)
>               goto oom;
>       }
>  +
>       if ((*ncf)->root == NULL)
>           goto oom;
>       (*ncf)->data_dir = getenv("NETCF_DATADIR");
>       if ((*ncf)->data_dir == NULL)
>  -        (*ncf)->data_dir = DATADIR "/netcf";
>  +        (*ncf)->data_dir = NETCF_DATADIR "/netcf"; 
Style

>       (*ncf)->debug = getenv("NETCF_DEBUG") != NULL;
>       return drv_init(*ncf);
>    oom:
>  @@ -116,6 +127,10 @@
>
>       ERR_COND_BAIL(ncf->ref > 1, ncf, EINUSE);
>
>  +#ifndef WIN32
>  +    *ncf = NULL;
>  +    ncf->driver = NULL;
>  +#endif
>       drv_close(ncf);
>       unref(ncf, netcf);
>       return 0;
>  @@ -131,15 +146,23 @@
>    */
>   int ncf_num_of_interfaces(struct netcf *ncf, unsigned int flags) {
>       API_ENTRY(ncf);
>  +#ifdef WIN32
>  +    return w32_num_of_interfaces(ncf, flags);
>  +#endif
>       return drv_num_of_interfaces(ncf, flags);
>   }
>
>  -int ncf_list_interfaces(struct netcf *ncf, int maxnames, char **names, unsigned int
> flags) {
>  +int ncf_list_interfaces(struct netcf *ncf, int maxnames,
>  +                        char **names, unsigned int flags) { 
Style

>       int result;
>
>       API_ENTRY(ncf);
>       MEMZERO(names, maxnames);
>  +#ifdef WIN32
>  +    result = w32_list_interfaces(ncf, maxnames, names, flags);
>  +#else
>       result = drv_list_interfaces(ncf, maxnames, names, flags);
>  +#endif
>       if (result < 0)
>           for (int i=0; i < maxnames; i++)
>               FREE(names[i]);
>  @@ -148,6 +171,9 @@
>
>   struct netcf_if * ncf_lookup_by_name(struct netcf *ncf, const char *name) {
>       API_ENTRY(ncf);
>  +#ifdef WIN32
>  +    return w32_lookup_by_name(ncf, name);
>  +#endif
>       return drv_lookup_by_name(ncf, name); 
Wouldn't it be preferrable to keep the same function name for these?

>   }
>
>  @@ -176,6 +202,9 @@
>
>   const char *ncf_if_mac_string(struct netcf_if *nif) {
>       API_ENTRY(nif->ncf);
>  +#ifdef WIN32
>  +    return w32_mac_string(nif);
>  +#endif 
As above

>       return drv_mac_string(nif);
>   }
>
>  @@ -189,6 +218,9 @@
>   int ncf_if_up(struct netcf_if *nif) {
>       /* I'm a bit concerned that this assumes nif (and nif->ncf) is non-NULL) */
>       API_ENTRY(nif->ncf);
>  +#ifdef WIN32
>  +    return w32_if_up(nif);
>  +#endif 
As above

>       return drv_if_up(nif);
>   }
>
>  @@ -196,9 +228,13 @@
>   int ncf_if_down(struct netcf_if *nif) {
>       /* I'm a bit concerned that this assumes nif (and nif->ncf) is non-NULL) */
>       API_ENTRY(nif->ncf);
>  +#ifdef WIN32
>  +    return w32_if_down(nif);
>  +#endif 
As above

>       return drv_if_down(nif);
>   }
>
>  +#ifndef WIN32
>   /* Produce an XML description for the interface, in the same format that
>    * NCF_DEFINE expects
>    */
>  @@ -216,6 +252,7 @@
>       API_ENTRY(nif->ncf);
>       return drv_xml_state(nif);
>   }
>  +#endif
>
>   /* Report various status info about the interface as bits in
>    * "flags". Returns 0 on success, -1 on failure
>  @@ -225,6 +262,16 @@
>       return drv_if_status(nif, flags);
>   }
>
>  +#ifdef WIN32
>  +/* Show ip addresses of active interfaces
>  + * Returns 0 on success, -1 on failure
>  + */
>  +int ncf_if_ipaddresses(struct netcf_if *nif) {
>  +    API_ENTRY(nif->ncf);
>  +    return w32_if_ipaddresses(nif);
>  +}
>  +#endif
>  +
>   /* Release any resources used by this NETCF_IF; the pointer is invalid
>    * after this call
>    */
>  @@ -247,6 +294,7 @@
>       return errcode;
>   }
>
>  +#ifdef HAVE_LIBAUGEAS
>   /*
>    * Test interface
>    */
>  @@ -261,6 +309,7 @@
>
>       return drv_put_aug(ncf, aug_xml, ncf_xml);
>   }
>  +#endif /* LIBAUGEAS */
>
>   /*
>    * Internal helpers
>  @@ -272,13 +321,16 @@
>                const char *commandline,
>                pid_t *pid)
>   {
>  +#ifdef HAVE_PTHREAD_SIGMASK
>       sigset_t oldmask, newmask;
>       struct sigaction sig_action;
>       char errbuf[128];
>  +#endif
>
>       /* commandline is only used for error reporting */
>       if (commandline == NULL)
>           commandline = argv[0];
>  +#ifdef HAVE_PTHREAD_SIGMASK
>       /*
>        * Need to block signals now, so that child process can safely
>        * kill off caller's signal handlers without a race.
>  @@ -332,11 +384,15 @@
>           /* don't report_error, as it will never be seen anyway */
>           _exit(1);
>       }
>  -
>       /* close all open file descriptors */
>  +#ifdef _SC_OPEN_MAX
>       int openmax = sysconf (_SC_OPEN_MAX);
>  +#else
>  +    int openmax = getdtablesize();
>  +#endif
>       for (i = 3; i < openmax; i++)
>           close(i);
>  +#endif
>
>       execvp(argv[0], (char **) argv);
>
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/netcf.h
> netcf-bm-clean/src/netcf.h
>  --- netcf/src/netcf.h	2010-08-27 11:13:14.260328610 -0400
>  +++ netcf-bm-clean/src/netcf.h	2010-08-27 11:20:55.178328519 -0400
>  @@ -56,7 +56,6 @@
>       NETCF_ENETLINK       /* something related to the netlink socket failed */
>   } netcf_errcode_t;
>
>  - 
Whitespace

>   /*
>    * flags accepted by ncf_num_of_interfaces and ncf_list_interfaces.
>    * IMPORTANT NOTE: These are bits, so you should assign only powers of two
>  @@ -169,6 +168,11 @@
>    */
>   int ncf_if_status(struct netcf_if *nif, unsigned int *flags);
>
>  +/* List ip addresses for each active interface
>  + * Returns 0 on success, -1 on failure
>  + */
>  +int ncf_if_ipaddresses(struct netcf_if *nif);
>  + 
Do we need this or is it just nice to have?
Because effectively we're changing their API here.

Probably best as a separate patch.

>   /* Release any resources used by this NETCF_IF; the pointer is
> invalid
>    * after this call
>    */
>  @@ -188,7 +192,6 @@
>   int ncf_error(struct netcf *, const char **errmsg, const char **details);
>   #endif
>
>  -
>   /*
>    * Local variables:
>    *  indent-tabs-mode: nil
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/netcf_win.c
> netcf-bm-clean/src/netcf_win.c
>  --- netcf/src/netcf_win.c	1969-12-31 19:00:00.000000000 -0500
>  +++ netcf-bm-clean/src/netcf_win.c	2010-08-27 11:24:47.357328517 -0400
>  @@ -0,0 +1,330 @@
>  +#include <config.h>
>  +#include <internal.h>
>  +
>  +#include <stdio.h>
>  +#include <stdlib.h>
>  +#include "netcf_win.h"
>  +#include "dutil.h"
>  +
>  +#define MAX_TRIES 3
>  +#define GAA_FLAGS ( GAA_FLAG_SKIP_DNS_SERVER | GAA_FLAG_SKIP_MULTICAST )
>  +#define BUFSIZE 8192
>  +
>  +#define MALLOC(x) HeapAlloc(GetProcessHeap(), 0, (x))
>  +#define FREE(x) HeapFree(GetProcessHeap(), 0, (x)) 
We cant just use malloc?

>  +
>  +PMIB_IPADDRTABLE _get_ip_addr_table(PMIB_IPADDRTABLE ipAddrTable) {
>  +    IN_ADDR addr;
>  +    DWORD r = 0;
>  +    DWORD buf = 0;
>  +
>  +    addr = (PMIB_IPADDRTABLE) MALLOC(sizeof (MIB_IPADDRTABLE));
>  +    if (GetIpAddrTable(addr, &buf, 0) == ERROR_INSUFFICIENT_BUFFER) {
>  +	FREE(addr);
>  +	addr = (PMIB_IPADDRTABLE) MALLOC(buf);
>  +	if (addr == NULL)
>  +	    goto error;
>  +    }
>  +
>  +    if((r = GetIpAddrTable(addr, &buf, 0)) == NO_ERROR)
>  +	return addr;
>  +
>  + error:
>  +    FREE(addr);
>  +    return ipAddrTable;
>  +}
>  +
>  +PMIB_IFTABLE _get_if_table(PMIB_IFTABLE intfTable) {
>  +    DWORD bufferLength = 0;
>  +    DWORD r = 0;
>  +    intfTable = (PMIB_IFTABLE) MALLOC(sizeof(MIB_IFTABLE));
>  +    if (intfTable == NULL)
>  +	goto error;
>  +
>  +    bufferLength = sizeof(MIB_IFTABLE);
>  +    if (GetIfTable(intfTable, &bufferLength, FALSE) == ERROR_INSUFFICIENT_BUFFER) {
>  +	FREE(intfTable);
>  +	intfTable = (PMIB_IFTABLE) MALLOC(bufferLength);
>  +	if (intfTable == NULL)
>  +	    goto error;
>  +    }
>  +
>  +    if ((r = GetIfTable(intfTable, &bufferLength, FALSE)) == NO_ERROR)
>  +	return intfTable;
>  + error:
>  +    FREE(intfTable);
>  +    return intfTable; 
Um...  you're returning a pointer to memory you just free'd


>  +}
>  +
>  +PIP_ADAPTER_ADDRESSES _get_ip_adapter_info(PIP_ADAPTER_ADDRESSES addrList) {
>  +    ULONG bufferLength = 0;
>  +    DWORD r;
>  +    int i;
>  +
>  +    for(i = 0; i < 5; i++) {
>  +	r = GetAdaptersAddresses(AF_INET, GAA_FLAGS, NULL, addrList, &bufferLength);
>  +	if (r != ERROR_BUFFER_OVERFLOW) {
>  +	    break;
>  +	}
>  +	if (addrList != NULL) {
>  +	    goto error;
>  +	}
>  +	addrList = (PIP_ADAPTER_ADDRESSES) MALLOC(bufferLength);
>  +	if (addrList == NULL)
>  +	    goto error;
>  +    }	
>  +    return addrList;
>  + error:
>  +    FREE(addrList);
>  +    return addrList; 
As above

>  +}
>  +
>  +int w32_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);
>  +    if(adapterp != NULL) {
>  +	for (nint = 0; adapterp != NULL; nint++) {
>  +	    if (names) {
>  +		char name[8192];
>  +		WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
>  +				    -1, name, sizeof(name), NULL, NULL);
>  +		names[nint] = strdup(name);
>  +	    }
>  +	    adapterp = adapterp->Next;
>  +	}
>  +    } else {
>  +	goto error;
>  +    }
>  +    return nint;
>  + error:
>  +    FREE(addrList);
>  +    return -1;
>  +}
>  +
>  +int w32_list_interfaces(struct netcf *ncf,
>  +		    int maxnames ATTRIBUTE_UNUSED, char **names,
>  +		    unsigned int flags ATTRIBUTE_UNUSED) {
>  +    return w32_list_interface_ids(ncf, 0, names, 0, NULL);
>  +}
>  +
>  +
>  +int w32_num_of_interfaces(struct netcf *ncf, unsigned int flags ATTRIBUTE_UNUSED) {
>  +    return w32_list_interface_ids(ncf, 0, NULL, 0, NULL);
>  +}
>  +
>  +
>  +struct netcf_if *w32_lookup_by_name(struct netcf *ncf) {
>  +    struct netcf_if *nif = NULL;
>  +    char *name_dup;
>  +    unsigned int nint = 0;
>  +    PIP_ADAPTER_ADDRESSES addrList = NULL;
>  +    PIP_ADAPTER_ADDRESSES adapterp = NULL;
>  +    adapterp = _get_ip_adapter_info(addrList);
>  +    if (adapterp != NULL) { 
Above if-clause is redundant

>  +	for (nint = 0; adapterp != NULL; nint++) {
>  +	    if (name) {
>  +		char wName[8192];
>  +		WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
>  +				    -1, wName, sizeof(wName), NULL, NULL);
>  +		if (strcmp(wName, nif->name) == 0) {
>  +		    name_dup = strdup(wName);
>  +		    nif = make_netcf_if(ncf, name_dup);
>  +		    goto done;
>  +		}
>  +	    }
>  +	    adapterp = adapterp->Next;
>  +	}
>  +    }
>  + done:
>  +    return nif;
>  +}
>  +
>  +
>  +const char *w32_mac_string(struct netcf_if *nif) {
>  +    // struct netcf *ncf = nif->ncf;
>  +    char mac[256];
>  +
>  +    PIP_ADAPTER_ADDRESSES addrList = NULL;
>  +    PIP_ADAPTER_ADDRESSES adapterp = NULL;
>  +    adapterp = _get_ip_adapter_info(addrList);
>  +    while(adapterp != NULL) {
>  +	char wName[8192];
>  +	WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
>  +			    -1, wName, sizeof(wName), NULL, NULL);
>  +       	if (strcmp(wName,nif->name) == 0) {
>  +	    if ((int)adapterp->PhysicalAddressLength > 6) 
> = ? 
>  +		continue; /* just want ethernet for now */
>  +	    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);
>  +	    goto done;
>  +	}
>  +	adapterp = adapterp->Next;
>  +    }
>  + done:
>  +    return nif->mac;
>  +}
>  +
>  +int w32_if_down(struct netcf_if *nif) {
>  +    PMIB_IFTABLE intfTable = NULL;
>  +    PMIB_IFTABLE intfTableDup = NULL;
>  +    PMIB_IFROW intRow;
>  +    int i;
>  +
>  +    intfTableDup = _get_if_table(intfTable);
>  +    if (intfTableDup != NULL) {
>  +	for (i = 0; i < (int) intfTableDup->dwNumEntries; i++) {
>  +	    intRow = (PMIB_IFROW) & intfTableDup->table[i];
>  +	    char wName[8192];
>  +	    WideCharToMultiByte(CP_UTF8, 0, intRow->wszName,
>  +				-1, wName, sizeof(wName), NULL, NULL);
>  +	    if (strcmp(wName,nif->name) == 0) {
>  +		intRow->dwAdminStatus = MIB_IF_ADMIN_STATUS_DOWN;
>  +		if (SetIfEntry(intRow) == NO_ERROR)
>  +		    goto done;
>  +	    }
>  +	}
>  +	/* Unable to shutdown interface */
>  +	return -1;
>  +    }
>  + done:
>  +    return 0;
>  +}
>  +
>  +int w32_if_up(struct netcf_if *nif) {
>  +    PMIB_IFTABLE intfTable = NULL;
>  +    PMIB_IFTABLE intfTableDup = NULL;
>  +    PMIB_IFROW intRow;
>  +    int i;
>  +
>  +    intfTableDup = _get_if_table(intfTable);
>  +    if (intfTableDup != NULL) {
>  +	for (i = 0; i < (int) intfTableDup->dwNumEntries; i++) {
>  +	    intRow = (PMIB_IFROW) & intfTableDup->table[i];
>  +	    char wName[8192];
>  +	    WideCharToMultiByte(CP_UTF8, 0, intRow->wszName,
>  +				-1, wName, sizeof(wName), NULL, NULL);
>  +	    if (strcmp(wName,nif->name) == 0) {
>  +		intRow->dwAdminStatus = MIB_IF_ADMIN_STATUS_UP;
>  +		if (SetIfEntry(intRow) == NO_ERROR)
>  +		    goto done;
>  +	    }
>  +	}
>  +	/* Unable to shutdown interface */
>  +	return -1;
>  +    }
>  + done:
>  +    return 0;
>  +}
>  +
>  +int w32_if_ipaddresses(struct netcf_if *nif, const 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) { 
Redundant if-clause

>  +	while(adapterp) {
>  +	    if (adapterp->OperStatus != 1)
>  +		continue;
>  +
>  +	    /* pull ip addresses from interface */
>  +	    for (i = 0; i<ipAddrTableDup->dwNumEntries; i++) {
>  +		if (ipAddrTableDup->table[i].dwIndex == adapterp->IfIndex) {
>  +		    char wName[8192];
>  +		    WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
>  +					-1, wName, sizeof(wName), NULL, NULL);
>  +		    if (strcmp(wName,nif->name) == 0) {
>  +			ipAddr.S_un.S_addr = (unsigned long) ipAddrTableDup->table[i].dwAddr;
>  +			sprintf(ipBuf,"%d",ntoa(ipAddr);
>  +			return 0;
>  +		    }
>  +		}
>  +	    }
>  +	    adapterp = adapterp->Next;
>  +	}
>  +    }
>  +    FREE(ipAddrTable);
>  +    FREE(addrList);
>  +    return -1;
>  +    }
>  +}
>  +
>  +int w32_add_ip_address(struct netcf_if *nif, UINT *ipAddr, UINT *netmask) {
>  +    IN_ADDR addr;
>  +    PMIB_IPADDRTABLE ipAddrTable = NULL;
>  +    PMIB_IPADDRTABLE ipAddrTableDup = NULL;
>  +    ULONG bufferLength = 0;
>  +    DWORD r;
>  +    DWORD ifIndex;
>  +    int i;
>  +
>  +    /* ipv4 addr/subnetmask */
>  +    UINT IPAddress;
>  +    UINT IPMask;
>  +
>  +    /* handles to IP returned */
>  +    ULONG NTEContext = 0;
>  +0    ULONG NTEInstance = 0; 
Stray character

>  +
>  +    if ((IPAddress = inet_addr(ipAddr)) == INADDR_NONE)
>  +	return -1;
>  +
>  +    if ((IPMask = inet_addr(netmask)) == INADDR_NONE)
>  +	return -1;
>  +
>  +
>  +    ipAddrTableDup = _get_ip_addr_table(ipAddrTable);
>  +    if (ipAddrTableDup != NULL) {
>  +	for(i=0;i<ipAddrTableDup->dwNumEntries;i++) {
>  +	    ifIndex = ipAddrTableDup->table[i].dwIndex;
>  +	    char wName[8192];
>  +	    WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
>  +				-1, wName, sizeof(wName), NULL, NULL);
>  +	    if (strcmp(wName,nif->name) == 0) {
>  +		if ((r = AddIPAddress(IPAddress, IPMask, ifIndex,
>  +				      &NTEContext, &NTEInstance)) == NO_ERROR) {
>  +		    return 0;
>  +		}
>  +	    }
>  +	}
>  +    }
>  +    FREE(ipAddrTable);
>  +    return -1;
>  +}
>  +
>  +
>  +int w32_rm_ip_address(struct netcf_if *nif, ULONG NTEContext) {
>  +    DWORD r = 0;
>  +    if ((r = DeleteIpAddress(NTEConext)) == NO_ERROR)
>  +	return 0;
>  +    return -1;
>  +}
>  +
>  +/* NOT IMPLEMENTED
>  +int w32_add_dns_server(struct netcf_if *nif, const char *dnsAddr) {
>  +    return 0; 
Should return -1

>  +}
>  +
>  +int w32_rm_dns_server(struct netcf_if *nif) {
>  +    return 0; 
Should return -1

>  +}
>  +*/
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/netcf_win.h
> netcf-bm-clean/src/netcf_win.h
>  --- netcf/src/netcf_win.h	1969-12-31 19:00:00.000000000 -0500
>  +++ netcf-bm-clean/src/netcf_win.h	2010-08-27 11:20:55.179328711 -0400
>  @@ -0,0 +1,80 @@
>  +/*
>  + * netcf-win.h: windows functions
>  + *
>  + * Copyright (C) 2009-2010 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, write to the Free Software
>  + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
>  + *
>  + * Author: Adam Stokes <ajs(a)redhat.com&gt;
>  + */
>  +
>  +#ifndef NETCF_WIN_H
>  +#define NETCF_WIN_H
>  +
>  +#ifndef WINVER
>  +#define WINVER 0x0501
>  +#endif
>  +
>  +#include <windows.h>
>  +#include <winsock.h>
>  +#include <winsock2.h>
>  +#include <ws2tcpip.h>
>  +#include <iphlpapi.h>
>  +#include "netcf.h"
>  +
>  +/* structure return of interface table */
>  +PMIB_IFTABLE _get_if_table(PMIB_IFTABLE intfTable);
>  +
>  +/* structure return of adapter info */
>  +PIP_ADAPTER_ADDRESSES _get_ip_adapter_info(PIP_ADAPTER_ADDRESSES addrList);
>  + 
See above.
We probably dont want the following definitions.

>  +/* return num of interfaces enabled */
>  +int w32_num_of_interfaces(struct netcf *ncf, unsigned int flags);
>  +
>  +/* helper for providing number of interfaces and associated names */
>  +int w32_list_interface_ids(struct netcf *ncf, int maxnames,
>  +			   char **names, unsigned int flags,
>  +			   const char *id_attr);
>  +			
>  +/* list interface names */
>  +int w32_list_interfaces(struct netcf *ncf,
>  +			int maxnames, char **names,
>  +			unsigned int flags);
>  +
>  +/* lookup interface by name */
>  +struct netcf_if *w32_lookup_by_name(struct netcf *ncf,
>  +				    const char *name);
>  +
>  +/* obtain hardware address of interface */
>  +const char *w32_mac_string(struct netcf_if *nif);
>  +
>  +/* bring interface down */
>  +int w32_if_down(struct netcf_if *nif);
>  +
>  +/* bring interface up */
>  +int w32_if_up(struct netcf_if *nif);
>  +
>  +/* Reports ip addresses */
>  +int w32_if_ipaddresses(struct netcf_if *nif);
>  +/* add ip address to device */
>  +int w32_add_ip_address(struct netcf_if *nif, const char *ipAddr);
>  +/* remove ip address from device */
>  +int w32_rm_ip_address(struct netcf_if *nif);
>  +/* add dns server to device */
>  +int w32_add_dns_server(struct netcf_if *nif, ULONG NTEContext);
>  +/* rm dns server from device */
>  +int w32_rm_dns_server(struct netcf_if *nif);
>  +
>  +#endif /* NETCF_WIN_H */
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/xslt_ext.c
> netcf-bm-clean/src/xslt_ext.c
>  --- netcf/src/xslt_ext.c	2010-08-27 11:13:14.260328610 -0400
>  +++ netcf-bm-clean/src/xslt_ext.c	2010-08-27 11:20:55.179328711 -0400
>  @@ -24,8 +24,8 @@
>   #include "internal.h"
>
>   #include <errno.h>
>  -#include <arpa/inet.h>
>
>  +#include <arpa/inet.h> 
Whitespace

>   #include <libxml/xpath.h>
>   #include <libxml/xpathInternals.h>
>   #include <libxslt/xsltutils.h>
>  @@ -106,7 +106,7 @@
>           goto error;
>       }
>
>  -    r = inet_aton((char *) netmask_str, &netmask);
>  +    r = inet_pton(AF_INET, (char *) netmask_str, &netmask);
>       if (r < 0) {
>           xsltTransformError(xsltXPathGetTransformContext(ctxt), NULL, NULL,
>                              "ipcalc:prefix: illegal netmask '%s'",
>

[Daniel P. Berrange]

On Fri, Aug 27, 2010 at 02:02:01PM -0400, ajs(a)redhat.com wrote:
>  I know this is the matahari list but I was asked to post patch here
> for technical review.
>  diff -Naur --exclude='.*' --exclude=ip_helper netcf/src/drv_initscripts.c
> netcf-bm-clean/src/drv_initscripts.c
>  --- netcf/src/drv_initscripts.c	2010-08-27 11:13:14.258328785 -0400
>  +++ netcf-bm-clean/src/drv_initscripts.c	2010-08-27 11:20:55.177328537 -0400
>  @@ -23,7 +23,10 @@
>   #include <config.h>
>   #include <internal.h>
>   
>  +#ifdef HAVE_LIBAUGEAS
>   #include <augeas.h>
>  +#endif
>  +
>   #include <stdlib.h>
>   #include <stdarg.h>
>   #include <stdio.h>
>  @@ -35,7 +38,9 @@
>   #include "ref.h"
>   #include "list.h"
>   #include "dutil.h"
>  +#ifndef WIN32
>   #include "dutil_linux.h"
>  +#endif
>   
>   #include <libxml/parser.h>
>   #include <libxml/relaxng.h>
>  @@ -100,7 +105,7 @@
>   
>   static const char *const prog_lokkit = "/usr/sbin/lokkit";
>   static const char *const lokkit_custom_rules =
>  -    "--custom-rules=ipv4:filter:" DATADIR
> "/netcf/iptables-forward-bridged";
>  +    "--custom-rules=ipv4:filter:" NETCF_DATADIR
> "/netcf/iptables-forward-bridged";
>   
>   static const char *const prog_rc_d_iptables = "/etc/init.d/iptables";
>   
>  @@ -163,7 +168,9 @@
>       int match = -1;
>       for (int i=0; i < nhwaddr; i++) {
>           const char *addr;
>  +#ifdef HAVE_LIBAUGEAS
>           r = aug_get(aug, hwaddr[i], &addr);
>  +#endif 

IMHO this is a pretty bad way to go about this. It is merely
making the code compile by deleting lines which are broken.
The resulting code will be totally non-functional, eg because
'r' in this case will be left uninitialized.

This source code file is basically Linux/Fedora specific, so
for Win32, it would be better to add a conditional rule to
the Makefile.am to skip compilation of the entire file. This
avoids the need to fill it with unusable #ifdefs.

>  diff -Naur --exclude='.*' --exclude=ip_helper
> netcf/src/netcf.c netcf-bm-clean/src/netcf.c
>  --- netcf/src/netcf.c	2010-08-27 11:13:14.260328610 -0400
>  +++ netcf-bm-clean/src/netcf.c	2010-08-27 11:20:55.178328519 -0400
>  @@ -131,15 +146,23 @@
>    */
>   int ncf_num_of_interfaces(struct netcf *ncf, unsigned int flags) {
>       API_ENTRY(ncf);
>  +#ifdef WIN32
>  +    return w32_num_of_interfaces(ncf, flags);
>  +#endif
>       return drv_num_of_interfaces(ncf, flags);
>   } 
This will generate compiler warnings about unreachable code,
unless you put the second line within an  #else clause. There
are similar issus later in the same file. If it doesn't
generate warnings, then netcf needs more GCC warnings turned
on :-)

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 :|

[Adam Stokes]

----- "Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote:

>  > @@ -163,7 +168,9 @@
>  >      int match = -1;
>  >      for (int i=0; i < nhwaddr; i++) {
>  >          const char *addr;
>  > +#ifdef HAVE_LIBAUGEAS
>  >          r = aug_get(aug, hwaddr[i], &addr);
>  > +#endif
>  
>  
>  IMHO this is a pretty bad way to go about this. It is merely
>  making the code compile by deleting lines which are broken.
>  The resulting code will be totally non-functional, eg because
>  'r' in this case will be left uninitialized.
>  
>  This source code file is basically Linux/Fedora specific, so
>  for Win32, it would be better to add a conditional rule to
>  the Makefile.am to skip compilation of the entire file. This
>  avoids the need to fill it with unusable #ifdefs.
>   
Thanks for the input Dan, I totally agree and will look at an alternative approach
to this porting effort.

-- 
        __          __                
.-----.|  |_.-----.|  |--.-----.-----.
|__ --||   _|  _  ||    <|  -__|__ --|
|_____||____|_____||__|__|_____|_____|
 .----------------------------------.
(   ajs(a)redhat.com || 919.754.4187   )
 `----------------------------------'