Hello,
please see simple patch set fixing minor memory leaks of providers. I'm not aware of any user hitting those currently.
Thanks!
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@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;
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@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.
OK, I just find the usage of tmp_ctx somewhat awkward in this function. I think function would be better off without it completely - service would be allocated directly on mem_ctx. But I agree this is not a bug and possibly just a matter of personal preference, so we can ignore that.
Just make sure tmp_ctx is freed as appropriate.
I haven't reviewed the rest of the patches.
if (!tmp_ctx) return ENOMEM;
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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;
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Bump. (I think that the first patch nacked by Jakub can be ignored.)
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@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;
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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.
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@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;
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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.
LS
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@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;
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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.
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@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
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@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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?
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@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
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@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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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@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.
On 09/29/2015 05:49 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 02:20:39PM +0200, Pavel Reichl wrote:
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.
Updated patch set attached. I removed the discussed 'talloc_new' line from 1st patch.
Bye.
On 09/30/2015 09:21 AM, Pavel Reichl wrote:
On 09/29/2015 05:49 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 02:20:39PM +0200, Pavel Reichl wrote:
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.
Updated patch set attached. I removed the discussed 'talloc_new' line from 1st patch.
Pavel would you re-ack the 1st patch (the rest of the patchset is unchanged), please?
Bye.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 10/02/2015 10:42 AM, Pavel Reichl wrote:
On 09/30/2015 09:21 AM, Pavel Reichl wrote:
On 09/29/2015 05:49 PM, Jakub Hrozek wrote:
On Tue, Sep 29, 2015 at 02:20:39PM +0200, Pavel Reichl wrote:
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 feel like God :D
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.
Updated patch set attached. I removed the discussed 'talloc_new' line from 1st patch.
Pavel would you re-ack the 1st patch (the rest of the patchset is unchanged), please?
Ack to all.
sssd-devel@lists.fedorahosted.org