[PATCH] subdomains: first destroy ptask than remove sdom - sssd-devel - Fedora mailing-lists

List overview All Threads
Download

[PATCH] subdomains: first destroy ptask than remove sdom

[PATCHES] IPA: add trusted...

[PATCH]...

Pavel Březina

22 Oct 2013 22 Oct '13

9:24 a.m.

Attachments:

0001-subdomains-first-destroy-ptask-then-remove-sdom.patch (text/x-patch — 1.3 KB)

Reply

Show replies by date

Jakub Hrozek

23 Oct 23 Oct

12:56 p.m.

On Tue, Oct 22, 2013 at 04:24:04PM +0200, Pavel Březina wrote:

From 5906f84cf340b925ae63d72e9fcec9b3700f9f14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 22 Oct 2013 16:15:16 +0200 Subject: [PATCH 1/2] subdomains: first destroy ptask then remove sdom

be_ptask_destroy was unreachable since sdom is not present in the list of sdap domains any more.

src/providers/ipa/ipa_subdomains.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 6b88bc69bf069cc531785e7e569484e8d8572748..294af47de46b68ea84ec017f734c81fc44556423 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -289,12 +289,12 @@ ipa_ad_subdom_remove(struct ipa_subdomains_ctx *ctx, return; }

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

sdom = sdap_domain_get(iter->ad_id_ctx->sdap_id_ctx->opts, subdom); if (sdom == NULL) return; be_ptask_destroy(&sdom->enum_task);

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

}

const char *get_flat_name_from_subdomain_name(struct be_ctx *be_ctx,

1.7.11.7

After working around a freeipa bug that didn't allow me to modify multiple trusts (#3994), I was able to verify the fix works as expected.

ACK

Reply

Pavel Březina

3:10 p.m.

----- Original Message -----

From: "Jakub Hrozek" jhrozek@redhat.com To: sssd-devel@lists.fedorahosted.org Sent: Wednesday, October 23, 2013 7:56:41 PM Subject: Re: [SSSD] [PATCH] subdomains: first destroy ptask than remove sdom

On Tue, Oct 22, 2013 at 04:24:04PM +0200, Pavel Březina wrote:

...
From 5906f84cf340b925ae63d72e9fcec9b3700f9f14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 22 Oct 2013 16:15:16 +0200 Subject: [PATCH 1/2] subdomains: first destroy ptask then remove sdom

be_ptask_destroy was unreachable since sdom is not present in the list of sdap domains any more.

src/providers/ipa/ipa_subdomains.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 6b88bc69bf069cc531785e7e569484e8d8572748..294af47de46b68ea84ec017f734c81fc44556423 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -289,12 +289,12 @@ ipa_ad_subdom_remove(struct ipa_subdomains_ctx *ctx, return; }

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

sdom = sdap_domain_get(iter->ad_id_ctx->sdap_id_ctx->opts, subdom); if (sdom == NULL) return; be_ptask_destroy(&sdom->enum_task);

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

}

const char *get_flat_name_from_subdomain_name(struct be_ctx *be_ctx,

1.7.11.7

After working around a freeipa bug that didn't allow me to modify multiple trusts (#3994), I was able to verify the fix works as expected.

ACK

Hi, there is another bug in this area. When a sdap domain is removed, its clean up task is still active. It is no big deal at the moment, but it is causing a crash when sdap domain is freed in my patches for #1968. I will probably convert it to ptask and destroy it here as well.

Reply

Jakub Hrozek

3:40 p.m.

On Wed, Oct 23, 2013 at 04:10:14PM -0400, Pavel Brezina wrote:

----- Original Message -----

...
From: "Jakub Hrozek" jhrozek@redhat.com To: sssd-devel@lists.fedorahosted.org Sent: Wednesday, October 23, 2013 7:56:41 PM Subject: Re: [SSSD] [PATCH] subdomains: first destroy ptask than remove sdom

On Tue, Oct 22, 2013 at 04:24:04PM +0200, Pavel Březina wrote:

...
From 5906f84cf340b925ae63d72e9fcec9b3700f9f14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 22 Oct 2013 16:15:16 +0200 Subject: [PATCH 1/2] subdomains: first destroy ptask then remove sdom

be_ptask_destroy was unreachable since sdom is not present in the list of sdap domains any more.

src/providers/ipa/ipa_subdomains.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 6b88bc69bf069cc531785e7e569484e8d8572748..294af47de46b68ea84ec017f734c81fc44556423 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -289,12 +289,12 @@ ipa_ad_subdom_remove(struct ipa_subdomains_ctx *ctx, return; }

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

sdom = sdap_domain_get(iter->ad_id_ctx->sdap_id_ctx->opts, subdom); if (sdom == NULL) return; be_ptask_destroy(&sdom->enum_task);

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

}

