[openib-general] RE: [PATCH] opensm: remove osm_pkey_mgr.h
Eitan Zahavi
eitan at mellanox.co.il
Mon May 22 03:55:19 PDT 2006
Hi Sasha,
My point is simple:
OpenSM has a very structured skeleton:
1. All mad receivers have two c files and two h files
1.1 mad receive controller which deals with dispatcher registration .
1.2 Mad receiver which deals with all the action happening after such a
mad is received
2. All algorithm stages (managers) have a c file and h file
An algorithm stage might be lid assignment, routing, partition
enforcement etc
3. All SMDB objects have a c file and h files.
Examples are Nodes, Ports, Multicast registrations etc
These are the structural code rules for OpenSM.
Even if you think it is better to merge functionality and avoid having
some of these h files and you might even be able to save some lines of
code by doing that, you break the code structure. If you personally like
to work with flat code - this is your preference.
I prefer having clear structure. So if I need to know where is the pkey
manager object defined? What are its internal state variables? What are
the algorithms it uses? Etc I can simply open up osm_pkey_mgr.h and find
this out.
If you want to redesign OpenSM structure to be "simpler" or more
"effective" you can propose doing that. But doing it in the salami way
is just going to hurt stability and leave us with no structure at all.
Unless we are willing to re-architect the code in a clean manner and
spend the years of development and validation for these changes - lets
keep the code with clear structure.
Eitan
Eitan Zahavi
Senior Engineering Director, Software Architect
Mellanox Technologies LTD
Tel:+972-4-9097208
Fax:+972-4-9593245
P.O. Box 586 Yokneam 20692 ISRAEL
> -----Original Message-----
> From: Sasha Khapyorsky [mailto:sashak at voltaire.com]
> Sent: Monday, May 22, 2006 12:14 PM
> To: Eitan Zahavi
> Cc: Hal Rosenstock; openib-general at openib.org; Yael Kalka; Ofer Gigi
> Subject: Re: [PATCH] opensm: remove osm_pkey_mgr.h
>
> Hi Eitan,
>
> On 09:27 Mon 22 May , Eitan Zahavi wrote:
> >
> > Every OpenSM manager has an H file.
>
> Even if it is not necessary? Why?
>
> > Instead of trying to "save" lines of code - please focus on
improving
> > code readability and structure.
>
> This is exactly my point - to improve code readability and structure
> (and obviously this is not last change needed in this area). And I
> cannot see how huge amount of duplicated code and useless structures
> may help there. Instead I can see how it hurts.
>
> > NOTE: this is not kernel code. The tradeoff for user land code is
> > different.
>
> Yes, this is not kernel code, but I don't see your point. (don't think
> that you are referring kernel sources as example of unreadable and bad
> structured code :))
>
> Probably you see this patch as "extremal optimization"? I see it as
> cleanup and structure improvement.
>
> Sasha
>
> > If you save us one header file - but break the structure of the code
you
> > make more damage than good.
> >
> > Eitan Zahavi
> > Senior Engineering Director, Software Architect
> > Mellanox Technologies LTD
> > Tel:+972-4-9097208
> > Fax:+972-4-9593245
> > P.O. Box 586 Yokneam 20692 ISRAEL
> >
> >
> > > -----Original Message-----
> > > From: Sasha Khapyorsky [mailto:sashak at voltaire.com]
> > > Sent: Monday, May 22, 2006 1:16 AM
> > > To: Hal Rosenstock
> > > Cc: openib-general at openib.org; Eitan Zahavi; Yael Kalka; Ofer Gigi
> > > Subject: [PATCH] opensm: remove osm_pkey_mgr.h
> > >
> > >
> > > Since we expect that osm_pkey_mgr_process() will be called only
from
> > > osm_state_mgr_process() this patch replaces osm_pkey_mgr.h header
file
> > > by local prototype.
> > >
> > > Signed-off-by: Sasha Khapyorsky <sashak at voltaire.com>
> > > ---
> > >
> > > osm/include/Makefile.am | 1
> > > osm/include/opensm/osm_pkey_mgr.h | 92
> > -------------------------------------
> > > osm/opensm/osm_pkey_mgr.c | 1
> > > osm/opensm/osm_state_mgr.c | 3 +
> > > 4 files changed, 2 insertions(+), 95 deletions(-)
> > >
> > > diff --git a/osm/include/Makefile.am b/osm/include/Makefile.am
> > > index b23b1de..2bee762 100644
> > > --- a/osm/include/Makefile.am
> > > +++ b/osm/include/Makefile.am
> > > @@ -96,7 +96,6 @@ EXTRA_DIST = \
> > > $(srcdir)/opensm/st.h \
> > > $(srcdir)/opensm/osm_mcast_tbl.h \
> > > $(srcdir)/opensm/osm_pkey.h \
> > > - $(srcdir)/opensm/osm_pkey_mgr.h \
> > > $(srcdir)/opensm/osm_sa_mad_ctrl.h \
> > > $(srcdir)/opensm/osm_req_ctrl.h \
> > > $(srcdir)/opensm/osm_sw_info_rcv.h \
> > > diff --git a/osm/include/opensm/osm_pkey_mgr.h
> > > b/osm/include/opensm/osm_pkey_mgr.h
> > > deleted file mode 100644
> > > index cb0075d..0000000
> > > --- a/osm/include/opensm/osm_pkey_mgr.h
> > > +++ /dev/null
> > > @@ -1,92 +0,0 @@
> > > -/*
> > > - * Copyright (c) 2006 Voltaire, Inc. All rights reserved.
> > > - * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights
> > reserved.
> > > - *
> > > - * 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.
> > > - *
> > > - * $Id$
> > > - */
> > > -
> > > -
> > > -/*
> > > - * Abstract:
> > > - * Prototype for osm_pkey_mgr_process() function
> > > - * This is part of the OpenSM family of objects.
> > > - *
> > > - * Environment:
> > > - * Linux User Mode
> > > - *
> > > - * $Revision: 1.4 $
> > > - */
> > > -
> > > -
> > > -#ifndef _OSM_PKEY_MGR_H_
> > > -#define _OSM_PKEY_MGR_H_
> > > -
> > > -#include <opensm/osm_base.h>
> > > -#include <opensm/osm_opensm.h>
> > > -
> > > -#ifdef __cplusplus
> > > -# define BEGIN_C_DECLS extern "C" {
> > > -# define END_C_DECLS }
> > > -#else /* !__cplusplus */
> > > -# define BEGIN_C_DECLS
> > > -# define END_C_DECLS
> > > -#endif /* __cplusplus */
> > > -
> > > -BEGIN_C_DECLS
> > > -
> > > -/****f* OpenSM: P_Key Manager/osm_pkey_mgr_process
> > > -* NAME
> > > -* osm_pkey_mgr_process
> > > -*
> > > -* DESCRIPTION
> > > -* This function enforces the pkey rules on the SM DB.
> > > -*
> > > -* SYNOPSIS
> > > -*/
> > > -osm_signal_t
> > > -osm_pkey_mgr_process(
> > > - IN osm_opensm_t *p_osm );
> > > -/*
> > > -* PARAMETERS
> > > -* p_osm
> > > -* [in] Pointer to an osm_opensm_t object.
> > > -*
> > > -* RETURN VALUES
> > > -* None
> > > -*
> > > -* NOTES
> > > -*
> > > -* SEE ALSO
> > > -*********/
> > > -
> > > -END_C_DECLS
> > > -
> > > -#endif /* _OSM_PKEY_MGR_H_ */
> > > diff --git a/osm/opensm/osm_pkey_mgr.c b/osm/opensm/osm_pkey_mgr.c
> > > index e08b7cc..91c1a95 100644
> > > --- a/osm/opensm/osm_pkey_mgr.c
> > > +++ b/osm/opensm/osm_pkey_mgr.c
> > > @@ -56,7 +56,6 @@ #include <complib/cl_qmap.h>
> > > #include <complib/cl_debug.h>
> > > #include <opensm/osm_node.h>
> > > #include <opensm/osm_switch.h>
> > > -#include <opensm/osm_pkey_mgr.h>
> > > #include <opensm/osm_partition.h>
> > > #include <opensm/osm_opensm.h>
> > >
> > > diff --git a/osm/opensm/osm_state_mgr.c
b/osm/opensm/osm_state_mgr.c
> > > index 42fd5e8..724b2b7 100644
> > > --- a/osm/opensm/osm_state_mgr.c
> > > +++ b/osm/opensm/osm_state_mgr.c
> > > @@ -66,14 +66,15 @@ #include <opensm/osm_helper.h>
> > > #include <opensm/osm_msgdef.h>
> > > #include <opensm/osm_node.h>
> > > #include <opensm/osm_port.h>
> > > -#include <opensm/osm_pkey_mgr.h>
> > > #include <vendor/osm_vendor_api.h>
> > > #include <opensm/osm_sm_state_mgr.h>
> > > #include <opensm/osm_opensm.h>
> > >
> > >
> >
/**********************************************************************
> > > + * Prototypes for manager processors used locally
> > >
> >
**********************************************************************/
> > > osm_signal_t osm_qos_setup(IN osm_opensm_t * p_osm);
> > > +osm_signal_t osm_pkey_mgr_process(IN osm_opensm_t * p_osm);
> > >
> > >
> >
/**********************************************************************
> > >
> >
**********************************************************************/
More information about the general
mailing list