[netcf-devel] [PATCH 1/5] Windows port of netcf api calls

Eric Blake eblake at redhat.com
Wed Sep 22 17:28:00 UTC 2010


On 09/17/2010 10:28 AM, Adam Stokes wrote:

A one-line commit message (the subject line) is rather short, but no 
need to make it longer if you can't think of anything else meaningful to 
add.  Ultimately, the goal is to achieve a balance so that someone 
reading 'git log' gets a good feel for whether they need to further 
investigate a particular patch.

Caveat - I've never packaged a mingw32 package before, so I'm (blindly) 
using libvirt/mingw32-libvirt.spec.in as my comparison for how it should 
be done.

> ---
>   mingw32-netcf.spec.in |   67 +++++++++
>   netcf-wininst.nsi     |   37 +++++
>   src/netcf_win.c       |  358 +++++++++++++++++++++++++++++++++++++++++++++++++
>   src/netcf_win.h       |   70 ++++++++++
>   4 files changed, 532 insertions(+), 0 deletions(-)
>   create mode 100644 mingw32-netcf.spec.in
>   create mode 100755 netcf-wininst.nsi
>   create mode 100644 src/netcf_win.c
>   create mode 100644 src/netcf_win.h
>
> diff --git a/mingw32-netcf.spec.in b/mingw32-netcf.spec.in
> new file mode 100644
> index 0000000..fe0e073
> --- /dev/null
> +++ b/mingw32-netcf.spec.in
> @@ -0,0 +1,67 @@
> +%global __strip %{_mingw32_strip}
> +%global __objdump %{_mingw32_objdump}
> +%global _use_internal_dependency_generator 0
> +%global __find_requires %{_mingw32_findrequires}
> +%global __find_provides %{_mingw32_findprovides}
> +%define __debug_install_post %{_mingw32_debug_install_post}
> +
> +Name:           mingw32-netcf
> +Version:        @VERSION@
> +Release:        1%{?dist}%{?extra_release}
> +Summary:        Cross-platform network configuration library

The summary line should specifically mention MinGW rather than the vague 
term 'cross-platform'.

> +
> +Group:          System Environment/Libraries
> +License:        LGPLv2+
> +URL:            https://fedorahosted.org/netcf/
> +Source0:        http://astokes.fedorapeople.org/%{name}/%{name}-%{version}.tar.gz

Should this ultimately be moved to fedorahosted.org, alongside the main 
netcf release?  But this is okay for now.

> +BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> +
> +BuildRequires:  mingw32-filesystem>= 56
> +BuildRequires:  mingw32-readline
> +BuildRequires:  mingw32-binutils
> +BuildRequires:  mingw32-libxml2

Trailing whitespace. (Ouch - I hate thunderbird's bug that reformats 
quoted text, and in turn ate the trailing whitespace from my reply).

Independent of your patch, it may be worth copying libvirt's example and 
enabling 'make syntax-check' to catch things like trailing whitespace.

> +BuildRequires:  mingw32-libxslt
> +BuildRequires:  mingw32-gcc
> +BuildRequires:  mingw32-gettext
> +BuildRequires:  mingw32-w32api
> +Requires:       pkgconfig

Following libvirt's example, I think you are missing:

# Need native version for msgfmt
BuildRequires: gettext

Most mingw32 packages are arch-independent (the binaries in the package 
are win32, and independent of whether the host is i686 or x86_64), so I 
thin you are missing:

BuildArch: noarch

Again following libvirt's example, I think you are missing:

%{?_mingw32_debug_package}

> +
> +%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}

Is the PATH setting needed for just the configure, or should it also be 
set in make?  If so, make it an argument to configure.  Libvirt didn't 
munge PATH, so I'm not quite sure why you needed to.

> +
> +%install
> +rm -rf $RPM_BUILD_ROOT
> +make install DESTDIR=$RPM_BUILD_ROOT INSTALL="%{__install} -p"

Hmm, libvirt didn't override INSTALL, so I'm not quite sure why you had 
to do so.