const char *get_flat_name_from_subdomain_name(struct be_ctx *be_ctx,

1.7.11.7

After working around a freeipa bug that didn't allow me to modify multiple trusts (#3994), I was able to verify the fix works as expected.

ACK

Hi, there is another bug in this area. When a sdap domain is removed, its clean up task is still active. It is no big deal at the moment, but it is causing a crash when sdap domain is freed in my patches for #1968. I will probably convert it to ptask and destroy it here as well.

Nice catch. I must admit I shut down SSSD after testing the patch, so I didn't see the cleanup task running after I removed the trust.

The solution itself depends on how fast this can be done. Please note that we should be releasing 1.11.2 tomorrow :-)

I took another look at the code and I'm wondering if we can have a wrapper around oneshot sync task in the be_ptask module? That would seem like the right thing to do, but I'm fine with doing the above in 1.12 and having just a smaller fix in 1.11.2

Reply

Pavel Březina

24 Oct 24 Oct

3:46 a.m.

On 10/23/2013 10:40 PM, Jakub Hrozek wrote:

On Wed, Oct 23, 2013 at 04:10:14PM -0400, Pavel Brezina wrote:

...
----- Original Message -----

...
From: "Jakub Hrozek" jhrozek@redhat.com To: sssd-devel@lists.fedorahosted.org Sent: Wednesday, October 23, 2013 7:56:41 PM Subject: Re: [SSSD] [PATCH] subdomains: first destroy ptask than remove sdom

On Tue, Oct 22, 2013 at 04:24:04PM +0200, Pavel Březina wrote:

...
From 5906f84cf340b925ae63d72e9fcec9b3700f9f14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 22 Oct 2013 16:15:16 +0200 Subject: [PATCH 1/2] subdomains: first destroy ptask then remove sdom

be_ptask_destroy was unreachable since sdom is not present in the list of sdap domains any more.

src/providers/ipa/ipa_subdomains.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 6b88bc69bf069cc531785e7e569484e8d8572748..294af47de46b68ea84ec017f734c81fc44556423 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -289,12 +289,12 @@ ipa_ad_subdom_remove(struct ipa_subdomains_ctx *ctx, return; }
sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);
sdom = sdap_domain_get(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);
if (sdom == NULL) return;
be_ptask_destroy(&sdom->enum_task);
sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter); }

const char *get_flat_name_from_subdomain_name(struct be_ctx *be_ctx,

-- 1.7.11.7
After working around a freeipa bug that didn't allow me to modify multiple trusts (#3994), I was able to verify the fix works as expected.

ACK
Hi, there is another bug in this area. When a sdap domain is removed, its clean up task is still active. It is no big deal at the moment, but it is causing a crash when sdap domain is freed in my patches for #1968. I will probably convert it to ptask and destroy it here as well.
Nice catch. I must admit I shut down SSSD after testing the patch, so I didn't see the cleanup task running after I removed the trust.

The solution itself depends on how fast this can be done. Please note that we should be releasing 1.11.2 tomorrow :-)

I took another look at the code and I'm wondering if we can have a wrapper around oneshot sync task in the be_ptask module?

I'm sorry, I don't know what you mean by this :-)

That would seem

like the right thing to do, but I'm fine with doing the above in 1.12 and having just a smaller fix in 1.11.2

Reply

Jakub Hrozek

3:53 a.m.

On Thu, Oct 24, 2013 at 10:46:18AM +0200, Pavel Březina wrote:

On 10/23/2013 10:40 PM, Jakub Hrozek wrote:

