[ofa-general] Re: [PATCH 2/6] [ib-diag] ibroute: add support for WinOF

Sasha Khapyorsky sashak at voltaire.com
Thu Feb 26 13:02:19 PST 2009


On 12:07 Thu 26 Feb     , Sean Hefty wrote:
> Both of your patches (ibnetdiscover and ibroute) build on winof.  I replaced my
> 2 patches with yours, updated to the latest codebase, and pushed everything:
> 
> git://git.openfabrics.org/~shefty/ib-mgmt.git master
> 
> Were there changes to the other patches that you wanted (including saquery,
> which wasn't part of the numbered series)?

Thanks. I applied everything except ibsysstat.c and saquery.c. Wanted to
clarify some things there:

> From 49f28a63589be21dd7218922ed9d0b2b719a92c2 Mon Sep 17 00:00:00 2001
> From: Sean Hefty <sean.hefty at intel.com>
> Date: Thu, 26 Feb 2009 10:12:07 -0800
> Subject: [PATCH 1/2] [ib-diag] ibsysstat: add support for WinOF
> 
> Signed-off-by: Sean Hefty <sean.hefty at intel.com>
> ---
>  infiniband-diags/src/ibsysstat.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/infiniband-diags/src/ibsysstat.c b/infiniband-diags/src/ibsysstat.c
> index cc1418d..b9f2f85 100644
> --- a/infiniband-diags/src/ibsysstat.c
> +++ b/infiniband-diags/src/ibsysstat.c
> @@ -183,7 +183,7 @@ static char *ibsystat_serv(void)
>  
>  		DEBUG("got packet: attr 0x%x mod 0x%x", attr, mod);
>  
> -		size = mk_reply(attr, mad + IB_VENDOR_RANGE2_DATA_OFFS,
> +		size = mk_reply(attr, (char *) mad + IB_VENDOR_RANGE2_DATA_OFFS,

What is the reason for such void * to char * casting?

>  				sizeof(buf) - umad_size() - IB_VENDOR_RANGE2_DATA_OFFS);
>  
>  		if (server_respond(umad, IB_VENDOR_RANGE2_DATA_OFFS + size) < 0)
> @@ -210,7 +210,7 @@ static char *ibsystat(ib_portid_t *portid, int attr)
>  {
>  	ib_rpc_t rpc = { 0 };
>  	int fd, agent, timeout, len;
> -	void *data = umad_get_mad(buf) + IB_VENDOR_RANGE2_DATA_OFFS;
> +	void *data = (char *) umad_get_mad(buf) + IB_VENDOR_RANGE2_DATA_OFFS;

Ditto.

>  
>  	DEBUG("Sysstat ping..");
>  
> @@ -318,7 +318,7 @@ int main(int argc, char **argv)
>  	const struct ibdiag_opt opts[] = {
>  		{ "oui", 'o', 1, NULL, "use specified OUI number" },
>  		{ "Server", 'S', 0, NULL, "start in server mode" },
> -		{ }
> +		{ 0 }
>  	};
>  	char usage_args[] = "<dest lid|guid> [<op>]";
>  
> -- 
> 1.6.1.2.319.gbd9e
> 
> 
> From 1b9685769339891670df6d9af66e9933794be8a0 Mon Sep 17 00:00:00 2001
> From: Sean Hefty <sean.hefty at intel.com>
> Date: Thu, 26 Feb 2009 10:12:29 -0800
> Subject: [PATCH 2/2] [ib-diag] saquery: add support for WinOF
> 
> Signed-off-by: Sean Hefty <sean.hefty at intel.com>
> ---
>  infiniband-diags/src/saquery.c |   80 ++++++++++++++++++++++------------------
>  1 files changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/infiniband-diags/src/saquery.c b/infiniband-diags/src/saquery.c
> index bcd1f61..4a5cfb8 100644
> --- a/infiniband-diags/src/saquery.c
> +++ b/infiniband-diags/src/saquery.c
> @@ -37,20 +37,25 @@
>   *
>   */
>  
> +#if HAVE_CONFIG_H
> +#  include <config.h>
> +#endif /* HAVE_CONFIG_H */
> +
>  #include <unistd.h>
>  #include <stdio.h>
>  #include <arpa/inet.h>
>  #include <ctype.h>
>  #include <string.h>
>  #include <errno.h>
> +#include <assert.h>
>  
>  #define _GNU_SOURCE
>  #include <getopt.h>
>  
>  #include <infiniband/umad.h>
>  #include <infiniband/mad.h>
> -#include <infiniband/iba/ib_types.h>
> -#include <infiniband/complib/cl_nodenamemap.h>
> +#include <iba/ib_types.h>
> +#include <complib/cl_nodenamemap.h>
>  
>  #include "ibdiag_common.h"
>  
> @@ -170,7 +175,7 @@ recv_mad:
>  	if (ibdebug > 1)
>  		xdump(stdout, "SA Response:\n", mad, len);
>  
> -	method = mad_get_field(mad, 0, IB_MAD_METHOD_F);
> +	method = (uint8_t) mad_get_field(mad, 0, IB_MAD_METHOD_F);
>  	offset = mad_get_field(mad, 0, IB_SA_ATTROFFS_F);
>  	result.status = mad_get_field(mad, 0, IB_MAD_STATUS_F);
>  	result.p_result_madw = mad;
> @@ -189,12 +194,12 @@ recv_mad:
>  static void *get_query_rec(void *mad, unsigned i)
>  {
>  	int offset = mad_get_field(mad, 0, IB_SA_ATTROFFS_F);
> -	return mad + IB_SA_DATA_OFFS + i * (offset << 3);
> +	return (char *) mad + IB_SA_DATA_OFFS + i * (offset << 3);

Ditto.

>  }
>  
>  static unsigned valid_gid(ib_gid_t *gid)
>  {
> -	ib_gid_t zero_gid = { };
> +	ib_gid_t zero_gid = { 0 };
>  	return memcmp(&zero_gid, gid, sizeof(*gid));
>  }
>  
> @@ -442,7 +447,7 @@ static void dump_multicast_member_record(void *data)
>  	char gid_str2[INET6_ADDRSTRLEN];
>  	ib_member_rec_t *p_mcmr = data;
>  	uint16_t mlid = cl_ntoh16(p_mcmr->mlid);
> -	int i = 0;
> +	unsigned i = 0;
>  	char *node_name = "<unknown>";
>  
>  	/* go through the node records searching for a port guid which matches
> @@ -758,7 +763,7 @@ static void dump_one_mft_record(void *data)
>  
>  static void dump_results(struct query_res *r, void (*dump_func) (void *))
>  {
> -	int i;
> +	unsigned i;
>  	for (i = 0; i < r->result_cnt; i++) {
>  		void *data = get_query_rec(r->p_result_madw, i);
>  		dump_func(data);
> @@ -768,7 +773,7 @@ static void dump_results(struct query_res *r, void (*dump_func) (void *))
>  static void return_mad(void)
>  {
>  	if (result.p_result_madw) {
> -		free(result.p_result_madw - umad_size());
> +		free((char *) result.p_result_madw - umad_size());

Ditto.

>  		result.p_result_madw = NULL;
>  	}
>  }
> @@ -839,7 +844,8 @@ get_lid_from_name(bind_handle_t h, const char *name, uint16_t* lid)
>  {
>  	ib_node_record_t *node_record = NULL;
>  	ib_node_info_t *p_ni = NULL;
> -	int i = 0, ret;
> +	unsigned i;
> +	int ret;
>  
>  	ret = get_all_records(h, IB_SA_ATTR_NODERECORD, 0);
>  	if (ret)
> @@ -869,7 +875,7 @@ static uint16_t get_lid(bind_handle_t h, const char *name)
>  	if (isalpha(name[0]))
>  		assert(get_lid_from_name(h, name, &rc_lid) == IB_SUCCESS);
>  	else
> -		rc_lid = atoi(name);
> +		rc_lid = (uint16_t) atoi(name);
>  	if (rc_lid == 0)
>  		fprintf(stderr, "Failed to find lid for \"%s\"\n", name);
>  	return rc_lid;
> @@ -917,8 +923,8 @@ static int parse_lid_and_ports(bind_handle_t h,
>  
>  #define cl_hton8(x) (x)
>  #define CHECK_AND_SET_VAL(val, size, comp_with, target, name, mask) \
> -	if (val > comp_with) { \
> -		target = cl_hton##size(val); \
> +	if ((uint##size##_t) val > (uint##size##_t) comp_with) { \
> +		target = cl_hton##size((uint##size##_t) val); \
>  		comp_mask |= IB_##name##_COMPMASK_##mask; \
>  	}
>  
> @@ -951,7 +957,8 @@ static int get_issm_records(bind_handle_t h, ib_net32_t capability_mask)
>  
>  static int print_node_records(bind_handle_t h)
>  {
> -	int i = 0, ret;
> +	unsigned i;
> +	int ret;
>  
>  	ret = get_all_records(h, IB_SA_ATTR_NODERECORD, 0);
>  	if (ret)
> @@ -1027,7 +1034,7 @@ static int query_path_records(const struct query_cmd *q, bind_handle_t h,
>  	CHECK_AND_SET_VAL(p->dlid, 16, 0, pr.dlid, PR, DLID);
>  	CHECK_AND_SET_VAL(p->hop_limit, 32, -1, pr.hop_flow_raw, PR, HOPLIMIT);
>  	CHECK_AND_SET_VAL(p->flow_label, 8, 0, flow, PR, FLOWLABEL);
> -	pr.hop_flow_raw |= cl_hton32(flow << 8);
> +	pr.hop_flow_raw |= (uint8_t) cl_hton32(flow << 8);

Why this casting is needed? This should be uint32_t to uint32_t
assignment, no?

>  	CHECK_AND_SET_VAL(p->tclass, 8, 0, pr.tclass, PR, TCLASS);
>  	CHECK_AND_SET_VAL(p->reversible, 8, -1, reversible, PR, REVERSIBLE);
>  	CHECK_AND_SET_VAL(p->numb_path, 8, -1, pr.num_path, PR, NUMBPATH);
> @@ -1089,7 +1096,7 @@ static int print_multicast_member_records(bind_handle_t h)
>  
>  return_mc:
>  	if (mc_group_result.p_result_madw)
> -		free(mc_group_result.p_result_madw - umad_size());
> +		free((char *) mc_group_result.p_result_madw - umad_size());

void * to char * casting again.

>  
>  	return ret;
>  }
> @@ -1267,7 +1274,7 @@ static int query_pkey_tbl_records(const struct query_cmd *q,
>  	memset(&pktr, 0, sizeof(pktr));
>  	CHECK_AND_SET_VAL(lid, 16, 0, pktr.lid, PKEY, LID);
>  	CHECK_AND_SET_VAL(port, 8, -1, pktr.port_num, PKEY, PORT);
> -	CHECK_AND_SET_VAL(block, 16, -1, pktr.port_num, PKEY, BLOCK);
> +	CHECK_AND_SET_VAL(block, 16, -1, pktr.block_num, PKEY, BLOCK);

This fix is unrelated to porting, right?

The rest looks fine for me.

Sasha

>  
>  	return get_and_dump_any_records(h, IB_SA_ATTR_PKEYTABLERECORD, 0,
>  					comp_mask, &pktr, smkey,
> @@ -1503,13 +1510,13 @@ static int process_opt(void *context, int ch, char *optarg)
>  		query_type = IB_SA_ATTR_LINKRECORD;
>  		break;
>  	case 5:
> -		p->slid = strtoul(optarg, NULL, 0);
> +		p->slid = (uint16_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 6:
> -		p->dlid = strtoul(optarg, NULL, 0);
> +		p->dlid = (uint16_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 7:
> -		p->mlid = strtoul(optarg, NULL, 0);
> +		p->mlid = (uint16_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 14:
>  		if (inet_pton(AF_INET6, optarg, &p->sgid) <= 0)
> @@ -1534,7 +1541,7 @@ static int process_opt(void *context, int ch, char *optarg)
>  		p->numb_path = strtoul(optarg, NULL, 0);
>  		break;
>  	case 18:
> -		p->pkey = strtoul(optarg, NULL, 0);
> +		p->pkey = (uint16_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 'Q':
>  		p->qos_class = strtoul(optarg, NULL, 0);
> @@ -1543,19 +1550,19 @@ static int process_opt(void *context, int ch, char *optarg)
>  		p->sl = strtoul(optarg, NULL, 0);
>  		break;
>  	case 'M':
> -		p->mtu = strtoul(optarg, NULL, 0);
> +		p->mtu = (uint8_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 'R':
> -		p->rate = strtoul(optarg, NULL, 0);
> +		p->rate = (uint8_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 20:
> -		p->pkt_life = strtoul(optarg, NULL, 0);
> +		p->pkt_life = (uint8_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 'q':
>  		p->qkey = strtoul(optarg, NULL, 0);
>  		break;
>  	case 'T':
> -		p->tclass = strtoul(optarg, NULL, 0);
> +		p->tclass = (uint8_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 'F':
>  		p->flow_label = strtoul(optarg, NULL, 0);
> @@ -1564,10 +1571,10 @@ static int process_opt(void *context, int ch, char *optarg)
>  		p->hop_limit = strtoul(optarg, NULL, 0);
>  		break;
>  	case 21:
> -		p->scope = strtoul(optarg, NULL, 0);
> +		p->scope = (uint8_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 'J':
> -		p->join_state = strtoul(optarg, NULL, 0);
> +		p->join_state = (uint8_t) strtoul(optarg, NULL, 0);
>  		break;
>  	case 'X':
>  		p->proxy_join = strtoul(optarg, NULL, 0);
> @@ -1582,14 +1589,7 @@ int main(int argc, char **argv)
>  {
>  	char usage_args[1024];
>  	bind_handle_t h;
> -	struct query_params params = {
> -		.hop_limit = -1,
> -		.reversible = -1,
> -		.numb_path = -1,
> -		.qos_class = -1,
> -		.sl = -1,
> -		.proxy_join = -1,
> -	};
> +	struct query_params params;
>  	const struct query_cmd *q;
>  	ib_api_status_t status;
>  	int n;
> @@ -1643,9 +1643,17 @@ int main(int argc, char **argv)
>  		{ "scope", 21, 1, NULL, "Scope (MCMemberRecord)" },
>  		{ "join_state", 'J', 1, NULL, "Join state (MCMemberRecord)" },
>  		{ "proxy_join", 'X', 1, NULL, "Proxy join (MCMemberRecord)" },
> -		{}
> +		{ 0 }
>  	};
>  
> +	memset(&params, 0, sizeof params);
> +	params.hop_limit = -1;
> +	params.reversible = -1;
> +	params.numb_path = -1;
> +	params.qos_class = -1;
> +	params.sl = -1;
> +	params.proxy_join = -1;
> +
>  	n = sprintf(usage_args, "[query-name] [<name> | <lid> | <guid>]\n"
>  		    "\nSupported query names (and aliases):\n");
>  	for (q = query_cmds; q->name; q++) {
> @@ -1680,7 +1688,7 @@ int main(int argc, char **argv)
>  
>  	if (argc) {
>  		if (node_print_desc == NAME_OF_LID) {
> -			requested_lid = strtoul(argv[0], NULL, 0);
> +			requested_lid = (uint16_t) strtoul(argv[0], NULL, 0);
>  			requested_lid_flag++;
>  		} else if (node_print_desc == NAME_OF_GUID) {
>  			requested_guid = strtoul(argv[0], NULL, 0);
> -- 
> 1.6.1.2.319.gbd9e
> 



More information about the general mailing list