On 09/22/2010 04:17 PM, Eric Blake wrote:
> 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.
Until proven otherwise, I'm convinced that the failure of ncf_init()
isn't benign, but is an indicator that something about the memory
management is seriously wrong, and prone to blowing up in our faces. I'd
like to get to the root of this problem before the code is made easily
usable by anyone else (ie before pushing it if at all possible).
> 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?
Probably on 99% of systems, but definitely not always - I've quite often
had Windows setups where the root was D: or E: (multibooters mostly). To
be safe, you should get the value of the environment variable
"SystemRoot" (I think) and use that.
> 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?
Actually, the intent of the entire if else (partially cut off in the
diffs) is to append a "/" to the root string if there isn't one there.
In Windows, that should be appending a "\" if there isn't one there. So
I think a #define PATH_COMPONENT_SEPARATOR (or something like that) is
in order. It can be up there with the #define ROOT.
> +#ifdef WIN32
> +int ncf_lookup_by_mac_string() { return -1; }
> +#else
> +#ifdef WIN32
> +/* No mingw implementation */
> +int ncf_if_undefine() { return -1; };
> +#ifdef WIN32
> +/* No mingw implementation */
> +char *ncf_if_xml_desc() { return NULL; }
> +/* No mingw implementation */
> +char *ncf_if_xml_state() { return NULL; }
> +#ifdef WIN32
> +/* No mingw implementation */
> +int ncf_if_status() { return -1; }
So this is definitely a preliminary implementation, since those are all
fairly important. Just how much functionality do you need for your
application?
> +#ifdef WIN32
> +/* No mingw implementation */
> +int ncf_get_aug() { return -1; }
> +#else
ncf_get_aug/ncf_put_aug should never even be called in the Windows
version. It would probably be better to #ifdef out their definitions in
the .c and .h file, and not even build test-initscripts.c and
ncftransform.c (which are really intended for testing the initscripts
version of the netcf backend, so don't apply to the Windows version).
> +#ifndef WIN32
> +/* Do not see any other way around this as there is no
> + * portable option for strerror_r or pthread_sigmask
> + */
> /*
> * Internal helpers
> */
#ifdef'ing these (exec_program and run_program) out is fine as long as
the Windows version doesn't require exec'ing any external program in
order to perform any of the tasks. The Linux version uses run_program to
call the ifup and ifdown scripts to bring interfaces up and down.
(And really these functions should probably be in dutil_linux.c, so the
ugliness of the #ifdef isn't your fault ;-)
> @@ -395,6 +446,7 @@ error:
> return ret;
> }
>
> +#endif /* WIN32 */
> /*
> * argv_to_string() is borrowed from libvirt's
> * src/util.c:virArgvToString()
argv_to_string() also is only called from run_program, so it can be part
of the #ifndef block directly above.