[ofa-general] Re: [PATCH] opensm/osm_subnet.c Fix memory leak for QOS string parameters.

[Sasha Khapyorsky]

On 08:43 Thu 15 Jan     , Eli Dorfman (Voltaire) wrote:
> Sasha Khapyorsky wrote:
> > Hi Eli,
> > 
> > On 13:54 Sun 21 Dec     , Eli Dorfman (Voltaire) wrote:
> >> Fix memory leak for QOS string parameters.
> >>
> >>  Signed-off-by: Slava Strebkov <slavas at amirm.voltaire.com>
> >>
> >> ---
> >>  opensm/opensm/osm_subnet.c |   21 +++++++++++++++++++++
> >>  1 files changed, 21 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
> >> index 122d4dd..f8b29f8 100644
> >> --- a/opensm/opensm/osm_subnet.c
> >> +++ b/opensm/opensm/osm_subnet.c
> >> @@ -331,6 +331,21 @@ static void subn_init_qos_options(IN osm_qos_options_t * opt)
> >>  	opt->sl2vl = NULL;
> >>  }
> >>  
> >> +static void subn_free_qos_options(IN osm_qos_options_t * opt)
> >> +{
> >> +	if ((opt->vlarb_high) && (opt->vlarb_high != OSM_DEFAULT_QOS_VLARB_HIGH)) {
> >> +		free(opt->vlarb_high);
> >> +	}
> >> +
> >> +	if ((opt->vlarb_low) && (opt->vlarb_low != OSM_DEFAULT_QOS_VLARB_LOW)) {
> >> +		free(opt->vlarb_low);
> >> +	}
> >> +
> >> +	if ((opt->sl2vl) && (opt->sl2vl != OSM_DEFAULT_QOS_SL2VL)) {
> >> +		free(opt->sl2vl);
> >> +	}
> >> +}
> > 
> > With gcc-4.3.2 using '-Wall' flag I get warning here:
> > 
> > "comparison with string literal results in unspecified behavior"
> > 
> > It is actually true since used OSM_DEFAULT_QOS_* macros are defined as
> > strings - it is something equal to:
> > 
> > 	char *p = "123456";
> > 
> > 	....
> > 
> > 	if (p != "123456")
> > 		free(p1);
> > 
> > Gcc is smart enough and uses the same string constant "123456" in both
> > cases (so your patch actually works), but don't think that it is
> > guaranteed in C language.
> > 
> > So I think to rework this part - we can always allocate string qos config
> > parameters (just similar to other config), and free it when it is not
> > NULL. Something like this:
> > 
> > 
> > diff --git a/opensm/opensm/osm_subnet.c b/opensm/opensm/osm_subnet.c
> > index a6db304..94b6332 100644
> > --- a/opensm/opensm/osm_subnet.c
> > +++ b/opensm/opensm/osm_subnet.c
> > @@ -333,13 +333,13 @@ static void subn_init_qos_options(IN osm_qos_options_t * opt)
> >  
> >  static void subn_free_qos_options(IN osm_qos_options_t * opt)
> >  {
> > -	if (opt->vlarb_high && opt->vlarb_high != OSM_DEFAULT_QOS_VLARB_HIGH)
> > +	if (opt->vlarb_high)
> >  		free(opt->vlarb_high);
> >  
> > -	if (opt->vlarb_low && opt->vlarb_low != OSM_DEFAULT_QOS_VLARB_LOW)
> > +	if (opt->vlarb_low)
> >  		free(opt->vlarb_low);
> >  
> > -	if (opt->sl2vl && opt->sl2vl != OSM_DEFAULT_QOS_SL2VL)
> > +	if (opt->sl2vl)
> >  		free(opt->sl2vl);
> >  }
> >  
> > @@ -803,7 +803,7 @@ static void subn_verify_vlarb(char **vlarb, const char *prefix,
> >  	if (*vlarb == NULL) {
> >  		log_report(" Invalid Cached Option: %s_vlarb_%s: "
> >  		"Using Default\n", prefix, suffix);
> > -		*vlarb = dflt;
> > +		*vlarb = strdup(dflt);
> >  		return;
> >  	}
> >  
> > @@ -872,7 +872,7 @@ static void subn_verify_sl2vl(char **sl2vl, const char *prefix, char *dflt)
> >  	if (*sl2vl == NULL) {
> >  		log_report(" Invalid Cached Option: %s_sl2vl: Using Default\n",
> >  			   prefix);
> > -		*sl2vl = dflt;
> > +		*sl2vl = strdup(dflt);
> >  		return;
> >  	}
> > 
> > 
> > Thoughts?
> 
> looks cleaner. will you commit this?

I will.

Sasha