> +rm -rf $RPM_BUILD_ROOT%{_mingw32_datadir}/netcf
> +
> +%clean
> +rm -rf $RPM_BUILD_ROOT
> +
> +%files
> +%defattr(-,root,root,-)
> +%{_mingw32_bindir}/ncftool.exe
> +%{_mingw32_bindir}/libnetcf-1.dll
> +%{_mingw32_includedir}/*.h
> +%{_mingw32_libdir}/pkgconfig/netcf.pc
> +%{_mingw32_libdir}/libnetcf.dll.a
> +%{_mingw32_libdir}/libnetcf.la
> +%{_mingw32_datadir}

Does this line need a %dir prefix?

> +
> +%doc AUTHORS COPYING NEWS
> +
> +%changelog
> +* Mon Sep 1 2010 Adam Stokes<ajs at redhat.com>  - 0.1.6-1
> +- MinGW Port
> diff --git a/netcf-wininst.nsi b/netcf-wininst.nsi
> new file mode 100755
> index 0000000..d0d1687
> --- /dev/null
> +++ b/netcf-wininst.nsi
> @@ -0,0 +1,37 @@
> +; netcf-wininst.nsi
> +; prompt for dir; copy files
> +;--------------------------------
> +
> +; The name of the installer
> +Name "netcf installer"
> +
> +; The file to write
> +OutFile "netcf-setup.exe"
> +
> +; The default installation directory
> +InstallDir $PROGRAMFILES\netcf
> +
> +; Request application privileges for Windows Vista
> +RequestExecutionLevel user
> +
> +;--------------------------------
> +
> +; Pages
> +
> +Page directory
> +Page instfiles
> +
> +;--------------------------------
> +
> +; The stuff to install
> +Section "" ;No components page, name is not important
> +
> +  ; Set output path to the installation directory.
> +  SetOutPath $INSTDIR
> +
> +  ; Put file there
> +  File src/.libs/libnetcf-1.dll
> +  File src/.libs/ncftool.exe
> +  File /usr/i686-pc-mingw32/sys-root/mingw/bin/*.dll
> +
> +SectionEnd ; end the section

No personal experience with .nsi files, but nothing obvious jumped out 
of me as wrong, and I'm assuming you've tested it.

> diff --git a/src/netcf_win.c b/src/netcf_win.c
> new file mode 100644
> index 0000000..f0a9818
> --- /dev/null
> +++ b/src/netcf_win.c
> @@ -0,0 +1,358 @@
> +#include<config.h>
> +#include<internal.h>
> +
> +#include<stdio.h>
> +#include<stdlib.h>
> +#include "netcf_win.h"
> +
> +#define ADDR_BLOCK 5
> +#define GAA_FLAGS ( GAA_FLAG_SKIP_DNS_SERVER | GAA_FLAG_SKIP_MULTICAST )
> +#define BUFSIZE 1024
> +
> +PMIB_IPADDRTABLE _get_ip_addr_table(PMIB_IPADDRTABLE ipAddrTable) {

Since this function starts with an underscore, should it be marked 
static?  That is, it is seldom wise to export a name with a leading 
underscore.

I'm not sure if netcf has a particular style, but I prefer:

PMIB_IPADDRTABLE
_get_ip_addr_table(PMIB_IPADDRTABLE ipAddrTable)
{

That is, separate the lines for the return type and function name, and 
start the open brace on it's own line at the implementation (while 
keeping the return type and name on the same line for forward 
declarations).  Why? Because some tools, including emacs and git diff, 
are hard-coded to recognize this pattern, making it easier to quickly 
locate a function implementation by filtering based on '^_get_ip_addr' 
without having to filter through all the call sites.

But libvirt does not strictly follow this style, and unless netcf has a 
policy, your format does not need to be changed.

> +    DWORD r = 0;
> +    DWORD buf = 0;
> +
> +    ipAddrTable = (PMIB_IPADDRTABLE) malloc(sizeof (MIB_IPADDRTABLE));

I much prefer:

ipAddrTable = malloc(sizeof (*ipAddrTable));

In C, the cast of malloc results is redundant, and using *var instead of 
(type) in the sizeof makes you immune to changing the type of var.

Maybe it would make sense as a separate patch to mimic libvirt's wrapper 
macro VIR_ALLOC, which also sets a compiler attribute to warn if you 
failed to check for ENOMEM condition...

> +    if (GetIpAddrTable(ipAddrTable,&buf, 0) == ERROR_INSUFFICIENT_BUFFER) {

...since this is a prime example where you would crash if the malloc 
failed, because you end up passing a NULL pointer to GetIpAddrTable. 
(Actually, I haven't looked at the windows documentation; if 
GetIpAddrTable is documented as gracefully handling a NULL argument with 
an appropriate error, you may be okay here).

> +	free(ipAddrTable);
> +	ipAddrTable = (PMIB_IPADDRTABLE) malloc(buf);

Again, in C, casting malloc results is redundant.

> +	if (ipAddrTable == NULL)
> +	    goto error;
> +    }
> +
> +    if((r = GetIpAddrTable(ipAddrTable,&buf, 0)) == NO_ERROR)

Hmm, this calls GetIpAddrTable twice, even when the first one succeeded. 
  I would instead write:

if ((r = GetIpAddrTable()) == ERROR_INSUFFICIENT_BUFFER) {
   ...
   r = GetIpAddrTable();
}
if (r == ERROR_INSUFFICIENT_BUFFER)
   ...

so that the second call is only used if the first one failed and you 
realloc'd accordingly.


> +	return ipAddrTable;
> +
> + error:
> +    free(ipAddrTable);
> +    return ipAddrTable;

Ouch.  This returns a stale pointer, which is a no-no.  Return NULL on 
failure.  Better yet, should netcf have a wrapper mimicking libvirt's 
VIR_FREE(var), which guarantees that var is set to NULL after releasing 
the memory?

> +}
> +
> +PMIB_IFTABLE _get_if_table(PMIB_IFTABLE intfTable) {
> +    DWORD bufferLength = 0;
> +    DWORD r = 0;
> +    intfTable = (PMIB_IFTABLE) malloc(sizeof(MIB_IFTABLE));

Hmm - this blindly ignores the incoming pointer.  Any reason you are 
passing it as an argument, only to ignore it?

> +    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)

Similar arguments about the double GetIfTable on success.

> +	return intfTable;
> + error:
> +    free(intfTable);
> +}

Missing return statement?  Was there a compiler warning you overlooked?

> +
> +PIP_ADAPTER_ADDRESSES _get_ip_adapter_info(PIP_ADAPTER_ADDRESSES addrList) {
> +    ULONG bufferLength = 0;
> +    DWORD r;
> +    int i;

I recommend using size_t instead of int when declaring an iterator, so 
long as the compiler doesn't warn about it.

> +
> +    for(i = 0; i<  ADDR_BLOCK; i++) {
> +	r = GetAdaptersAddresses(AF_INET, GAA_FLAGS, NULL, addrList,&bufferLength);
> +	if (r != ERROR_BUFFER_OVERFLOW) {
> +	    break;
> +	}
> +	if (addrList != NULL) {
> +	    goto error;
> +	}

Hmm - if the user passes in a non-null addrList, then the first 
iteration of this loop calls GetAdaptersAddresses(,,,ptr,0), which 
should cause ERROR_BUFFER_OVERFLOW, and immediately goto error.  So it 
seems like the user has to always pass in NULL to get this to work, so 
why is addrList an input parameter in the first place?

> +	addrList = (PIP_ADAPTER_ADDRESSES) malloc(bufferLength);
> +	if (addrList == NULL)
> +	    goto error;
> +    }	

Trailing whitespace.

> +    return addrList;

Logic bug?  You break out of the loop for anything other than 
ERROR_BUFFER_OVERFLOW, but you aren't detecting whether you had NO_ERROR 
or some other real error, but blindly return the pointer anyways as if 
you had always succeeded.

> + error:
> +    free(addrList);
> +}

Missing return statement.

> +
> +/* Create a new netcf if instance for interface NAME */
> +struct netcf_if *make_netcf_if(struct netcf *ncf, char *name) {

Should this be const char *name?

> +    int r;
> +    struct netcf_if *result = NULL;
> +
> +    r = make_ref(result);
> +    ERR_NOMEM(r<  0, ncf);
> +    result->ncf = ref(ncf);
> +    result->name = name;

Should this be a strdup() of name?

> +    return result;
> +
> + error:
> +    unref(result, netcf_if);
> +    return result;
> +}
> +
> +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) {

For extensibility reasons, it's always better to check that flags == 0, 
rather than marking it as ATTRIBUTE_UNUSED - that way, if a future 
extension comes up with a reasonable flag, but then calls older code 
that does not know how to handle the flag, you gracefully exit with 
error instead of proceeding without acting on the flag.

> +    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++) {

Potential buffer overflow - how do you know that you are not stepping 
beyond maxnames positions in the names array?  maxnames should not be 
ATTRIBUTE_UNUSED.

> +	if (names) {
> +	    char name[BUFSIZE];
> +	    WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
> +				-1, name, sizeof(name), NULL, NULL);

Can this conversion ever fail?

> +	    names[nint] = strdup(name);

Check for memory allocation failure.

> +	}
> +	adapterp = adapterp->Next;
> +    }
> +    return nint;
> + error:
> +    free(addrList);

