[ofa-general] Re: [PATCH 1/6] complib/nodenamemap: add generic parse_node_map() function

[Sasha Khapyorsky]

Hi Ira,

On 17:52 Wed 26 Mar     , Ira Weiny wrote:
> > diff --git a/opensm/complib/cl_nodenamemap.c b/opensm/complib/cl_nodenamemap.c
> > index af5da39..696d455 100644
> > --- a/opensm/complib/cl_nodenamemap.c
> > +++ b/opensm/complib/cl_nodenamemap.c
> > @@ -160,3 +160,48 @@ clean_nodedesc(char *nodedesc)
> >  
> >  	return (nodedesc);
> >  }
> > +
> > +int parse_node_map(const char *file_name,
> > +		   int (*create)(void *, uint64_t, char *), void *cxt)
> 
> Why do you need this new way of parsing the file format?  I think passing this
> callback makes the code more complicated that it needs to be.

I don't think that it is much more complicated. OTOH it provides a great
flexibility (it is used in later patches).

> Is there a reason not to use the nn_map_t as the type for root_nodes_list in
> the updn_t struct?

If you will look at later updn patches you will find that there is no
any root nodes storage at all. The same is true for later up/down IDs
patch. Likely similar (intermediate storage free) improvement is
possible in fat-tree as well.

> We could rename nn_map_t to be more generic if necessary.
>
> I worry about maintaining 2 functions which parse the same file format in the
> same c file.

I agree with you, just didn't time to merge it.

Sasha