[netcf-devel] [PATCH 1/2] Windows port - routines patch

Laine Stump laine at laine.org
Tue Nov 9 18:45:40 UTC 2010


On 11/08/2010 02:02 PM, Adam Stokes wrote:
> ---
>   src/netcf_win.c |  253 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/netcf_win.h |   54 ++++++++++++


Okay, so this appears to be your "drv_mswindows.c". It would be good to 
keep the naming consistent with the existing platform-specific files, so 
that as the list of supported platforms grows, it's easy to spot which 
files are generic across all platforms, and what file(s) you need to 
recreate for a new port.


>   2 files changed, 307 insertions(+), 0 deletions(-)
>   create mode 100644 src/netcf_win.c
>   create mode 100644 src/netcf_win.h
>
> diff --git a/src/netcf_win.c b/src/netcf_win.c
> new file mode 100644
> index 0000000..e42f5b1
> --- /dev/null
> +++ b/src/netcf_win.c
> @@ -0,0 +1,253 @@
> +/*
> + * netcf-win.c: 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>
> + */
> +
> +#include<config.h>
> +#include<internal.h>
> +
> +#include<stdio.h>
> +#include<stdlib.h>
> +#include<spawn.h>
> +#include "netcf_win.h"
> +
> +#define GAA_FLAGS ( GAA_FLAG_SKIP_DNS_SERVER | GAA_FLAG_SKIP_MULTICAST | GAA_FLAG_SKIP_ANYCAST )
> +#define BUFSIZE 1024
> +
> +char *strerror_r(int errnum, char *buf, size_t buflen) {
> +    return strerror(errnum);
> +}

This has already been covered elsewhere.


> +
> +/* Like asprintf, but set *STRP to NULL on error */
> +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;
> +}


Rather than duplicating this function (already present in dutil.c), 
please use the one there. In order to make dutil.c build on Windows, any 
function in dutil.c that isn't applicable to MS Windows should be moved 
to dutil_linux.c (that file was created later, with the intent of 
migrating Linux OS-specific functions to there).


> +
> +/* Create a new netcf if instance for interface NAME */
> +struct netcf_if *make_netcf_if(struct netcf *ncf, char *name) {
> +    int r;
> +    struct netcf_if *result = NULL;
> +
> +    r = make_ref(result);
> +    ERR_NOMEM(r<  0, ncf);
> +    result->ncf = ref(ncf);
> +    result->name = strdup(name);


The callers to make_netcf_if have already strdup()ed name before calling 
(both in the Linux version, and in your new code here). You're creating 
an extra copy, and leaking the one that was previously strduped in the 
caller.


> +    return result;
> +
> + error:
> +    unref(result, netcf_if);
> +    free(result);
> +    return result;


???? You just free()ed result, then you return it.

Also, that and strdup'ing the name (which will have already been 
strduped by the caller) are the only differences between this and the 
version of make_netcf_if() in dutil.c. I think maybe you're making the 
cut-line for port-specific functions a layer too high in this case (as 
well as in your #ifdefs in netcf.c, but maybe that's temporary?)


> +}
> +
> +PIP_ADAPTER_ADDRESSES build_adapter_table(struct netcf *ncf) {
> +    int r = 0;
> +    DWORD tableSize = 0;
> +    PIP_ADAPTER_ADDRESSES pAddresses = NULL;
> +
> +    GetAdaptersAddresses(AF_UNSPEC, GAA_FLAGS, NULL, pAddresses,&tableSize);
> +    pAddresses = malloc(tableSize);


Please don't use malloc(). Use ALLOC() and ALLOC_N() instead.


> +    ERR_NOMEM(pAddresses == NULL, ncf);
> +    r = GetAdaptersAddresses(AF_INET, GAA_FLAGS, NULL, pAddresses,&tableSize);
> +    ERR_COND_BAIL(r != NO_ERROR, ncf, EOTHER);
> +    return pAddresses;
> +error:
> +    free(pAddresses);
> +    return NULL;
> +}

I'll have to take your word for this one :-)


> +
> +static int list_interface_ids(struct netcf *ncf,
> +                              int maxnames,
> +                              char **names, unsigned int flags,
> +                              const char *id_attr) {
> +    size_t nint = 0;
> +    int r = 0;
> +    DWORD tableSize = 0;
> +    IP_ADAPTER_ADDRESSES *adapter;
> +
> +    adapter = build_adapter_table(ncf);
> +    ERR_COND_BAIL(adapter == NULL, ncf, EOTHER);
> +
> +    while(adapter) {
> +        if(names) {
> +            char wName[8192];
> +            r = WideCharToMultiByte(CP_UTF8, 0, adapter->FriendlyName,
> +                                    -1, wName, sizeof(wName), NULL, NULL);
> +            ERR_NOMEM(r == 0, ncf);
> +            names[nint] = strdup(wName);
> +            ERR_NOMEM(names[nint] == NULL, ncf);


If I recall correctly, you were having trouble finding a way to 
determine whether or not an interface was currently up, which I suppose 
is why you don't do anything with the NETCF_IFACE_ACTIVE and 
NETCF_IFACE_INACTIVE flags here...


> +        }
> +        nint++;
> +        adapter = adapter->Next;
> +    }
> +    return nint;
> + error:
> +    free(adapter);
> +    while(nint>  0) {
> +        free(names[nint]);
> +        nint--;


0) This is a dual-purpose function, and can be called with names==NULL. 
You haven't qualified the free() here for that case.

1) Did you intend to not free names[0] in case of error? You're only 
freeing [1], [2], etc.