...
On Wed, Oct 23, 2013 at 04:10:14PM -0400, Pavel Brezina wrote:

...
----- Original Message -----

...
From: "Jakub Hrozek" jhrozek@redhat.com To: sssd-devel@lists.fedorahosted.org Sent: Wednesday, October 23, 2013 7:56:41 PM Subject: Re: [SSSD] [PATCH] subdomains: first destroy ptask than remove sdom

On Tue, Oct 22, 2013 at 04:24:04PM +0200, Pavel Březina wrote:

...
From 5906f84cf340b925ae63d72e9fcec9b3700f9f14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 22 Oct 2013 16:15:16 +0200 Subject: [PATCH 1/2] subdomains: first destroy ptask then remove sdom

be_ptask_destroy was unreachable since sdom is not present in the list of sdap domains any more.

src/providers/ipa/ipa_subdomains.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 6b88bc69bf069cc531785e7e569484e8d8572748..294af47de46b68ea84ec017f734c81fc44556423 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -289,12 +289,12 @@ ipa_ad_subdom_remove(struct ipa_subdomains_ctx *ctx, return; }

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

sdom = sdap_domain_get(iter->ad_id_ctx->sdap_id_ctx->opts, subdom); if (sdom == NULL) return; be_ptask_destroy(&sdom->enum_task);

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

}

const char *get_flat_name_from_subdomain_name(struct be_ctx *be_ctx,

1.7.11.7

After working around a freeipa bug that didn't allow me to modify multiple trusts (#3994), I was able to verify the fix works as expected.

ACK

Hi, there is another bug in this area. When a sdap domain is removed, its clean up task is still active. It is no big deal at the moment, but it is causing a crash when sdap domain is freed in my patches for #1968. I will probably convert it to ptask and destroy it here as well.

Nice catch. I must admit I shut down SSSD after testing the patch, so I didn't see the cleanup task running after I removed the trust.

The solution itself depends on how fast this can be done. Please note that we should be releasing 1.11.2 tomorrow :-)

I took another look at the code and I'm wondering if we can have a wrapper around oneshot sync task in the be_ptask module?

I'm sorry, I don't know what you mean by this :-)

Currently the ptask requires a pair of be_ptask_send_t,be_ptask_recv_t.

In contrast, the cleanup task is synchronous as it just operates on the cache. I was proposing creating a generic thin wrapper in the ptask module that you could use to setup a periodic task without having to create the wrapper for every synchronous periodic function.

Reply

Jakub Hrozek

9:32 a.m.

On Wed, Oct 23, 2013 at 07:56:41PM +0200, Jakub Hrozek wrote:

On Tue, Oct 22, 2013 at 04:24:04PM +0200, Pavel Březina wrote:

...
From 5906f84cf340b925ae63d72e9fcec9b3700f9f14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 22 Oct 2013 16:15:16 +0200 Subject: [PATCH 1/2] subdomains: first destroy ptask then remove sdom

be_ptask_destroy was unreachable since sdom is not present in the list of sdap domains any more.

src/providers/ipa/ipa_subdomains.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 6b88bc69bf069cc531785e7e569484e8d8572748..294af47de46b68ea84ec017f734c81fc44556423 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -289,12 +289,12 @@ ipa_ad_subdom_remove(struct ipa_subdomains_ctx *ctx, return; }

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

sdom = sdap_domain_get(iter->ad_id_ctx->sdap_id_ctx->opts, subdom); if (sdom == NULL) return; be_ptask_destroy(&sdom->enum_task);

sdap_domain_remove(iter->ad_id_ctx->sdap_id_ctx->opts, subdom);

DLIST_REMOVE(ctx->id_ctx->server_mode->trusts, iter);

}

const char *get_flat_name_from_subdomain_name(struct be_ctx *be_ctx,

1.7.11.7

After working around a freeipa bug that didn't allow me to modify multiple trusts (#3994), I was able to verify the fix works as expected.

ACK

Pushed to master and sssd-1-11

Reply

4737

Age (days ago)

4739

Last active (days ago)

sssd-devel@lists.fedorahosted.org

6 comments

2 participants

tags (0)

participants (2)

Jakub Hrozek
Pavel Březina