[openib-general] [PATCHv5] osm: partition manager force policy

[Hal Rosenstock]

Hi Eitan,

On Wed, 2006-06-21 at 02:10, Eitan Zahavi wrote:
> Hi Hal,
> 
> Thanks for applying the patch.
> 
> Regarding the issues :
> 
> Hal Rosenstock wrote:
> >>+
> >>+	CL_ASSERT( p_pkey_tbl );
> > 
> > 
> > Should the other routines also assert on this or should this be
> > consistent with the others ?
> Yes it should b consistent.
> Normally I add assertion on OUT parameters such that a "misuse" is caught.
> The idea is that parameters provided by reference are more likely to be passed
> by mistake as NULL. 
> So I would remove the assert on p_key_tbl.

p_pkey_tbl is a pointer so wouldn't that rule apply ?

I do notice that in the particular usage in osm_pkey_mgr.c it would
already get caught by the assert in osm_physp_get_mod_pkey_tbl.

> >>+	CL_ASSERT( p_block_idx != NULL );
> >>+	CL_ASSERT( p_pkey_idx != NULL );
> > 
> > 
> > There is no p_pkey_idx parameter. I presume this should be p_pkey_index.
> Ooops - this means the code will not compile in debug mode !
> I see you fixed that.
> > 
> > 
> > Also, two things about osm_pkey_mgr.c:
> > 
> > Was there a need to reorder the routines ? This broke the diff so it had
> > to be done largely by hand.
> I reordered to to be defined in the order used.
> Already agree with Sasha that I should have done that on separate patch.

I thought since the patch was reissued several times this comment would
have been addressed.

-- Hal

> > Also, it would have been nice not to mix the format changes with the
> > substantive changes. Try to keep it to "one thought per patch".
> OK.
> > 
> > This patch has been applied with cosmetic changes. We will go from
> > here...
> Thanks
> 
> Eitan