[ofa-general] Re: [PATCH 1/3 - no ibcommon] Create a new library libibnetdisc

[Ira Weiny]

Sasha,

I have integrated your comments except for the responses below...


On Wed, 21 Jan 2009 16:49:23 +0200
Sasha Khapyorsky <sashak at voltaire.com> wrote:

> On 15:47 Fri 09 Jan     , Ira Weiny wrote:
> > 
> > Signed-off-by: weiny2 at llnl.gov <weiny2 at llnl.gov>
> 

<snip>

> > +
> > diff --git a/infiniband-diags/libibnetdisc/src/internal.h b/infiniband-diags/libibnetdisc/src/internal.h
> > new file mode 100644
> > index 0000000..89f238f
> > --- /dev/null
> > +++ b/infiniband-diags/libibnetdisc/src/internal.h
> > @@ -0,0 +1,82 @@
> > +/*
> > + * Copyright (c) 2008 Lawrence Livermore National Laboratory
> > + *
> > + * This software is available to you under a choice of one of two
> > + * licenses.  You may choose to be licensed under the terms of the GNU
> > + * General Public License (GPL) Version 2, available from the file
> > + * COPYING in the main directory of this source tree, or the
> > + * OpenIB.org BSD license below:
> > + *
> > + *     Redistribution and use in source and binary forms, with or
> > + *     without modification, are permitted provided that the following
> > + *     conditions are met:
> > + *
> > + *      - Redistributions of source code must retain the above
> > + *        copyright notice, this list of conditions and the following
> > + *        disclaimer.
> > + *
> > + *      - Redistributions in binary form must reproduce the above
> > + *        copyright notice, this list of conditions and the following
> > + *        disclaimer in the documentation and/or other materials
> > + *        provided with the distribution.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> > + * SOFTWARE.
> > + *
> > + */
> > +
> > +/** =========================================================================
> > + * Define the internal data structures.
> > + */
> > +
> > +#ifndef _INTERNAL_H_
> > +#define _INTERNAL_H_
> > +
> > +#include <infiniband/ibnetdisc.h>
> > +
> > +struct ibnd_node {
> > +	/* This member MUST BE FIRST */
> > +	ibnd_node_t node;
> > +
> > +	/* internal use only */
> > +	unsigned char ch_found;
> > +	struct ibnd_node *htnext; /* hash table list */
> > +	struct ibnd_node *dnext; /* nodesdist next */
> > +	struct ibnd_node *type_next; /* next based on type */
> > +};
> > +#define CONV_NODE_INTERNAL(node) ((struct ibnd_node *)node)
> > +
> > +struct ibnd_port {
> > +	/* This member MUST BE FIRST */
> > +	ibnd_port_t port;
> > +
> > +	/* internal use only */
> > +	struct ibnd_port *htnext;
> > +};
> > +#define CONV_PORT_INTERNAL(port) ((struct ibnd_port *)port)
> > +
> > +struct ibnd_fabric {
> > +	/* This member MUST BE FIRST */
> > +	ibnd_fabric_t fabric;
> > +
> > +	/* internal use only */
> > +	void *ibmad_port;
> > +	struct ibnd_node *nodestbl[HTSZ];
> > +	struct ibnd_port *portstbl[HTSZ];
> > +	struct ibnd_node *nodesdist[MAXHOPS+1];
> > +	ibnd_chassis_t *first_chassis;
> > +	ibnd_chassis_t *current_chassis;
> > +	ibnd_chassis_t *last_chassis;
> > +	struct ibnd_node *switches;
> > +	struct ibnd_node *ch_adapters;
> > +	struct ibnd_node *routers;
> > +};
> > +#define CONV_FABRIC_INTERNAL(fabric) ((struct ibnd_fabric *)fabric)
> 
> Why should we hide this internal data so hard?
> 
> Maybe we can use a single structures, to mark those fields (in comment)
> as "for internal use" - somebody may want to use it.

At first I did mark "for internal use", however, as I worked on the library
more I realized just how important this data is to the library.  If the user
should misuse the data then the library will fail.  I felt it was better to
keep this house keeping internal.  For now I plan on keeping it internal and we
can discuss more later.

> 
> > +
> > +#endif /* _INTERNAL_H_ */
> > diff --git a/infiniband-diags/libibnetdisc/src/libibnetdisc.map b/infiniband-diags/libibnetdisc/src/libibnetdisc.map
> > new file mode 100644
> > index 0000000..5e8c315
> > --- /dev/null
> > +++ b/infiniband-diags/libibnetdisc/src/libibnetdisc.map
> > @@ -0,0 +1,27 @@
> > +IBNETDISC_1.0 {
> > +	global:
> > +		ibnd_debug;
> > +		ibnd_show_progress;
> > +		ibnd_discover_fabric;
> > +		ibnd_cache_fabric;
> > +		ibnd_read_fabric;
> > +		ibnd_destroy_fabric;
> > +		ibnd_find_node_guid;
> > +		ibnd_update_node;
> 
> Where is ibnd_update_node() useful (in public API)?

I have not used it in the diags yet but I have plans to use it in a deamon
which will update nodes without rescaning the entire fabric.  I can remove it
if you really want but I feel this is the first step to using the library
somewhere other than in "single run" tools.

Ira

<snip>