[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