Memory leak corner case - if you allocate some, but not all, of 
names[nint], then you need to free the ones you did allocate.

> +    return -1;
> +}
> +
> +int drv_list_interfaces(struct netcf *ncf,
> +		    int maxnames ATTRIBUTE_UNUSED, char **names,
> +		    unsigned int flags ATTRIBUTE_UNUSED) {
> +    return list_interface_ids(ncf, 0, names, 0, NULL);

Shouldn't this pass maxnames and flags on through?

> +}
> +
> +
> +int drv_num_of_interfaces(struct netcf *ncf, unsigned int flags ATTRIBUTE_UNUSED) {
> +    return list_interface_ids(ncf, 0, NULL, 0, NULL);

Shouldn't this pass flags on through?

> +}
> +
> +
> +struct netcf_if *drv_lookup_by_name(struct netcf *ncf, const char *name) {
> +    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);
> +    for (nint = 0; adapterp != NULL; nint++) {
> +	if (name) {
> +	    char wName[BUFSIZE];
> +	    WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
> +				-1, wName, sizeof(wName), NULL, NULL);
> +	    if (strcmp(wName, name) == 0) {
> +		name_dup = strdup(wName);
> +		nif = make_netcf_if(ncf, name_dup);

Does this leak name_dup?

> +		goto done;
> +	    }
> +	}
> +	adapterp = adapterp->Next;
> +    }
> +
> + done:
> +    return nif;
> +}
> +
> +
> +const char *drv_mac_string(struct netcf_if *nif) {
> +    // struct netcf *ncf = nif->ncf;

Why keep this comment?

> +
> +    PIP_ADAPTER_ADDRESSES addrList = NULL;
> +    PIP_ADAPTER_ADDRESSES adapterp = NULL;
> +    adapterp = _get_ip_adapter_info(addrList);
> +    while(adapterp != NULL) {
> +	char wName[BUFSIZE];
> +	WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
> +			    -1, wName, sizeof(wName), NULL, NULL);
> +       	if (strcmp(wName,nif->name) == 0) {
> +	    if ((int)adapterp->PhysicalAddressLength>= 6)

Why the cast?

> +		continue; /* just want ethernet for now */
> +	    nif->mac = asprintf(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]);

Is adapterp->PhysicalAddress[0] signed or unsigned?  If signed, then you 
need to mask off any potential sign-extensions, or use "%02hhX" (except 
that mingw probably doesn't support the hh modifier, and I don't know if 
gnulib's asprintf guarantees it).

> +	    goto done;
> +	}
> +	adapterp = adapterp->Next;
> +    }
> + done:
> +    return nif->mac;

