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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel