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

Laine Stump laine at laine.org
Thu Sep 23 15:06:22 UTC 2010


  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.




More information about the netcf-devel mailing list