Calling this repeatedly is a memory leak; nif->mac will be overwritten 
on each call.  Should you be checking if nif->mac is non-NULL and 
directly returning it in that case, rather than hunting through adapterp 
in that case?

> +}
> +
> +int drv_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++) {

Why the cast?  Why not make i the correct type to begin with?

> +	    intRow = (PMIB_IFROW)&  intfTableDup->table[i];
> +	    char wName[BUFSIZE];
> +	    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;
> +}

Memory leak.  _get_if_table mallocs, but you never free intfTable and 
never return it to the user.

> +
> +int drv_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[BUFSIZE];
> +	    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;
> +}

Same memory leak.

> +
> +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;

Why ipAddrTable and ipAddrTableDup, since ipAddrTable is always NULL?

> +
> +    adapterp = _get_ip_adapter_info(addrList);
> +    if (adapterp != NULL) {
> +	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;

Why the cast?

> +			sprintf(ipBuf,"%d",inet_ntoa(ipAddr));

Where is the guarantee that ipBuf is big enough for this sprintf, or is 
there a possibility of buffer overflow?

> +			return 0;
> +		    }
> +		}
> +	    }
> +	    adapterp = adapterp->Next;
> +	}
> +
> +    }
> +    free(ipAddrTable);

Memory leak - you are freeing the always-NULL ipAddrTable, but not the 
malloc'd ipAddrTableDup.

