[netcf-devel] [PATCH 5/5] Conditionalize Windows portion

Eric Blake eblake at redhat.com
Wed Sep 22 20:17:48 UTC 2010


On 09/17/2010 10:29 AM, Adam Stokes wrote:
> ---
>   src/drv_initscripts.c |    2 +-
>   src/internal.h        |    3 ++
>   src/ncftool.c         |    7 ++++++
>   src/netcf.c           |   54 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   src/xslt_ext.c        |    2 +-
>   5 files changed, 65 insertions(+), 3 deletions(-)
>
> diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
> index c0bcfd5..2127d2a 100644
> --- a/src/drv_initscripts.c
> +++ b/src/drv_initscripts.c
> @@ -100,7 +100,7 @@ static const struct augeas_xfm_table augeas_xfm_iptables =
>
>   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";

I still don't see why you had to do this rename.  A commit comment 
explaining why would be great, and it may be possible to do things 
without a rename.

>
>   static const char *const prog_rc_d_iptables = "/etc/init.d/iptables";
>
> diff --git a/src/internal.h b/src/internal.h
> index d06665d..1b1726c 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -83,6 +83,9 @@
>   #define ATTRIBUTE_NOINLINE
>   #endif                                   /* __GNUC__ */
>
> +#ifndef WIN32
> +#define DATADIR NETCF_DATADIR
> +#endif
>   /* This needs ATTRIBUTE_RETURN_CHECK */
>   #include "ref.h"
>
> diff --git a/src/ncftool.c b/src/ncftool.c
> index 2cb139d..6cdbdef 100644
> --- a/src/ncftool.c
> +++ b/src/ncftool.c
> @@ -677,7 +677,11 @@ static void parse_opts(int argc, char **argv) {
>       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

NACK to this hunk; I'd rather get gnulib to relax the 'setenv' module to 
LGPLv2+ and rely on that (I've just sent an email to the gnulib list 
requesting that).

>               break;
>           case 'h':
>               usage();
> @@ -784,7 +788,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

It would be nicer to find out why before this commit goes in.  But even 
if we don't figure out why, this reads awkwardly to me.  How about:


#ifdef WIN32
   // FIXME: ncf_init is always failing on WIN32, but we can proceed
   // anyways.  Find out why and simplify this code.
#else
   exit(EXIT_FAILURE);
#endif

>       }
>       readline_init();
>       if (optind<  argc) {
> diff --git a/src/netcf.c b/src/netcf.c
> index 1a95332..9ec6dea 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"
> +#else
>   #include "dutil.h"
> +#endif
>
>   /* Clear error code and details */
>   #define API_ENTRY(ncf)                          \
> @@ -88,7 +92,11 @@ int ncf_init(struct netcf **ncf, const char *root) {
>       if (make_ref(*ncf)<  0)
>           goto oom;
>       if (root == NULL)
> +#ifdef WIN32
> +        root = "c:\\";
> +#else
>           root = "/";
> +#endif

Rather than having an in-function #ifdef, it would be nicer to have a 
#define earlier in the file:

#ifdef WIN32
# define ROOT "c:\\"
#else
# define ROOT "/"
#endif

Also, is the C: drive always the right root?

>       if (root[strlen(root)-1] == '/') {
>           (*ncf)->root = strdup(root);

This code is skipped given your WIN32 definition of root; but your root 
is still absolute.  Is that a problem?

>       } else {
> @@ -99,9 +107,14 @@ int ncf_init(struct netcf **ncf, const char *root) {
>           goto oom;
>       (*ncf)->data_dir = getenv("NETCF_DATADIR");
>       if ((*ncf)->data_dir == NULL)
> -        (*ncf)->data_dir = DATADIR "/netcf";
> +        (*ncf)->data_dir = NETCF_DATADIR "/netcf";
>       (*ncf)->debug = getenv("NETCF_DEBUG") != NULL;
> +#ifdef WIN32
> +    return 0;
> +#else
> +    /* Needs investigation on WIN32 */

Move this comment into the WIN32 block.

>       return drv_init(*ncf);
> +#endif
>    oom:
>       ncf_close(*ncf);
>       *ncf = NULL;
> @@ -151,12 +164,16 @@ struct netcf_if * ncf_lookup_by_name(struct netcf *ncf, const char *name) {
>       return drv_lookup_by_name(ncf, name);
>   }
>
> +#ifdef WIN32
> +int ncf_lookup_by_mac_string() { return -1; }
> +#else
>   int
>   ncf_lookup_by_mac_string(struct netcf *ncf, const char *mac,
>                            int maxifaces, struct netcf_if **ifaces) {
>       API_ENTRY(ncf);
>       return drv_lookup_by_mac_string(ncf, mac, maxifaces, ifaces);
>   }
> +#endif
>
>   /*
>    * Define/start/stop/undefine interfaces
> @@ -180,10 +197,15 @@ const char *ncf_if_mac_string(struct netcf_if *nif) {
>   }
>
>   /* Delete the definition */
> +#ifdef WIN32
> +/* No mingw implementation */
> +int ncf_if_undefine() { return -1; };
> +#else
>   int ncf_if_undefine(struct netcf_if *nif) {
>       API_ENTRY(nif->ncf);
>       return drv_undefine(nif);
>   }
> +#endif
>
>   /* Bring the interface up */
>   int ncf_if_up(struct netcf_if *nif) {
> @@ -202,28 +224,43 @@ int ncf_if_down(struct netcf_if *nif) {
>   /* Produce an XML description for the interface, in the same format that
>    * NCF_DEFINE expects
>    */
> +#ifdef WIN32
> +/* No mingw implementation */
> +char *ncf_if_xml_desc() { return NULL; }
> +#else
>   char *ncf_if_xml_desc(struct netcf_if *nif) {
>       API_ENTRY(nif->ncf);
>       return drv_xml_desc(nif);
>   }
> +#endif
>
>   /* Produce an XML description of the current live state of the
>    * interface, in the same format that NCF_DEFINE expects, but
>    * potentially with extra info not contained in the static config (ie
>    * the current IP address of an interface that uses DHCP)
>    */
> +#ifdef WIN32
> +/* No mingw implementation */
> +char *ncf_if_xml_state() { return NULL; }
> +#else
>   char *ncf_if_xml_state(struct netcf_if *nif) {
>       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
>    */
> +#ifdef WIN32
> +/* No mingw implementation */
> +int ncf_if_status() { return -1; }
> +#else
>   int ncf_if_status(struct netcf_if *nif, unsigned int *flags) {
>       API_ENTRY(nif->ncf);
>       return drv_if_status(nif, flags);
>   }
> +#endif
>
>   /* Release any resources used by this NETCF_IF; the pointer is invalid
>    * after this call
> @@ -250,18 +287,32 @@ int ncf_error(struct netcf *ncf, const char **errmsg, const char **details) {
>   /*
>    * Test interface
>    */
> +#ifdef WIN32
> +/* No mingw implementation */
> +int ncf_get_aug() { return -1; }
> +#else
>   int ncf_get_aug(struct netcf *ncf, const char *ncf_xml, char **aug_xml) {
>       API_ENTRY(ncf);
>
>       return drv_get_aug(ncf, ncf_xml, aug_xml);
>   }
> +#endif
>
> +#ifdef WIN32
> +/* No mingw implementation */
> +int ncf_put_aug() { return -1; }
> +#else
>   int ncf_put_aug(struct netcf *ncf, const char *aug_xml, char **ncf_xml) {
>       API_ENTRY(ncf);
>
>       return drv_put_aug(ncf, aug_xml, ncf_xml);
>   }
> +#endif
>
> +#ifndef WIN32
> +/* Do not see any other way around this as there is no
> + * portable option for strerror_r or pthread_sigmask
> + */
>   /*
>    * Internal helpers
>    */
> @@ -395,6 +446,7 @@ error:
>       return ret;
>   }
>
> +#endif /* WIN32 */
>   /*
>    * argv_to_string() is borrowed from libvirt's
>    * src/util.c:virArgvToString()
> diff --git a/src/xslt_ext.c b/src/xslt_ext.c
> index 07e2b41..444889a 100644
> --- a/src/xslt_ext.c
> +++ b/src/xslt_ext.c
> @@ -106,7 +106,7 @@ static void ipcalc_prefix(xmlXPathParserContextPtr ctxt, int nargs) {
>           goto error;
>       }
>
> -    r = inet_aton((char *) netmask_str,&netmask);
> +    r = inet_pton(AF_INET, (char *) netmask_str,&netmask);

This hunk doesn't seem like it fits with the rest of the patch.  Is it 
an independent bug fix?

>       if (r<  0) {
>           xsltTransformError(xsltXPathGetTransformContext(ctxt), NULL, NULL,
>                              "ipcalc:prefix: illegal netmask '%s'",

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


More information about the netcf-devel mailing list