2) If you're going to free these here, you should use FREE(), so that 
the pointers in names are NULLed out, to avoid the possibility of the 
caller attempting to free them (which is exactly what 
ncf_list_interfaces() does).

3) Really though, since the one and only caller of list_interface_ids() 
that doesn't pass in a NULL names is already free'ing all the items, you 
should just remove that bit of code.

(It could be argued that all the implementations of list_interface_ids() 
should be free'ing their own names on failure, but that should be a 
cleanup separate of this work (possibly prior to it?)_


> +    }
> +    return -1;
> +}
> +
> +int drv_list_interfaces(struct netcf *ncf,
> +                        int maxnames, char **names,
> +                        unsigned int flags) {
> +    return list_interface_ids(ncf, maxnames, names, 0, NULL);
> +}
> +
> +
> +int drv_num_of_interfaces(struct netcf *ncf, unsigned int flags) {
> +    return list_interface_ids(ncf, 0, NULL, 0, NULL);
> +}
> +
> +
> +struct netcf_if *drv_lookup_by_name(struct netcf *ncf, const char *name) {
> +    struct netcf_if *nif = NULL;
> +    char *nameDup;
> +    int r = 0;
> +    IP_ADAPTER_ADDRESSES *adapter;
> +
> +    adapter = build_adapter_table(ncf);
> +    ERR_COND_BAIL(adapter == NULL, ncf, EOTHER);
> +
> +    while(adapter) {
> +        if(name) {
> +            char wName[8192];


You need to do something about this excessively large use of the stack. 
Is there a way to predict this size based on the number of bytes in 
adapter->FriendlyName? Even just allocate the same number of bytes, 
knowing that you might waste some?


> +            r = WideCharToMultiByte(CP_UTF8, 0, adapter->FriendlyName,
> +                                    -1, wName, sizeof(wName), NULL, NULL);
> +            ERR_NOMEM(r == 0, ncf);
> +            if(strcmp(name,wName) == 0) {
> +                nameDup = strdup(wName);
> +                ERR_NOMEM(nameDup == NULL, ncf);
> +                nif = make_netcf_if(ncf, nameDup);


Right here is what I was talking about above - you've just strduped 
wName, and then you call make_netcf_if(), which again calls strdup(). 
This is why the strdup() in make_netcf_if() should be removed.


> +                ERR_BAIL(ncf);
> +                return nif;
> +            }
> +        }
> +        adapter = adapter->Next;
> +    }
> +    /* If we get here then the device wasn't found, however,
> +       for cases where we know the device is disabled and
> +       want to re-enable it we have to assume the device is
> +       physically present
> +    */
> +    nameDup = strdup(name);
> +    ERR_NOMEM(nameDup == NULL, ncf);
> +    nif = make_netcf_if(ncf, nameDup);
> +    ERR_BAIL(ncf);
> +    return nif;
> + error:
> +    free(adapter);
> +    if(nameDup)
> +        free(nameDup);
> +    unref(nif, netcf_if);
> +    return nif;
> +}
> +
> +const char *drv_mac_string(struct netcf_if *nif) {
> +    struct netcf *ncf = nif->ncf;
> +    size_t i = 0;
> +    int r = 0;
> +    char mac[BUFSIZE], *buf;


Ah. I've been waiting for this ever since I saw #ifdef BUFSIZE. Can you 
ALLOC_N this instead?


> +
> +    IP_ADAPTER_ADDRESSES *adapter;
> +
> +    adapter = build_adapter_table(ncf);
> +    ERR_COND_BAIL(adapter == NULL, ncf, EOTHER);
> +
> +    while(adapter) {
> +        char wName[8192];


Another excessive use of the stack.


> +        r = WideCharToMultiByte(CP_UTF8, 0, adapter->FriendlyName,
> +                                -1, wName, sizeof(wName), NULL, NULL);
> +        ERR_NOMEM(r == 0, ncf);
> +        if(strcmp(nif->name, wName) == 0) {
> +            for(i = 0; i<  adapter->PhysicalAddressLength; i++) {


There we go. The length of buf is (adapter->PhysicalAddressLength * 3).


> +                if (i == 0) {
> +                    ERR_NOMEM(asprintf(&buf, "%.2X:", adapter->PhysicalAddress[i])<  0, ncf);
> +                    strcpy(mac, buf);
> +                }
> +                if (i == (adapter->PhysicalAddressLength - 1)) {
> +                    ERR_NOMEM(asprintf(&buf, "%.2X", adapter->PhysicalAddress[i])<  0, ncf);
> +                    strcat(mac, buf);
> +                } else {
> +                    ERR_NOMEM(asprintf(&buf, "%.2X:", adapter->PhysicalAddress[i])<  0, ncf);
> +                    strcat(mac, buf);
> +                }
> +                nif->mac = strdup(mac);
> +                ERR_NOMEM(nif->mac == NULL, ncf);


How about something that avoids strcat and asprintf, like:

char *mac = NULL;

if (adapter->PhysicalAddressLength > 0) {
     ERR_NOMEM(ALLOC_N(mac, adapter->PhysicalAddressLength * 3) < 0, ncf);
     char *cur = mac;
     for (ii = 0; ii < adapter->PhysicalAddressLength; ii++) {
         *mac++ = hexdigit(adapter->PhysicalAddress[i] >> 4);
         *mac++ = hexdigit(adapter->PhysicalAddress[i] && 0xf);
         *mac++ = (i < adapter->PhysicalAddressLength - 1) ? ':' : '\0';
     }
}
nif->mac = mac;

(NB: aren't you leaking nif->mac here?)


> +            }
> +            return nif->mac;
> +        }
> +        adapter = adapter->Next;
> +    }
> + error:
> +    free(adapter);
> +    free(buf);
> +    free(mac);
> +    return nif->mac;
> +}
> +
> +int drv_if_down(struct netcf_if *nif) {
> +    struct netcf *ncf = nif->ncf;
> +    char *exe_path;
> +    char *p;
> +    int r = 0;
> +
> +    p = getenv("WINDIR");
> +    r = asprintf(&exe_path, "%s\\system32\\netsh", p);

There are a few places in the test files that use asprintf(), but in 
general we tried to use xasprintf() everywhere, to make error cleanup 
simpler.


> +    ERR_NOMEM(r<  0, ncf);
> +
> +    r = _spawnl(_P_WAIT, exe_path, exe_path, "interface",
> +                "set", "interface", nif->name, "disabled", NULL);


Although it's not really necessary, I was thinking it might be nice to 
have a run_program() function just as in linux, and call that rather 
than calling _spawnl directly.

While I'm on that subject, run_program really should be in 
dutil_linux.c, not in netcf.c. I'll try to remember and fix that.


> +    ERR_COND_BAIL(r != 0, ncf, EEXEC);
> +    return 0;
> + error:
> +    free(exe_path);
> +    return -1;
> +}
> +
> +int drv_if_up(struct netcf_if *nif) {
> +    struct netcf *ncf = nif->ncf;
> +    char *exe_path;
> +    char *p;
> +    int r = 0;
> +
> +    p = getenv("WINDIR");
> +    r = asprintf(&exe_path, "%s\\system32\\netsh", p);
> +    ERR_NOMEM(r<  0, ncf);
> +
> +    r = _spawnl(_P_WAIT, exe_path, exe_path, "interface",
> +                "set", "interface", nif->name, "enabled", NULL);


Again, xasprintf() instead of asprintf(), and make a run_program() 
function for dutil_mswindows.c.


> +    ERR_COND_BAIL(r != 0, ncf, EEXEC);
> +    return 0;
> + error:
> +    free(exe_path);
> +    return -1;
> +}
> +
> diff --git a/src/netcf_win.h b/src/netcf_win.h
> new file mode 100644
> index 0000000..659a1b7
> --- /dev/null
> +++ b/src/netcf_win.h
> @@ -0,0 +1,54 @@
> +/*
> + * 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
> +#endif
> +
> +#include "internal.h"
> +
> +#include<stdbool.h>
> +#include<string.h>
> +#include<windows.h>
> +#include<winsock2.h>
> +#include<ws2tcpip.h>
> +#include<windns.h>
> +#include<iphlpapi.h>
> +#include<process.h>
> +#include "safe-alloc.h"
> +#include "ref.h"
> +#include "list.h"
> +
> +char *strerror_r(int errnum, char *buf, size_t buflen);
> +
> +/* Like asprintf, but set *STRP to NULL on error */
> +ATTRIBUTE_FORMAT(printf, 2, 3)
> +int xasprintf(char **strp, const char *format, ...);
> +
> +/* Create a new netcf if instance for interface NAME */
> +struct netcf_if *make_netcf_if(struct netcf *ncf, char *name);
> +
> +PIP_ADAPTER_ADDRESSES build_adapter_table(struct netcf *ncf);
> +#endif /* NETCF_WIN_H */


Unless you're calling build_adapter_table() from another file (which I 
would think you wouldn't be), you shouldn't list it in the .h.

As a matter of fact, ideally this file should be called 
dutil_mswindows.c, and all the functions that it exports to the 
platform-independent parts of netcf should be listed in internal.h 
(which should be identical for Windows and Linux). Windows utility 
functions should be in dutil_mswindows.c, and prototyped in 
dutil_mswindows.h, and platform_independent utility functions should be 
in dutil.c/h (which will be compiled on all platforms.

If there is anything in dutil.c that doesn't work for Windows, it should 
be moved to dutil_linux.c.



More information about the netcf-devel mailing list