[openib-general] [PATCH 7/13] osm: port to WinIB stack : opensm/osm_ucast_file.c

[Sasha Khapyorsky]

Hi Eitan,

Some more comments...

On 18:59 Sun 17 Sep     , Eitan Zahavi wrote:
> Hi Hal
> 
> 1. Avoid varargs macros not supported by win
> 2. Some explicit casting required
> 3. Use stroull and not stroll
> 
> Thanks
> 
> Eitan
> 
> Signed-off-by:  Eitan Zahavi <eitan at mellanox.co.il>
> 
> Index: opensm/osm_ucast_file.c
> ===================================================================
> --- opensm/osm_ucast_file.c	(revision 9502)
> +++ opensm/osm_ucast_file.c	(working copy)
> @@ -52,18 +52,11 @@
>  
>  #include <iba/ib_types.h>
>  #include <complib/cl_qmap.h>
> +#include <complib/cl_debug.h>

Why this?

>  #include <opensm/osm_opensm.h>
>  #include <opensm/osm_switch.h>
>  #include <opensm/osm_log.h>
>  
> -#define PARSEERR(log, file_name, lineno, fmt, arg...) \
> -		osm_log(log, OSM_LOG_ERROR, "PARSE ERROR: %s:%u: " fmt , \
> -			file_name, lineno, ##arg )
> -
> -#define PARSEWARN(log, file_name, lineno, fmt, arg...) \
> -		osm_log(log, OSM_LOG_VERBOSE, "PARSE WARN: %s:%u: " fmt , \
> -			file_name, lineno, ##arg )
> -

Is it possible to use C99 style var args macros (with __VA_ARGS___)? MS
claims it is supported by VC. And it is supported by gcc too.

>  static uint16_t remap_lid(osm_opensm_t *p_osm, uint16_t lid, ib_net64_t guid)
>  {
>  	osm_port_t *p_port;
> @@ -72,10 +65,11 @@ static uint16_t remap_lid(osm_opensm_t *
>  
>  	p_port = (osm_port_t *)cl_qmap_get(&p_osm->subn.port_guid_tbl, guid);
>  	if (!p_port ||
> -	    p_port == (osm_port_t *)cl_qmap_end(&p_osm->subn.port_guid_tbl)) {
> +	    p_port == (osm_port_t *)cl_qmap_end(&p_osm->subn.port_guid_tbl)) 
> +	{

Please don't break existing code formatting.

>  		osm_log(&p_osm->log, OSM_LOG_VERBOSE,
> -			"remap_lid: cannot find port guid 0x%016" PRIx64
> -			" , will use the same lid\n", cl_ntoh64(guid));
> +				  "remap_lid: cannot find port guid 0x%016" PRIx64
> +				  " , will use the same lid\n", cl_ntoh64(guid));
>  		return lid;
>  	}
>  
> @@ -182,19 +176,21 @@ static int do_ucast_file_load(void *cont
>  				"skipping parsing. Using default routing algorithm\n");
>  
>  		}
> +

Ditto.

>  		else if (!strncmp(p, "Unicast lids", 12)) {
>  			q = strstr(p, " guid 0x");
>  			if (!q) {
> -				PARSEERR(&p_osm->log, file_name, lineno,
> -					 "cannot parse switch definition\n");
> +				osm_log(&p_osm->log, OSM_LOG_ERROR, "PARSE ERROR: %s:%u:" 
> +						  " cannot parse switch definition\n", 
> +						  file_name, lineno);
>  				return -1;
>  			}
>  			p = q + 6;
> -			sw_guid = strtoll(p, &q, 16);
> +			sw_guid = strtoull(p, &q, 16);

Good.

>  			if (q && !isspace(*q)) {
> -				PARSEERR(&p_osm->log, file_name, lineno,
> -					 "cannot parse switch guid: \'%s\'\n",
> -					 p);
> +				osm_log(&p_osm->log, OSM_LOG_ERROR, "PARSE ERROR: %s:%u:" 
> +						  "cannot parse switch guid: \'%s\'\n",
> +						  file_name, lineno, p);
>  				return -1;
>  			}
>  			sw_guid = cl_hton64(sw_guid);
> @@ -212,40 +208,39 @@ static int do_ucast_file_load(void *cont
>  			}
>  		}
>  		else if (p_sw && !strncmp(p, "0x", 2)) {
> -			lid = strtoul(p, &q, 16);
> +			lid = (uint16_t)strtoul(p, &q, 16);
>  			if (q && !isspace(*q)) {
> -				PARSEERR(&p_osm->log, file_name, lineno,
> -					 "cannot parse lid: \'%s\'\n", p);
> +				osm_log(&p_osm->log, OSM_LOG_ERROR, "PARSE ERROR: %s:%u:"
> +						  "cannot parse lid: \'%s\'\n", file_name, lineno, p);
>  				return -1;
>  			}
>  			p = q;
>  			while (isspace(*p))
>  				p++;
> -			port_num = strtoul(p, &q, 10);
> +			port_num = (uint8_t)strtoul(p, &q, 10);
>  			if (q && !isspace(*q)) {
> -				PARSEERR(&p_osm->log, file_name, lineno,
> -					 "cannot parse port: \'%s\'\n", p);
> +				osm_log(&p_osm->log, OSM_LOG_ERROR, "PARSE ERROR: %s:%u:" 
> +						  "cannot parse port: \'%s\'\n", file_name, lineno, p);
>  				return -1;
>  			}
>  			p = q;
>  			/* additionally try to exract guid */
>  			q = strstr(p, " portguid 0x");
>  			if (!q) {
> -				PARSEWARN(&p_osm->log, file_name, lineno,
> -					  "cannot find port guid "
> -					  "(maybe broken dump): \'%s\'\n", p);
> +				osm_log(&p_osm->log, OSM_LOG_VERBOSE, "PARSE WARNING: %s:%u:" 
> +						  "cannot find port guid "
> +						  "(maybe broken dump): \'%s\'\n", file_name, lineno, p);
>  				port_guid = 0;
>  			}
>  			else
>  			{
>  				p = q + 10;
> -				port_guid = strtoll(p, &q, 16);
> +				port_guid = strtoull(p, &q, 16);

Good.

Sasha

>  				if (!q && !isspace(*q) && *q != ':') {
> -					PARSEWARN(&p_osm->log, file_name,
> -						  lineno,
> -						  "cannot parse port guid "
> -						  "(maybe broken dump): "
> -						  "\'%s\'\n", p);
> +					osm_log(&p_osm->log, OSM_LOG_VERBOSE, "PARSE WARNING: %s:%u:" 
> +							  "cannot parse port guid "
> +							  "(maybe broken dump): "
> +							  "\'%s\'\n", file_name, lineno, p);
>  					port_guid = 0;
>  				}
>  			}
> 
> 
> 
> _______________________________________________
> openib-general mailing list
> openib-general at openib.org
> http://openib.org/mailman/listinfo/openib-general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
>