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

[Eli Dorfman (Voltaire)]

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?

Thanks,
Eli