[ofa-general] Re: [PATCH 4/7] Change node name map implementation to use qmap in memory storage

Sasha Khapyorsky sashak at voltaire.com
Sun Nov 4 12:34:12 PST 2007


Hi Ira,

On 20:15 Thu 01 Nov     , Ira Weiny wrote:
> From 2dacfc928856351820fadc416da787350254419e Mon Sep 17 00:00:00 2001
> From: Ira K. Weiny <weiny2 at llnl.gov>
> Date: Thu, 1 Nov 2007 19:29:02 -0700
> Subject: [PATCH] Change node name map implementation to use qmap in memory storage
> 
> Signed-off-by: Ira K. Weiny <weiny2 at llnl.gov>
> ---
>  infiniband-diags/src/ibnetdiscover.c    |   18 +++---
>  infiniband-diags/src/ibroute.c          |    1 +
>  infiniband-diags/src/ibtracert.c        |   16 ++--
>  infiniband-diags/src/saquery.c          |   12 ++--
>  infiniband-diags/src/smpquery.c         |   12 ++--
>  opensm/complib/cl_nodenamemap.c         |  116 ++++++++++++++++++++----------
>  opensm/include/complib/cl_nodenamemap.h |   19 ++++-
>  7 files changed, 122 insertions(+), 72 deletions(-)
> 
> diff --git a/infiniband-diags/src/ibnetdiscover.c b/infiniband-diags/src/ibnetdiscover.c
> index 03ef6f9..8b229c1 100644
> --- a/infiniband-diags/src/ibnetdiscover.c
> +++ b/infiniband-diags/src/ibnetdiscover.c
> @@ -92,8 +92,8 @@ static FILE *f;
>  
>  char *argv0 = "ibnetdiscover";
>  
> -static char *node_name_map = NULL;
> -static FILE *node_name_map_fp = NULL;
> +static char *node_name_map_file = NULL;
> +static nn_map_t *node_name_map = NULL;
>  
>  Node *nodesdist[MAXHOPS+1];     /* last is Ca list */
>  Node *mynode;
> @@ -460,7 +460,7 @@ void
>  list_node(Node *node)
>  {
>  	char *node_type;
> -	char *nodename = remap_node_name(node_name_map_fp, node->nodeguid,
> +	char *nodename = remap_node_name(node_name_map, node->nodeguid,
>  					      node->nodedesc);
>  
>  	switch(node->type) {
> @@ -537,7 +537,7 @@ out_switch(Node *node, int group, char *chname)
>  		fprintf(f, "%d Chip %d", node->chrecord->slotnum, node->chrecord->anafanum);
>  	}
>  
> -	nodename = remap_node_name(node_name_map_fp, node->nodeguid,
> +	nodename = remap_node_name(node_name_map, node->nodeguid,
>  				node->nodedesc);
>  
>  	fprintf(f, "\nSwitch\t%d %s\t\t# \"%s\" %s port 0 lid %d lmc %d\n",
> @@ -606,7 +606,7 @@ out_switch_port(Port *port, int group)
>  	if (ext_port_str)
>  		fprintf(f, "%s", ext_port_str);
>  
> -	rem_nodename = remap_node_name(node_name_map_fp,
> +	rem_nodename = remap_node_name(node_name_map,
>  				port->remoteport->node->nodeguid,
>  				port->remoteport->node->nodedesc);
>  
> @@ -650,7 +650,7 @@ out_ca_port(Port *port, int group)
>  	if (port->remoteport->node->type != SWITCH_NODE)
>  		fprintf(f, " (%" PRIx64 ") ", port->remoteport->portguid);
>  
> -	rem_nodename = remap_node_name(node_name_map_fp,
> +	rem_nodename = remap_node_name(node_name_map,
>  				port->remoteport->node->nodeguid,
>  				port->remoteport->node->nodedesc);
>  
> @@ -890,7 +890,7 @@ main(int argc, char **argv)
>  			break;
>  		switch(ch) {
>  		case 1:
> -			node_name_map = strdup(optarg);
> +			node_name_map_file = strdup(optarg);
>  			break;
>  		case 'C':
>  			ca = optarg;
> @@ -947,7 +947,7 @@ main(int argc, char **argv)
>  		IBERROR("can't open file %s for writing", argv[0]);
>  
>  	madrpc_init(ca, ca_port, mgmt_classes, 2);
> -	node_name_map_fp = open_node_name_map(node_name_map);
> +	node_name_map = open_node_name_map(node_name_map_file);
>  
>  	if (discover(&my_portid) < 0)
>  		IBERROR("discover");
> @@ -957,6 +957,6 @@ main(int argc, char **argv)
>  
>  	dump_topology(list, group);
>  
> -	close_node_name_map(node_name_map_fp);
> +	close_node_name_map(node_name_map);
>  	exit(0);
>  }
> diff --git a/infiniband-diags/src/ibroute.c b/infiniband-diags/src/ibroute.c
> index 44d2fc8..664f7f5 100644
> --- a/infiniband-diags/src/ibroute.c
> +++ b/infiniband-diags/src/ibroute.c
> @@ -50,6 +50,7 @@
>  #include <infiniband/common.h>
>  #include <infiniband/umad.h>
>  #include <infiniband/mad.h>
> +#include <infiniband/complib/cl_nodenamemap.h>
>  
>  #include "ibdiag_common.h"
>  

I think this chunk should be part of patch 3 - I moved it there already.

> diff --git a/infiniband-diags/src/ibtracert.c b/infiniband-diags/src/ibtracert.c
> index c8a7b19..010f45f 100644
> --- a/infiniband-diags/src/ibtracert.c
> +++ b/infiniband-diags/src/ibtracert.c
> @@ -71,8 +71,8 @@ static FILE *f;
>  
>  char *argv0 = "ibtracert";
>  
> -static char *node_name_map = NULL;
> -static FILE *node_name_map_fp = NULL;
> +static char *node_name_map_file = NULL;
> +static nn_map_t *node_name_map = NULL;
>  
>  typedef struct Port Port;
>  typedef struct Switch Switch;
> @@ -205,7 +205,7 @@ dump_endnode(int dump, char *prompt, Node *node, Port *port)
>  		return;
>  	}
>  
> -	nodename = remap_node_name(node_name_map_fp, node->nodeguid, node->nodedesc);
> +	nodename = remap_node_name(node_name_map, node->nodeguid, node->nodedesc);
>  
>  	fprintf(f, "%s %s {0x%016" PRIx64 "} portnum %d lid 0x%x-0x%x \"%s\"\n",
>  		prompt,
> @@ -225,7 +225,7 @@ dump_route(int dump, Node *node, int outport, Port *port)
>  	if (!dump && !verbose)
>  		return;
>  
> -	nodename = remap_node_name(node_name_map_fp, node->nodeguid, node->nodedesc);
> +	nodename = remap_node_name(node_name_map, node->nodeguid, node->nodedesc);
>  
>  	if (dump == 1)
>  		fprintf(f, "[%d] -> {0x%016" PRIx64 "}[%d]\n",
> @@ -637,7 +637,7 @@ dump_mcpath(Node *node, int dumplevel)
>  	if (node->upnode)
>  		dump_mcpath(node->upnode, dumplevel);
>  
> -	nodename = remap_node_name(node_name_map_fp, node->nodeguid, node->nodedesc);
> +	nodename = remap_node_name(node_name_map, node->nodeguid, node->nodedesc);
>  
>  	if (!node->dist) {
>  		printf("From %s 0x%" PRIx64 " port %d lid 0x%x-0x%x \"%s\"\n",
> @@ -741,7 +741,7 @@ main(int argc, char **argv)
>  			break;
>  		switch(ch) {
>  		case 1:
> -			node_name_map = strdup(optarg);
> +			node_name_map_file = strdup(optarg);
>  			break;
>  		case 'C':
>  			ca = optarg;
> @@ -799,7 +799,7 @@ main(int argc, char **argv)
>  		usage();
>  
>  	madrpc_init(ca, ca_port, mgmt_classes, 3);
> -	node_name_map_fp = open_node_name_map(node_name_map);
> +	node_name_map = open_node_name_map(node_name_map_file);
>  
>  	if (ib_resolve_portid_str(&src_portid, argv[0], dest_type, sm_id) < 0)
>  		IBERROR("can't resolve source port %s", argv[0]);
> @@ -838,6 +838,6 @@ main(int argc, char **argv)
>  	/* dump multicast path */
>  	dump_mcpath(endnode, dumplevel);
>  
> -	close_node_name_map(node_name_map_fp);
> +	close_node_name_map(node_name_map);
>  	exit(0);
>  }
> diff --git a/infiniband-diags/src/saquery.c b/infiniband-diags/src/saquery.c
> index a8d810f..c6cc0a2 100644
> --- a/infiniband-diags/src/saquery.c
> +++ b/infiniband-diags/src/saquery.c
> @@ -60,8 +60,8 @@
>  
>  char *argv0 = "saquery";
>  
> -static char *node_name_map = NULL;
> -static FILE *node_name_map_fp = NULL;
> +static char *node_name_map_file = NULL;
> +static nn_map_t *node_name_map = NULL;
>  
>  /**
>   * Declare some globals because I don't want this to be too complex.
> @@ -137,7 +137,7 @@ print_node_record(ib_node_record_t *node_record)
>  		return;
>  	case NAME_OF_LID:
>  	case NAME_OF_GUID:
> -		name = remap_node_name(node_name_map_fp,
> +		name = remap_node_name(node_name_map,
>  					  cl_ntoh64(p_ni->node_guid),
>  					  (char *)p_nd->description);
>  		printf("%s\n", name);
> @@ -1143,7 +1143,7 @@ main(int argc, char **argv)
>  			break;
>  		}
>  		case 2:
> -			node_name_map = strdup(optarg);
> +			node_name_map_file = strdup(optarg);
>  			break;
>  		case 'p':
>  			query_type = IB_MAD_ATTR_PATH_RECORD;
> @@ -1248,7 +1248,7 @@ main(int argc, char **argv)
>  	}
>  
>  	bind_handle = get_bind_handle();
> -	node_name_map_fp = open_node_name_map(node_name_map);
> +	node_name_map = open_node_name_map(node_name_map_file);
>  
>  	switch (query_type) {
>  	case IB_MAD_ATTR_NODE_RECORD:
> @@ -1294,6 +1294,6 @@ main(int argc, char **argv)
>  	if (dst)
>  		free(dst);
>  	clean_up();
> -	close_node_name_map(node_name_map_fp);
> +	close_node_name_map(node_name_map);
>  	return (status);
>  }
> diff --git a/infiniband-diags/src/smpquery.c b/infiniband-diags/src/smpquery.c
> index 7c2c129..89b48f3 100644
> --- a/infiniband-diags/src/smpquery.c
> +++ b/infiniband-diags/src/smpquery.c
> @@ -85,8 +85,8 @@ static const match_rec_t match_tbl[] = {
>  };
>  
>  char *argv0 = "smpquery";
> -static char *node_name_map = NULL;
> -static FILE *node_name_map_fp = NULL;
> +static char *node_name_map_file = NULL;
> +static nn_map_t *node_name_map = NULL;
>  
>  /*******************************************/
>  static char *
> @@ -108,7 +108,7 @@ node_desc(ib_portid_t *dest, char **argv, int argc)
>  	if (!smp_query(nd, dest, IB_ATTR_NODE_DESC, 0, 0))
>  		return "node desc query failed";
>  
> -	nodename = remap_node_name(node_name_map_fp, node_guid, nd);
> +	nodename = remap_node_name(node_name_map, node_guid, nd);
>  
>  	l = strlen(nodename);
>  	if (l < 32) {
> @@ -457,7 +457,7 @@ main(int argc, char **argv)
>  			break;
>  		switch(ch) {
>  		case 1:
> -			node_name_map = strdup(optarg);
> +			node_name_map_file = strdup(optarg);
>  			break;
>  		case 'd':
>  			ibdebug++;
> @@ -513,7 +513,7 @@ main(int argc, char **argv)
>  		IBERROR("operation '%s' not supported", argv[0]);
>  
>  	madrpc_init(ca, ca_port, mgmt_classes, 3);
> -	node_name_map_fp = open_node_name_map(node_name_map);
> +	node_name_map = open_node_name_map(node_name_map_file);
>  
>  	if (dest_type != IB_DEST_DRSLID) {
>  		if (ib_resolve_portid_str(&portid, argv[1], dest_type, sm_id) < 0)
> @@ -530,6 +530,6 @@ main(int argc, char **argv)
>  		if ((err = fn(&portid, argv+3, argc-3)))
>  			IBERROR("operation %s: %s", argv[0], err);
>  	}
> -	close_node_name_map(node_name_map_fp);
> +	close_node_name_map(node_name_map);
>  	exit(0);
>  }
> diff --git a/opensm/complib/cl_nodenamemap.c b/opensm/complib/cl_nodenamemap.c
> index 144a7e4..584c78c 100644
> --- a/opensm/complib/cl_nodenamemap.c
> +++ b/opensm/complib/cl_nodenamemap.c
> @@ -44,67 +44,105 @@
>  
>  #include <complib/cl_nodenamemap.h>
>  
> -FILE *
> +static nn_map_t *
> +read_names(nn_map_t *map)
> +{
> +	char *line = NULL;
> +	size_t len = 0;
> +	name_map_item_t *item;
> +
> +	rewind(map->fp);
> +	while (getline(&line, &len, map->fp) != -1) {
> +		char *guid_str = NULL;
> +		char *name = NULL;
> +		line[len-1] = '\0';
> +		if (line[0] == '#')
> +			goto next_one;
> +
> +		guid_str = strtok(line, "\"#");
> +		name = strtok(NULL, "\"#");
> +		if (!guid_str || !name)
> +			goto next_one;
> +
> +		item = malloc(sizeof(*item));
> +		if (!item) {
> +			goto error;
> +		}
> +		item->guid = strtoull(guid_str, NULL, 0);
> +		item->name = strdup(name);
> +		cl_qmap_insert(&(map->map), item->guid, (cl_map_item_t *)item);
> +
> +next_one:
> +		free (line);
> +		line = NULL;

getline() is able to realloc 'line' buffer by itself, so should this
repeated free() be moved out of loop (with adding a variable which
stores allocated 'line' size)?

> +	}
> +
> +error:
> +	return (map);
> +}
> +
> +nn_map_t *
>  open_node_name_map(char *node_name_map)
>  {
> -	FILE *rc = NULL;
> +	FILE *tmp_fp = NULL;
> +	nn_map_t *rc = NULL;
>  
>  	if (node_name_map != NULL) {
> -		rc = fopen(node_name_map, "r");
> -		if (rc == NULL) {
> +		tmp_fp = fopen(node_name_map, "r");
> +		if (tmp_fp == NULL) {
>  			fprintf(stderr,
>  				"WARNING failed to open switch map \"%s\" (%s)\n",
>  				node_name_map, strerror(errno));
>  		}
>  #ifdef HAVE_DEFAULT_NODENAME_MAP
>  	} else {
> -		rc = fopen(HAVE_DEFAULT_NODENAME_MAP, "r");
> +		tmp_fp = fopen(HAVE_DEFAULT_NODENAME_MAP, "r");
>  #endif /* HAVE_DEFAULT_NODENAME_MAP */
>  	}
> -	return (rc);
> +	if (!tmp_fp)
> +		return (NULL);
> +
> +	rc = malloc(sizeof(*rc));
> +	if (!rc)
> +		return (NULL);
> +	rc->fp = tmp_fp;
> +	cl_qmap_init(&(rc->map));
> +	return (read_names(rc));

read_names() function cannot fail. Probably it would be cleaner to make
it void and just to return rc here.

>  }
>  
>  void
> -close_node_name_map(FILE *fp)
> +close_node_name_map(nn_map_t *map)
>  {
> -	if (fp)
> -		fclose(fp);
> +	name_map_item_t *item = NULL;
> +
> +	if (!map)
> +		return;
> +
> +	item = (name_map_item_t *)cl_qmap_head(&(map->map));
> +	while (item != cl_qmap_end(&(map->map))) {

There are compilation warning about different pointer types.

> +		item = (name_map_item_t *)cl_qmap_remove(&(map->map), item->guid);
> +		free(item->name);
> +		free(item);
> +		item = (name_map_item_t *)cl_qmap_head(&(map->map));
> +	}
> +	if (map->fp)
> +		fclose(map->fp);
> +	free(map);
>  }
>  
>  char *
> -remap_node_name(FILE *node_name_map_fp, uint64_t target_guid, char *nodedesc)
> +remap_node_name(nn_map_t *map, uint64_t target_guid, char *nodedesc)
>  {
> -#define NAME_LEN (256)
> -	char     *line = NULL;
> -	size_t    len = 0;
> -	uint64_t  guid = 0;
> -	char     *rc = NULL;
> -	int       line_count = 0;
> -
> -	if (node_name_map_fp == NULL)
> +	char *rc = NULL;
> +	name_map_item_t *item = NULL;
> +
> +	if (!map)
>  		goto done;
>  
> -	rewind(node_name_map_fp);
> -	for (line_count = 1;
> -		getline(&line, &len, node_name_map_fp) != -1;
> -		line_count++) {
> -		line[len-1] = '\0';
> -		if (line[0] == '#')
> -			goto next_one;
> -		char *guid_str = strtok(line, "\"#");
> -		char *name = strtok(NULL, "\"#");
> -		if (!guid_str || !name)
> -			goto next_one;
> -		guid = strtoull(guid_str, NULL, 0);
> -		if (target_guid == guid) {
> -			rc = strdup(name);
> -			free (line);
> -			goto done;
> -		}
> -next_one:
> -		free (line);
> -		line = NULL;
> -	}
> +	item = (name_map_item_t *)cl_qmap_get(&(map->map), target_guid);
> +	if (item != cl_qmap_end(&(map->map)))

Ditto.

Sasha

> +		rc = strdup(item->name);
> +
>  done:
>  	if (rc == NULL)
>  		rc = strdup(clean_nodedesc(nodedesc));
> diff --git a/opensm/include/complib/cl_nodenamemap.h b/opensm/include/complib/cl_nodenamemap.h
> index a4a09f7..9d0b7d4 100644
> --- a/opensm/include/complib/cl_nodenamemap.h
> +++ b/opensm/include/complib/cl_nodenamemap.h
> @@ -36,17 +36,28 @@
>  
>  #include <stdio.h>
>  #include <stdint.h>
> +#include <complib/cl_qmap.h>
>  
> -/* NOTE: this modifies the parameter "nodedesc". */
> +/* NOTE: this may modify the parameter "nodedesc". */
>  char *clean_nodedesc(char *nodedesc);
>  
> +typedef struct _name_map_item {
> +	cl_map_item_t item;
> +	uint64_t guid;
> +	char *name;
> +} name_map_item_t;
> +typedef struct _node_name_map {
> +	FILE *fp;
> +	cl_qmap_t map;
> +} nn_map_t;
> +
>  /**
>   * Node name map interface.
>   * It is OK to pass NULL for the node_name_map[_fp] parameters.
>   */
> -FILE *open_node_name_map(char *node_name_map);
> -void  close_node_name_map(FILE *node_name_map_fp);
> -char *remap_node_name(FILE *node_name_map_fp, uint64_t target_guid,
> +nn_map_t *open_node_name_map(char *node_name_map);
> +void  close_node_name_map(nn_map_t *map);
> +char *remap_node_name(nn_map_t *map, uint64_t target_guid,
>  			char *nodedesc);
>  	/* NOTE: parameter "nodedesc" may be modified here. */
>  
> -- 
> 1.5.1
> 





More information about the general mailing list