[ofa-general] Re: [opensm patch][1/2] fix qos config parsing bugs

[Sasha Khapyorsky]

On 09:20 Thu 13 Nov     , Al Chu wrote:
> Hey Sasha,
> 
> On Thu, 2008-11-13 at 02:24 +0200, Sasha Khapyorsky wrote:
> > Hi Al,
> > 
> > On 15:57 Tue 11 Nov     , Al Chu wrote:
> > > 
> > > Sorry, I may have not explained it well. Lets say I do this in the
> > > config file.
> > > 
> > > qos_vlarb_high FOOBAR
> > > # qos_ca_vlarb_high BLAH
> > > qos_swe_vlarb_high XYZZY
> > > 
> > > I currently expect qos_ca_vlarb_high to use the value of FOOBAR because
> > > I commented out the field.  But it uses OSM_DEFAULT_QOS_HIGH_LIMIT
> > > instead.  The reason is because qos_build_config() checks for NULL to
> > > use default vs. non-default values.
> > > 
> > > p = opt->vlarb_high ? opt->vlarb_high : dflt->vlarb_high;
> > > 
> > > Under the above situation where I've commented out veral fields, opt-
> > > >vlarb_high is always non-NULL b/c it was set to
> > > OSM_DEFAULT_QOS_HIGH_LIMIT. Thus OSM_DEFAULT_QOS_HIGH_LIMIT is used
> > > instead of FOOBAR.
> > > 
> > > > > 2)
> > > > > 
> > > > > In qos_build_config() we load the high_limit like this:
> > > > > 
> > > > > cfg->vl_high_limit = (uint8_t) opt->high_limit;
> > > > > 
> > > > > So there is no way to tell the qos_ca, qos_swe, qos_rtr, etc. high_limit
> > > > > options to "go back to" the default high_limit.  It just assumes that
> > > > > whatever is input (or was set by default) is what you should use.
> > > > 
> > > > Right. What is a limitation here? That an user cannot set this to
> > > > "no value"? But she/he can just skip it.
> > > 
> > > Similar to the above issue, lets say I want to do:
> > > 
> > > qos_high_limit 8
> > > # qos_ca_high_limit 15
> > > # qos_swe_high_limit 15
> > > 
> > > I want qos_ca_high_limit and qos_swe_high_limit to use whatever I set in
> > > qos_high_limit.  But the code doesn't allow for this.
> > > 
> > > > 
> > > > > 3)
> > > > > 
> > > > > Some fields like qos_vlarb_high are assumed to be correctly set and can
> > > > > segfault opensm.
> > > > 
> > > > qos_build_config() assumes that valid parameters are used. And we are
> > > > using this this way (I hope :)) (finally it is not library API).
> > > 
> > > I think the issue is the osm_subnet.c code did not properly check all
> > > inputs, and subsequently some inputs used in qos_build_config() were
> > > bad.  I think
> > > 
> > > qos_vlarb_high (null)
> > > 
> > > was something I tried that opensm seg-faulted on.  
> > 
> > Ok. I see now.
> > 
> > Probably it will be simpler just to generate a valid qos parameter sets
> > right after parser (in verification time)?
> 
> Ahh, I see what you did.  It's much cleaner this way.
> 
> > Like in your modified (and
> > rebased against recent patches) patch below?
> 
> Patch looks good to me.

Applied. Thakns.

Sasha