Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
On (11/10/15 22:00), Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
From 2f71ee096701e869ed3e4646cbf8a4bf7a380f00 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 11 Oct 2015 18:08:46 +0200 Subject: [PATCH 1/4] FO: Don't free rc-allocated structure
src/providers/fail_over.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c index b076687ac6e571f7e27402fd11ac60183ea46951..b309f1c68d0f4219d4b97eb0c01416e53ea856d0 100644 --- a/src/providers/fail_over.c +++ b/src/providers/fail_over.c @@ -507,7 +507,6 @@ create_server_common(TALLOC_CTX *mem_ctx, struct fo_ctx *ctx, const char *name)
common->name = talloc_strdup(common, name); if (common->name == NULL) {
talloc_free(common); return NULL;-- 2.4.3
From 3dd31148b43d0b466c055cf637c00ea8391b694b Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 11 Oct 2015 15:31:44 +0200 Subject: [PATCH 2/4] tests: Reduce failover code duplication
Slightly related request. Could you also merge check bashed failover test? src/tests/fail_over-tests.c
It causes failures in minimal build enviroment if /etc/hosts does not contain localhost. https://fedorahosted.org/sssd/ticket/2617#comment:6 Such failure can be solved with mocked function using cmocka :-)
LS
On (12/10/15 07:42), Lukas Slebodnik wrote:
On (11/10/15 22:00), Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
From 2f71ee096701e869ed3e4646cbf8a4bf7a380f00 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 11 Oct 2015 18:08:46 +0200 Subject: [PATCH 1/4] FO: Don't free rc-allocated structure
src/providers/fail_over.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c index b076687ac6e571f7e27402fd11ac60183ea46951..b309f1c68d0f4219d4b97eb0c01416e53ea856d0 100644 --- a/src/providers/fail_over.c +++ b/src/providers/fail_over.c @@ -507,7 +507,6 @@ create_server_common(TALLOC_CTX *mem_ctx, struct fo_ctx *ctx, const char *name)
common->name = talloc_strdup(common, name); if (common->name == NULL) {
talloc_free(common); return NULL;-- 2.4.3
From 3dd31148b43d0b466c055cf637c00ea8391b694b Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 11 Oct 2015 15:31:44 +0200 Subject: [PATCH 2/4] tests: Reduce failover code duplication
Slightly related request. Could you also merge check bashed failover test? src/tests/fail_over-tests.c
Or we can just remove src/tests/fail_over-tests.c if the same functionality is coverd in cmocka failover test.
LS
On Mon, Oct 12, 2015 at 08:44:27AM +0200, Lukas Slebodnik wrote:
On (12/10/15 07:42), Lukas Slebodnik wrote:
On (11/10/15 22:00), Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
From 2f71ee096701e869ed3e4646cbf8a4bf7a380f00 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 11 Oct 2015 18:08:46 +0200 Subject: [PATCH 1/4] FO: Don't free rc-allocated structure
src/providers/fail_over.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c index b076687ac6e571f7e27402fd11ac60183ea46951..b309f1c68d0f4219d4b97eb0c01416e53ea856d0 100644 --- a/src/providers/fail_over.c +++ b/src/providers/fail_over.c @@ -507,7 +507,6 @@ create_server_common(TALLOC_CTX *mem_ctx, struct fo_ctx *ctx, const char *name)
common->name = talloc_strdup(common, name); if (common->name == NULL) {
talloc_free(common); return NULL;-- 2.4.3
From 3dd31148b43d0b466c055cf637c00ea8391b694b Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 11 Oct 2015 15:31:44 +0200 Subject: [PATCH 2/4] tests: Reduce failover code duplication
Slightly related request. Could you also merge check bashed failover test? src/tests/fail_over-tests.c
Or we can just remove src/tests/fail_over-tests.c if the same functionality is coverd in cmocka failover test.
LS
Not all of it (at least not explicitly), but I'm fine with merging the two. I just don't think it should block review of this set.
On (12/10/15 09:26), Jakub Hrozek wrote:
On Mon, Oct 12, 2015 at 08:44:27AM +0200, Lukas Slebodnik wrote:
On (12/10/15 07:42), Lukas Slebodnik wrote:
On (11/10/15 22:00), Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
From 2f71ee096701e869ed3e4646cbf8a4bf7a380f00 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 11 Oct 2015 18:08:46 +0200 Subject: [PATCH 1/4] FO: Don't free rc-allocated structure
src/providers/fail_over.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c index b076687ac6e571f7e27402fd11ac60183ea46951..b309f1c68d0f4219d4b97eb0c01416e53ea856d0 100644 --- a/src/providers/fail_over.c +++ b/src/providers/fail_over.c @@ -507,7 +507,6 @@ create_server_common(TALLOC_CTX *mem_ctx, struct fo_ctx *ctx, const char *name)
common->name = talloc_strdup(common, name); if (common->name == NULL) {
talloc_free(common); return NULL;-- 2.4.3
From 3dd31148b43d0b466c055cf637c00ea8391b694b Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Sun, 11 Oct 2015 15:31:44 +0200 Subject: [PATCH 2/4] tests: Reduce failover code duplication
Slightly related request. Could you also merge check bashed failover test? src/tests/fail_over-tests.c
Or we can just remove src/tests/fail_over-tests.c if the same functionality is coverd in cmocka failover test.
LS
Not all of it (at least not explicitly), but I'm fine with merging the two. I just don't think it should block review of this set.
sure, I didn't want to review patches :-)
LS
On Sun, Oct 11, 2015 at 10:00:52PM +0200, Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet
Now I did; all downstream tests I ran passed. Feel free to ping me for test results.
On 10/11/2015 10:00 PM, Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
It is not a good idea, but it is a way to go as we agreed on the call.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
Another reason to do failover refactoring soon.
Ack to the patches.
On Wed, Oct 21, 2015 at 11:50:07AM +0200, Pavel Březina wrote:
On 10/11/2015 10:00 PM, Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
It is not a good idea, but it is a way to go as we agreed on the call.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
Another reason to do failover refactoring soon.
Ack to the patches.
Thank you, I squashed the patch that reproduces the error to the patch that fixes the bug, because otherwise the test would fail in CI and pushed the patches to master: * 10c07e188323a2f9824b5e34379f3b1a9b37759e * 4a4af8e1b6a9bab7c7a34d86055a400376e3829e * 63af9215ea9114062fd87003161e6b5982bf9b1f
On (23/10/15 10:22), Jakub Hrozek wrote:
On Wed, Oct 21, 2015 at 11:50:07AM +0200, Pavel Březina wrote:
On 10/11/2015 10:00 PM, Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
It is not a good idea, but it is a way to go as we agreed on the call.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
Another reason to do failover refactoring soon.
Ack to the patches.
Thank you, I squashed the patch that reproduces the error to the patch that fixes the bug, because otherwise the test would fail in CI and pushed the patches to master:
- 10c07e188323a2f9824b5e34379f3b1a9b37759e
- 4a4af8e1b6a9bab7c7a34d86055a400376e3829e
- 63af9215ea9114062fd87003161e6b5982bf9b1f
Do we plan to include these patches in 1.13.4?
They are in fedora since Nov 20 2015
LS
On Thu, Mar 03, 2016 at 04:50:33PM +0100, Lukas Slebodnik wrote:
On (23/10/15 10:22), Jakub Hrozek wrote:
On Wed, Oct 21, 2015 at 11:50:07AM +0200, Pavel Březina wrote:
On 10/11/2015 10:00 PM, Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
It is not a good idea, but it is a way to go as we agreed on the call.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
Another reason to do failover refactoring soon.
Ack to the patches.
Thank you, I squashed the patch that reproduces the error to the patch that fixes the bug, because otherwise the test would fail in CI and pushed the patches to master:
- 10c07e188323a2f9824b5e34379f3b1a9b37759e
- 4a4af8e1b6a9bab7c7a34d86055a400376e3829e
- 63af9215ea9114062fd87003161e6b5982bf9b1f
Do we plan to include these patches in 1.13.4?
They are in fedora since Nov 20 2015
Yeah, feel free to push them. I was reluctant at first because the user I developed the patches for seemed to have issues with the patched build, but I never could get more details about the issue they were seeing..
On (23/10/15 10:22), Jakub Hrozek wrote:
On Wed, Oct 21, 2015 at 11:50:07AM +0200, Pavel Březina wrote:
On 10/11/2015 10:00 PM, Jakub Hrozek wrote:
Hi,
the attached patches are my proposal to fix https://fedorahosted.org/sssd/ticket/2829
I haven't tested them past make check yet, because I'm not sure I like them myself :) but at the same time I can't see a better way to keep track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would prefer to instead have a fo_server_wrap structure that would be used for the server_list, but I didn't want to do a large change before we agree the refcount is a good idea at all.
It is not a good idea, but it is a way to go as we agreed on the call.
The other ugly side-effect is that we need to be sure that nobody calls talloc_free on the fo_server structure. Instead, only the parent context can be freed (that's also what the first patch is about).
Another reason to do failover refactoring soon.
Ack to the patches.
Thank you, I squashed the patch that reproduces the error to the patch that fixes the bug, because otherwise the test would fail in CI and pushed the patches to master:
- 10c07e188323a2f9824b5e34379f3b1a9b37759e
- 4a4af8e1b6a9bab7c7a34d86055a400376e3829e
- 63af9215ea9114062fd87003161e6b5982bf9b1f
sssd-1-13: * c532290fb1a85a63c3bc9ef95ba7d6248f0e94c1 * 383bdbe8d40cba2d0848b2256716358c7f8e8f2d * 5d485ce42a4d56581d44c7224e78083a4ff1e81b
LS
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina