[ofa-general] Re: porting IB management code to Windows

[Sasha Khapyorsky]

Hi Sean,

On 11:18 Thu 11 Dec     , Sean Hefty wrote:
> 
> We've started porting the IB management code (IB-diags at this point) to
> Windows.  My strong preference is to avoid branching the code and instead keep a
> single source code tree.  Is there any objection to accepting changes against
> the management tree to allow the code to run on both Linux and Windows?

Basically I have no objections against porting changes. And I also would
prefer to keep a single code base.

However, I would prefer to minimize amount of needed changes and would
really prefer to not get a lot of limitations in using modern C. I will
comment inline in the patch example below.

> (We can
> figure out the logistics of build related files later.  I'm most concerned about
> the code itself.)
> 
> The patch below gives an example of the changes needed to make this happen.
> Most are a result of compiler differences.
> 
> - Sean
> 
> --- infiniband-diags-1.4.2\src\sminfo.c	2008-10-19 11:34:42.000000000 -0700
> +++ scm\winof\branches\winverbs\tools\infiniband_diags\src\sminfo.c
> 2008-12-10 15:06:01.096000000 -0800
> @@ -37,12 +37,19 @@
>  
>  #include <stdio.h>
>  #include <stdlib.h>
> +
> +#if defined(_WIN32) || defined(_WIN64)
> +#include <windows.h>
> +#include <winsock2.h>
> +#include "..\..\..\..\etc\user\getopt.c"
> +#include "..\ibdiag_common.c"
> +#else
>  #include <unistd.h>
>  #include <stdarg.h>
>  #include <inttypes.h>
>  #include <getopt.h>
> +#endif

Could such ugly header mess be eliminated?

I'm not familiar with windows environment, but would expect that headers
like <stdarg.h> exist there (although I may be wrong about it). Of
course some header file may be missing, this is not so bad - you could
add one somewhere under WinOF tree in the include path, then something
like:

winof/include/path/getopt.h:

#ifndef WINOF_GETOPT_H
#define WINOF_GETOPT_H

#include "..\..\..\..\etc\user\getopt.c"

#endif

could resolve the problem. And similar with another header files (also
AFAIK WinOF is not using autotools, so file config.h could be also good
place for various wrappers).

> -#include <infiniband/common.h>
>  #include <infiniband/umad.h>
>  #include <infiniband/mad.h>
>  
> @@ -72,13 +79,13 @@ enum {
>  };
>  
>  char *statestr[] = {
> -	[SMINFO_NOTACT] "SMINFO_NOTACT",
> -	[SMINFO_DISCOVER] "SMINFO_DISCOVER",
> -	[SMINFO_STANDBY] "SMINFO_STANDBY",
> -	[SMINFO_MASTER] "SMINFO_MASTER",
> +	"SMINFO_NOTACT",
> +	"SMINFO_DISCOVER",
> +	"SMINFO_STANDBY",
> +	"SMINFO_MASTER",
>  };

Could VC++ understand C99 like initializations (maybe with using some
flags)? I would really prefer to use something like this.

>  
> -#define STATESTR(s)	(((uint)(s)) < SMINFO_STATE_LAST ? statestr[s] : "???")
> +#define STATESTR(s)	(((unsigned int)(s)) < SMINFO_STATE_LAST ? statestr[s] :
> "???")
>  
>  int
>  main(int argc, char **argv)
> @@ -88,7 +95,7 @@ main(int argc, char **argv)
>  	ib_portid_t portid = {0};
>  	int timeout = 0;	/* use default */
>  	uint8_t *p;
> -	uint act = 0;
> +	unsigned int act = 0;

All 'uint' -> 'unsigned int' conversions seem fine for me (I think we
need to do this even w/out connection to WinOF porting issue).

>  	int prio = 0, state = SMINFO_STANDBY;
>  	uint64_t guid = 0, key = 0;
>  	extern int ibdebug;
> @@ -97,8 +104,8 @@ main(int argc, char **argv)
>  	char *ca = 0;
>  	int ca_port = 0;
>  
> -	static char const str_opts[] = "C:P:t:s:p:a:deDGVhu";
> -	static const struct option long_opts[] = {
> +	static char str_opts[] = "C:P:t:s:p:a:deDGVhu";
> +	static struct option long_opts[] = {

I saw in your another email that 'const' issue could be solved (worst
case it could be masked in WinOF config.h - #define const ). Right?

>  		{ "C", 1, 0, 'C'},
>  		{ "P", 1, 0, 'P'},
>  		{ "debug", 0, 0, 'd'},
> @@ -112,7 +119,7 @@ main(int argc, char **argv)
>  		{ "timeout", 1, 0, 't'},
>  		{ "help", 0, 0, 'h'},
>  		{ "usage", 0, 0, 'u'},
> -		{ }
> +		{ 0 }

Could VC be learned with some flags to understand {}? Basically we could
except such change, but it will be hard to remember to follow this rule on
linux side :)

>  	};
>  
>  	argv0 = argv[0];
> @@ -188,7 +195,7 @@ main(int argc, char **argv)
>  
>  	if (mod) {
>  		if (!(p = smp_set(sminfo, &portid, IB_ATTR_SMINFO, mod,
> timeout)))
> -			IBERROR("query");
> +			IBERROR("set");

This is fine (and guess is not related to porting issue :))

Sasha

>  	} else
>  		if (!(p = smp_query(sminfo, &portid, IB_ATTR_SMINFO, 0,
> timeout)))
>  			IBERROR("query");
> 
>