On Tue, Sep 29, 2015 at 02:20:39PM +0200, Pavel Reichl wrote:
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.
I don't mind too much, it just doesn't seem to be helpful change, that's
all, so I'll be a bit happier if we drop it.
On the other hand, if we decide to use talloc pools, then we'll change
all or most NULL contexts, which would essentially revert this change.