[openib-general] Re: [PATCH] osm: support for trivial PKey manager

[Hal Rosenstock]

Hi again Ofer,

On Thu, 2005-12-29 at 05:20, Ofer Gigi wrote:
> Hi Hal,
> 
> My name is Ofer Gigi, and I am a new software engineer in Mellanox 
> working on OpenSM.  
> This patch provides a new manager that solves the following problem:
> 
> OpenSM is not currently compliant to the spec statement:
> C14.62.1.1 Table 183 p870 l34:
> "However, the SM shall ensure that one of the P_KeyTable entries in every
>  node contains either the value 0xFFFF (the default P_Key, full membership) 
>  or the value 0x7FFF (the default P_Key, partial membership)."
> 
> Luckily, all IB devices comes up from reset with preconfigured 0xffff key.
> This was discovered during last plugfest. 
> 
> To overcome this limitation I implemented a simple elementary PKey manager
> that will enforce the above rule (currently adds 0xffff if missing).
> 
> This additional manager would be used for a full PKey policy manager 
> in the future.
> 
> We have tested this patch 
> 
> Thanks

Thanks. Applied.

Some mechanical comments below (and also embedded).

The general rule is one thought per patch. osm_indent is separate from
this.

Please try to ensure there is no extra whitespace at the end of the
lines. There were several places where it was present.

-- Hal

> Ofer G.
> 
> Signed-off-by:  Ofer Gigi <oferg at mellanox.co.il>

[snip...]

> Index: opensm/osm_state_mgr.c
> ===================================================================
> --- opensm/osm_state_mgr.c	(revision 4651)
> +++ opensm/osm_state_mgr.c	(working copy)

> @@ -2216,9 +2219,11 @@ osm_state_mgr_process(
>                    }
>                 }
>              }
> +            
>              /*  Need to continue with lid assigning */
>              osm_drop_mgr_process( p_mgr->p_drop_mgr );
> -            p_mgr->state = OSM_SM_STATE_SET_SM_UCAST_LID;
> +            
> +            p_mgr->state = OSM_SM_STATE_SET_PKEY;
>  
>              /*
>               * If we are not MASTER already - this means that we are
> @@ -2229,6 +2234,62 @@ osm_state_mgr_process(
>                 osm_sm_state_mgr_process( p_mgr->p_sm_state_mgr,
>                                           OSM_SM_SIGNAL_DISCOVERY_COMPLETED );
>  
> +            /* signal = osm_lid_mgr_process_sm( p_mgr->p_lid_mgr ); */

Why add this commented out line ?

[I think this was also in one other place as well.]

> +            /* the returned signal might be DONE or DONE_PENDING */
> +            signal = osm_pkey_mgr_process( p_mgr->p_pkey_mgr );
> +            break;
> +
> +         default:
> +           __osm_state_mgr_signal_error( p_mgr, signal );
> +           signal = OSM_SIGNAL_NONE;
> +           break;
> +         }
> +         break;
> +

[snip...]

> Index: opensm/osm_indent
> ===================================================================
> --- opensm/osm_indent	(revision 4651)
> +++ opensm/osm_indent	(working copy)
> @@ -63,8 +63,8 @@
>  # -i3	Substitute indent with 3 spaces
>  # -npcs	No space after procedure calls
>  # -prs	Space after parenthesis
> -# -nsai	No space after if keyword
> -# -nsaw	No space after while keyword
> +# -nsai	No space after if keyword - removed
> +# -nsaw	No space after while keyword - removed

Should these comments just be removed ?

>  # -sc	Put * at left of comments in a block comment style
>  # -nsob	Don't swallow unnecessary blank lines
>  # -ts3	Tab size is 3
> @@ -81,7 +81,7 @@ for sourcefile in $*; do
>          perl -piW -e's/\x0D//' "$sourcefile"
>          echo Processing $sourcefile
>          indent -bad -bap -bbb -nbbo -bl -bli0 -bls -cbi0 -ci3 -cli0 -ncs \
> -                -hnl -i3 -npcs -prs -nsai -nsaf -nsaw -sc -nsob -ts3 -psl -bfda -nut $sourcefile
> +                -hnl -i3 -npcs -prs -sc -nsob -ts3 -psl -bfda -nut $sourcefile
>  
>          rm ${sourcefile}W
>