> +    free(addrList);
> +    return -1;
> +}
> +
> +int drv_add_ip_address(struct netcf_if *nif, char *ipAddr, char *netmask) {
> +    IN_ADDR addr;
> +    PIP_ADAPTER_ADDRESSES addrList = NULL;
> +    PIP_ADAPTER_ADDRESSES adapterp = NULL;
> +    PMIB_IPADDRTABLE ipAddrTable = NULL;
> +    PMIB_IPADDRTABLE ipAddrTableDup = NULL;

Again, why ipAddrTable, if it is always NULL?

> +    ULONG bufferLength = 0;
> +    DWORD r;
> +    DWORD ifIndex;
> +    int i;
> +
> +    /* ipv4 addr/subnetmask */
> +    UINT IPAddress;
> +    UINT IPNetMask;
> +
> +    /* handles to IP returned */
> +    ULONG NTEContext = 0;
> +    ULONG NTEInstance = 0;
> +
> +    if ((IPAddress = inet_addr(ipAddr)) == INADDR_NONE)
> +	return -1;
> +
> +    if ((IPNetMask = 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[BUFSIZE];
> +	    WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
> +				-1, wName, sizeof(wName), NULL, NULL);
> +	    if (strcmp(wName,nif->name) == 0) {
> +		if ((r = AddIPAddress(IPAddress, IPNetMask, ifIndex,
> +				&NTEContext,&NTEInstance)) == NO_ERROR) {
> +		    return 0;
> +		}
> +	    }
> +	}
> +    }
> +    free(ipAddrTable);

Memory leak, freeing the wrong variable.

