[SSSD] [PATCH] LDAP: Setup periodic task only once.

Lukas Slebodnik lslebodn at redhat.com
Tue Feb 25 19:05:45 UTC 2014


On (24/02/14 20:45), Jakub Hrozek wrote:
>On Mon, Feb 24, 2014 at 08:34:35PM +0100, Jakub Hrozek wrote:
>> On Sat, Feb 22, 2014 at 12:18:01AM +0100, Lukas Slebodnik wrote:
>> > ehlo,
>> > 
>> > If id provider is {ipa, ad} periodic task will be stared in sssm_{ipa,ad}_init
>> > If you enable enumeration and use different providers for id and sudo(autofs)
>> > then another periodic task will be scheduled.
>> > This can cause weird behaviour (e.g. missing members of group)
>> > 
>> > I provided test package to reporter of bug #2153 with attached patch
>> > (actually it was patch for 1.9 branch). I was not able to reproduce problem
>> > with missing groups. Thus I was wainting for response from customer.
>> > But it will be better to do a (pre-)review of patch.
>> > 
>> > I am also attaching part of log file. You can notice Two enumerations are
>> > started. There is difference only few milliseconds.
>> > 
>> > LS
>> 
>> I admit I would have preferred an idempotent solution that would just
>> result in a noop if called twice (maybe checking if either cleanup or enum
>> is loaded), but after some time thinking about the solution I don't think
>> it would be so easy.
>> 
>> Could you just add a comment to ldap_id_init_internal() explaining what
>> the main_init does and another comment to sssm_ldap_id_init() reminding
>> the developer that changes should go directly to ldap_id_init_internal()
>> ?
>
>Or what about moving the periodic task setup from ldap_id_init_internal()
>to sssm_ldap_id_init() after the internal init is finished? That way we'd
>be able to drop the boolean parameter as well.

As you wish master.

updated patch is attached.

LS
-------------- next part --------------
>From d8b2afd1ad42189663f3722416d90e61f47a2905 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Wed, 12 Feb 2014 14:33:49 +0100
Subject: [PATCH] LDAP: Setup periodic task only once.

If id provider is {ipa, ad} periodic task will be stared in sssm_{ipa,ad}_init
If you enable enumeration and use different providers for id and sudo(autofs)
then another periodic task will be scheduled.
This can cause weird behaviour (e.g. missing members of group)

Resolves:
https://fedorahosted.org/sssd/ticket/2153
---
 src/providers/ldap/ldap_init.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/src/providers/ldap/ldap_init.c b/src/providers/ldap/ldap_init.c
index a14e6ceaecdd5a72e1d28c8cb14782cdc6c2d54a..a1cc0c8f0d2585124251746b894abd4889e9211d 100644
--- a/src/providers/ldap/ldap_init.c
+++ b/src/providers/ldap/ldap_init.c
@@ -85,9 +85,10 @@ errno_t check_order_list_for_duplicates(char **list,
     return EOK;
 }
 
-int sssm_ldap_id_init(struct be_ctx *bectx,
-                      struct bet_ops **ops,
-                      void **pvt_data)
+static int ldap_id_init_internal(struct be_ctx *bectx,
+                                 struct bet_ops **ops,
+                                 void **pvt_data,
+                                 bool main_init)
 {
     struct sdap_id_ctx *ctx = NULL;
     const char *urls;
@@ -163,9 +164,14 @@ int sssm_ldap_id_init(struct be_ctx *bectx,
     ret = sdap_idmap_init(ctx, ctx, &ctx->opts->idmap_ctx);
     if (ret != EOK) goto done;
 
-    ret = ldap_id_setup_tasks(ctx);
-    if (ret != EOK) {
-        goto done;
+    if (main_init) {
+        ret = ldap_id_setup_tasks(ctx);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  "sdap_id_setup_tasks failed [%d][%s].\n",
+                  ret, strerror(ret));
+            goto done;
+        }
     }
 
     ret = sdap_setup_child();
@@ -205,6 +211,13 @@ done:
     return ret;
 }
 
+int sssm_ldap_id_init(struct be_ctx *bectx,
+                      struct bet_ops **ops,
+                      void **pvt_data)
+{
+    return ldap_id_init_internal(bectx, ops, pvt_data, true);
+}
+
 int sssm_ldap_auth_init(struct be_ctx *bectx,
                         struct bet_ops **ops,
                         void **pvt_data)
@@ -214,7 +227,7 @@ int sssm_ldap_auth_init(struct be_ctx *bectx,
     struct sdap_auth_ctx *ctx;
     int ret;
 
-    ret = sssm_ldap_id_init(bectx, ops, &data);
+    ret = ldap_id_init_internal(bectx, ops, &data, false);
     if (ret == EOK) {
         id_ctx = talloc_get_type(data, struct sdap_id_ctx);
 
@@ -308,9 +321,10 @@ int sssm_ldap_access_init(struct be_ctx *bectx,
         goto done;
     }
 
-    ret = sssm_ldap_id_init(bectx, ops, (void **)&access_ctx->id_ctx);
+    ret = ldap_id_init_internal(bectx, ops, (void **)&access_ctx->id_ctx,
+                                false);
     if (ret != EOK) {
-        DEBUG(SSSDBG_CRIT_FAILURE, "sssm_ldap_id_init failed.\n");
+        DEBUG(SSSDBG_CRIT_FAILURE, "ldap_id_init_internal failed.\n");
         goto done;
     }
 
@@ -428,7 +442,7 @@ int sssm_ldap_sudo_init(struct be_ctx *be_ctx,
     void *data;
     int ret;
 
-    ret = sssm_ldap_id_init(be_ctx, ops, &data);
+    ret = ldap_id_init_internal(be_ctx, ops, &data, false);
     if (ret != EOK) {
         DEBUG(SSSDBG_CRIT_FAILURE, "Cannot init LDAP ID provider [%d]: %s\n",
                                     ret, strerror(ret));
@@ -458,7 +472,7 @@ int sssm_ldap_autofs_init(struct be_ctx *be_ctx,
     void *data;
     int ret;
 
-    ret = sssm_ldap_id_init(be_ctx, ops, &data);
+    ret = ldap_id_init_internal(be_ctx, ops, &data, false);
     if (ret != EOK) {
         DEBUG(SSSDBG_CRIT_FAILURE, "Cannot init LDAP ID provider [%d]: %s\n",
                                     ret, strerror(ret));
-- 
1.8.5.3



More information about the sssd-devel mailing list