[SSSD] [PATCHES] fix minor memory leaks

Pavel Reichl preichl at redhat.com
Tue Sep 29 12:20:39 UTC 2015



On 09/29/2015 02:01 PM, Lukas Slebodnik wrote:
> On (29/09/15 13:32), Pavel Reichl wrote:
>>
>>
>> On 09/21/2015 11:16 AM, Lukas Slebodnik wrote:
>>> On (21/09/15 10:59), Pavel Březina wrote:
>>>> On 09/21/2015 10:58 AM, Lukas Slebodnik wrote:
>>>>> On (21/09/15 10:40), Pavel Březina wrote:
>>>>>> On 09/20/2015 07:58 PM, Pavel Reichl wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 09/04/2015 01:56 PM, Jakub Hrozek wrote:
>>>>>>>> On Fri, Sep 04, 2015 at 01:09:36PM +0200, Pavel Reichl wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> please see simple patch set fixing minor memory leaks of providers.
>>>>>>>>> I'm not aware of any user hitting those currently.
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>>>  From ef62f0245cde314fd11b1cd2584589e018ede050 Mon Sep 17 00:00:00 2001
>>>>>>>>> From: Pavel Reichl <preichl at redhat.com>
>>>>>>>>> Date: Fri, 4 Sep 2015 07:02:42 -0400
>>>>>>>>> Subject: [PATCH 1/4] AD: fix minor memory leak
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>   src/providers/ad/ad_common.c | 7 ++++---
>>>>>>>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
>>>>>>>>> index
>>>>>>>>> 130cdeb613aae3843f7453a478815daaae6aab77..aca550581708e6501c99f1c69e3c5ec9303d3b8c
>>>>>>>>> 100644
>>>>>>>>> --- a/src/providers/ad/ad_common.c
>>>>>>>>> +++ b/src/providers/ad/ad_common.c
>>>>>>>>> @@ -658,7 +658,7 @@ ad_failover_init(TALLOC_CTX *mem_ctx, struct
>>>>>>>>> be_ctx *bectx,
>>>>>>>>>       TALLOC_CTX *tmp_ctx;
>>>>>>>>>       struct ad_service *service;
>>>>>>>>>
>>>>>>>>> -    tmp_ctx = talloc_new(mem_ctx);
>>>>>>>>> +    tmp_ctx = talloc_new(NULL);
>>>>>>>>
>>>>>>>> I don't think we should do this kind of change, it's quite possible
>>>>>>>> we'll start using talloc pools soon which will make us migrate from
>>>>>>>> using NULL context.
>>>>>>>>
>>>>>>>> Just make sure tmp_ctx is freed as appropriate.
>>>>>>>>
>>>>>>>> I haven't reviewed the rest of the patches.
>>>>>>>>
>>>>>>>>>       if (!tmp_ctx) return ENOMEM;
>>>>>>>>
>>>>>>> Bump. (I think that the first patch nacked by Jakub can be ignored.)
>>>>>>
>>>>>> Ack to all.
>>>>>>
>>>>>> I think even the first one should be pushed. 1) I don't think we are going to
>>>>>> use talloc pools anytime soon. 2) When and if we are going to use them it
>>>>>> will be a big change and in my opinion it will be more doable to convert
>>>>>> consistent code.
>>>>>>
>>>>> Consistent code does not require following change in the 1st patch
>>>>> -    tmp_ctx = talloc_new(mem_ctx);
>>>>> +    tmp_ctx = talloc_new(NULL);
>>>>>
>>>>> temporary context needn't be allocated on NULL context
>>>>> IMHO; it can be allocated on mem_ctx without any issue.
>>>>
>>>> It can. It is not our coding style though.
>>>>
>>> But it needn't :-)
>>> So this change is not necessary.
>>>
>>> I checked the source code and in 10% of cases we do not use
>>> NULL context for temporary context. Moreover if we want to use talloc pools
>>> in future then it would be better to reduce usage of NULL context.
>>>
>>> It's fine to use NULL context for allocation in function which can be unit
>>> tested (leak_check_setup, leak_check_teardown); but it is not this case
>>> in the 1st patch.
>>>
>>> LS
>>>
>>
>> As the discussion about the first patch stalled I propose to omit it and push the rest of the patch set to master.
>>
>> Should I send updated patch set or committer can skip the first patch himself?
> It would be good to to also push 1st patch
> without change
> -    tmp_ctx = talloc_new(mem_ctx);
> +    tmp_ctx = talloc_new(NULL);
>

OK, I'll amend the patch.

>
> There were two -1 for the first patch (jhrozek, lslebodn)
> and one +1 from pbrezina.
>
> If we consider you an author then there is another +1.
>
> Do we need another (5th person) to decide which way is better?
> or are two -1s enough to remove such change?

In my opinion we can drop the change. This is not an imminent bug it's rather code style dispute and I don't think that we have to bother Sumit for that. Unless of course pbrezina feels different.

>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>


More information about the sssd-devel mailing list