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 <sla...@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
_______________________________________________
general mailing list
general@lists.openfabrics.org
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
