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

[Ira Weiny]

On Thu, 27 Mar 2008 08:54:29 +0000
Sasha Khapyorsky <sashak at voltaire.com> wrote:

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

Doesn't the root_nodes_list member of updn_t store the guids?  I'm not quite
sure what you are avoiding to store.  It does not look as though you are
searching the file each time the code wants to know if a particular guid is a
root.

In particular I don't want the node name lookup to search the file each time.
That is why I read the file and store it in a map object.  I suppose the
callback method allows for users to use their own data structure object.

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

Ok, however, I think there is a slight difference in the formats the 2
functions parse.  I am looking for '"' symbols to delineate the name string.  I
don't think you are.  For example:

   0xcafebabe "hello world"

Would result in a node guid of 0xcafebabe and a string of "hello world".  I
think your function would parse it as '"hello'?  In fact including the '"'?

Ira