> +    return -1;
> +}
> +
> +
> +/* needs further testing
> +int drv_rm_ip_address(struct netcf_if *nif, ULONG NTEContext) {
> +    DWORD r = 0;
> +    if ((r = DeleteIpAddress(NTEConext)) == NO_ERROR)
> +	return 0;
> +    return -1;
> +}
> +*/
> +
> +int drv_list_dns_server(struct netcf_if *nif, char *ip_str) {
> +    char bufferLength[1024];
> +    IP4_ARRAY *ips = (IP4_ARRAY*) bufferLength;
> +    DWORD len = sizeof(bufferLength);
> +    DNS_STATUS status;
> +
> +    status = DnsQueryConfig(DnsConfigDnsServerList, FALSE,
> +			    NULL, NULL, ips,&len);
> +    if (status == 0) {
> +	DWORD i;
> +	for (i = 0; i<  ips->AddrCount; i++) {
> +	    DWORD ip = ips->AddrArray[i];
> +	    snprintf(ip_str, sizeof(ip_str),

sizeof(ip_str) == sizeof(char*), which is probably not what you meant 
here.  Why is the length of ip_str not a parameter to this function?

> +		     "%d.%d.%d.%d", (ip>>  0)&  255, (ip>>  8)&  255,
> +		     (ip>>  16)&  255, (ip>>  24)&  255);

Maybe it's just me, but I prefer 0xff rather than 255 when masking bits. 
  If we could guarantee that gnulib gives us %hhd support, you could 
avoid the mask (and just do the shift).

> +	}
> +    } else {
> +	return -1;
> +    }
> +    return 0;
> +}
> +
> +/* NOT IMPLEMENTED
> +int drv_add_dns_server(struct netcf_if *nif, const char *dnsAddr) {
> +    return -1;
> +}
> +
> +int drv_rm_dns_server(struct netcf_if *nif) {
> +    return -1;
> +}
> +*/
> diff --git a/src/netcf_win.h b/src/netcf_win.h
> new file mode 100644
> index 0000000..1b88772
> --- /dev/null
> +++ b/src/netcf_win.h
> @@ -0,0 +1,70 @@
> +/*
> + * netcf-win.h: windows functions
> + *
> + * Copyright (C) 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 at redhat.com>
> + */
> +
> +#ifndef NETCF_WIN_H
> +#define NETCF_WIN_H
> +
> +#ifndef WINVER
> +#define WINVER 0x0501

I like using cppi, where this would be:

#ifndef WINVER
# define WINVER 0x0501
#endif

but I don't know if netcf plans on copying libvirt's lead in this style 
matter.

> +#endif
> +
> +#include<config.h>

Generally, if all .c files include <config.h> first, then none of the .h 
files should include it.

> +#include "internal.h"
> +
> +#include<stdbool.h>
> +#include<string.h>
> +#include<windows.h>
> +#include<winsock.h>
> +#include<winsock2.h>

Do you really need <winsock.h>, or is <winsock2.h> sufficient?

> +#include<ws2tcpip.h>
> +#include<iphlpapi.h>
> +#include<windns.h>
> +#include "safe-alloc.h"
> +#include "ref.h"
> +#include "list.h"
> +
> +struct netcf_if *make_netcf_if(struct netcf *ncf, char *name);
> +
> +/* structure return of interface table */
> +PMIB_IFTABLE _get_if_table(PMIB_IFTABLE intfTable);

Should this (and related helper function) declarations be moved to 
netcf_win.c, as part of making the functions static, since you probably 
should not be exporting them?

> +
> +/* structure return of adapter info */
> +PIP_ADAPTER_ADDRESSES _get_ip_adapter_info(PIP_ADAPTER_ADDRESSES addrList);
> +
> +PMIB_IPADDRTABLE _get_ip_addr_table(PMIB_IPADDRTABLE ipAddrTable);
> +
> +/* Reports ip addresses */
> +int drv_if_ipaddresses(struct netcf_if *nif, char *ipBuf);
> +
> +/* add ip address to device */
> +int drv_add_ip_address(struct netcf_if *nif, char *ipAddr,
> +		       char *netmask);
> +/* remove ip address from device */
> +int drv_rm_ip_address(struct netcf_if *nif, ULONG NTEContext);

Using ULONG in a public header seems odd, should this be a more 
standardized type, like uint32_t?

> +/* add dns server to device */
> +int drv_add_dns_server(struct netcf_if *nif, ULONG NTEContext);
> +/* rm dns server from device */
> +int drv_rm_dns_server(struct netcf_if *nif);
> +/* list dns server */
> +int drv_list_dns_server(struct netcf_if *ncf, char *ip_str);
> +
> +#endif /* NETCF_WIN_H */

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


More information about the netcf-devel mailing list