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);
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?
LS