[netcf-devel] [PATCH] Windows port

Eric Blake eblake at redhat.com
Thu Sep 9 19:10:31 UTC 2010


[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 at 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 at 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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


More information about the netcf-devel mailing list