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

[Ira Weiny]

On Sun, 4 Nov 2007 22:34:12 +0200
Sasha Khapyorsky <sashak at voltaire.com> wrote:

> 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>
> >  }
> > 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.

Thanks.

<snip>


> > 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)?

yep, your right.  New patch attached.

> 
> > +	}
> > +
> > +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.

Done, new patch attached.

> 
> >  }
> >  
> >  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.

Fixed.

> 
> > +		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.

Fixed.

Sorry about these I should have caught them.  New patch is attached.

Thanks,
Ira
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Change-node-name-map-implementation-to-use-qmap-in-m.patch
Type: application/octet-stream
Size: 12955 bytes
Desc: not available
URL: <http://lists.openfabrics.org/pipermail/general/attachments/20071105/55305f45/attachment.obj>