[SSSD] [PATCHES] Enumeration fixes

Sumit Bose sbose at redhat.com
Thu Jan 6 11:11:24 UTC 2011


On Tue, Jan 04, 2011 at 12:08:01PM -0500, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 01/04/2011 08:18 AM, Sumit Bose wrote:
> > Hi,
> > 
> > based on an issue found by Shanks (#757) I had a closer look at some
> > parts of the enumeration code in the nss responder and found a different
> > issue (#758) and some non functional code.
> > 
> > 0001: should fix #758.
> > 
> 
> Ack
> 
> > 0002: should fix #757. If only the local provider was configured the
> > code did not return to the main loop during execution and the callback
> > of the corresponding tevent request was never called.I think this is a
> > regression from the introduction of the synchronous sysdb interface. To
> > fix it I added a timer event which just calls *_step() again to make
> > sure we always return to the main loop at least once. Of someone knows
> > of a more elegant solution I'm all ears. Btw. the code cries for
> > refactoring.
> > 
> 
> We have a convention we use in other similar cases. Instead of creating
> a new event, just change the return values from nss_cmd_set*ent_step()
> to use EOK for "return with immediate post" and to EAGAIN for "return
> and re-enter mainloop"

ok, I've changed the patch to use this convention. New and renamed
version attached.

bye,
Sumit

> 
> 
> > 0003: the code arround last_user_enum and last_group_enum was not
> > functional anymore and the enumeration timeout is handled by
> > *_result_timeout().
> > 
> 
> Ack

> 
> - -- 
> Stephen Gallagher
> RHCE 804006346421761
> 
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
> 
> iEYEARECAAYFAk0jU/EACgkQeiVVYja6o6NYZgCffKDXINpPAAwYbtHC1GkqtmSV
> NJ0An0Laf5iiFiYJdpVoLBAZ9YC45Oi9
> =UQ6q
> -----END PGP SIGNATURE-----
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
From 352e3b0e29831ca6f785f645360db34bc1a82f4e Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Mon, 3 Jan 2011 16:42:00 +0100
Subject: [PATCH 1/3] Return groups and users from all domains during enumeration

---
 src/responder/nss/nsssrv_cmd.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 65d9c58..2decf84 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -1386,7 +1386,8 @@ static int nss_cmd_retpwent(struct cli_ctx *cctx, int num)
         if (n <= 0 && (cctx->pwent_dom_idx+1 < pctx->num)) {
             cctx->pwent_dom_idx++;
             pdom = &pctx->doms[cctx->pwent_dom_idx];
-            n = pdom->res->count - cctx->pwent_cur;
+            n = pdom->res->count;
+            cctx->pwent_cur = 0;
         }
 
         if (!n) break;
@@ -2564,10 +2565,11 @@ static int nss_cmd_retgrent(struct cli_ctx *cctx, int num)
         gdom = &gctx->doms[cctx->grent_dom_idx];
 
         n = gdom->res->count - cctx->grent_cur;
-        if (n <= 0 && (cctx->grent_cur+1 < gctx->num)) {
+        if (n <= 0 && (cctx->grent_dom_idx+1 < gctx->num)) {
             cctx->grent_dom_idx++;
             gdom = &gctx->doms[cctx->grent_dom_idx];
-            n = gdom->res->count - cctx->grent_cur;
+            n = gdom->res->count;
+            cctx->grent_cur = 0;
         }
 
         if (!n) break;
-- 
1.7.3.3

-------------- next part --------------
From e79cbd7cb25e69efda83be9122f1c16073b2e236 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Thu, 6 Jan 2011 11:20:02 +0100
Subject: [PATCH 2/3] Post enumeration tevent request if needed

---
 src/responder/nss/nsssrv_cmd.c     |   49 ++++++++++++++++++++++++++++++------
 src/responder/nss/nsssrv_private.h |    2 +
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 2decf84..c9c7920 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -1081,9 +1081,14 @@ struct tevent_req *nss_cmd_setpwent_send(TALLOC_CTX *mem_ctx,
     step_ctx->getent_ctx = state->getent_ctx;
     step_ctx->rctx = client->rctx;
     step_ctx->enum_cached = enum_cached;
+    step_ctx->returned_to_mainloop = false;
 
     ret = nss_cmd_setpwent_step(step_ctx);
-    if (ret != EOK) goto error;
+    if (ret != EOK && ret != EAGAIN) goto error;
+
+    if (ret == EOK) {
+        tevent_req_post(req, client->rctx->ev);
+    }
 
     return req;
 
@@ -1099,6 +1104,11 @@ static void setpwent_result_timeout(struct tevent_context *ev,
                                     struct tevent_timer *te,
                                     struct timeval current_time,
                                     void *pvt);
+
+/* nss_cmd_setpwent_step returns
+ *   EOK if everything is done and the request needs to be posted explicitly
+ *   EAGAIN if the caller can safely return to the main loop
+ */
 static errno_t nss_cmd_setpwent_step(struct setent_step_ctx *step_ctx)
 {
     errno_t ret;
@@ -1145,6 +1155,7 @@ static errno_t nss_cmd_setpwent_step(struct setent_step_ctx *step_ctx)
         /* if this is a caching provider (or if we haven't checked the cache
          * yet) then verify that the cache is uptodate */
         if (dctx->check_provider) {
+            step_ctx->returned_to_mainloop = true;
             /* Only do this once per provider */
             dctx->check_provider = false;
             timeout = SSS_CLI_SOCKET_TIMEOUT;
@@ -1154,7 +1165,7 @@ static errno_t nss_cmd_setpwent_step(struct setent_step_ctx *step_ctx)
                                        timeout, dom->name, true,
                                        SSS_DP_USER, NULL, 0);
             if (ret == EOK) {
-                return ret;
+                return EAGAIN;
             } else {
                 DEBUG(2, ("Enum Cache refresh for domain [%s] failed."
                           " Trying to return what we have in cache!\n",
@@ -1218,7 +1229,12 @@ static errno_t nss_cmd_setpwent_step(struct setent_step_ctx *step_ctx)
         nctx->pctx->reqs = nctx->pctx->reqs->next;
         talloc_free(req);
     }
-    return EOK;
+
+    if (step_ctx->returned_to_mainloop) {
+        return EAGAIN;
+    } else {
+        return EOK;
+    }
 }
 
 static void setpwent_result_timeout(struct tevent_context *ev,
@@ -1253,7 +1269,7 @@ static void nss_cmd_setpwent_dp_callback(uint16_t err_maj, uint32_t err_min,
     }
 
     ret = nss_cmd_setpwent_step(step_ctx);
-    if (ret) {
+    if (ret != EOK && ret != EAGAIN) {
         /* Notify any waiting processes of failure */
         while(step_ctx->nctx->pctx->reqs) {
             tevent_req_error(pctx->reqs->req, ret);
@@ -2334,9 +2350,14 @@ struct tevent_req *nss_cmd_setgrent_send(TALLOC_CTX *mem_ctx,
     step_ctx->getent_ctx = state->getent_ctx;
     step_ctx->rctx = client->rctx;
     step_ctx->enum_cached = enum_cached;
+    step_ctx->returned_to_mainloop = false;
 
     ret = nss_cmd_setgrent_step(step_ctx);
-    if (ret != EOK) goto error;
+    if (ret != EOK && ret != EAGAIN) goto error;
+
+    if (ret == EOK) {
+        tevent_req_post(req, client->rctx->ev);
+    }
 
     return req;
 
@@ -2352,6 +2373,11 @@ static void setgrent_result_timeout(struct tevent_context *ev,
                                     struct tevent_timer *te,
                                     struct timeval current_time,
                                     void *pvt);
+
+/* nss_cmd_setgrent_step returns
+ *   EOK if everything is done and the request needs to be posted explicitly
+ *   EAGAIN if the caller can safely return to the main loop
+ */
 static errno_t nss_cmd_setgrent_step(struct setent_step_ctx *step_ctx)
 {
     errno_t ret;
@@ -2398,6 +2424,7 @@ static errno_t nss_cmd_setgrent_step(struct setent_step_ctx *step_ctx)
         /* if this is a caching provider (or if we haven't checked the cache
          * yet) then verify that the cache is uptodate */
         if (dctx->check_provider) {
+            step_ctx->returned_to_mainloop = true;
             /* Only do this once per provider */
             dctx->check_provider = false;
             timeout = SSS_CLI_SOCKET_TIMEOUT;
@@ -2407,7 +2434,7 @@ static errno_t nss_cmd_setgrent_step(struct setent_step_ctx *step_ctx)
                                        timeout, dom->name, true,
                                        SSS_DP_GROUP, NULL, 0);
             if (ret == EOK) {
-                return ret;
+                return EAGAIN;
             } else {
                 DEBUG(2, ("Enum Cache refresh for domain [%s] failed."
                           " Trying to return what we have in cache!\n",
@@ -2471,7 +2498,13 @@ static errno_t nss_cmd_setgrent_step(struct setent_step_ctx *step_ctx)
         nctx->gctx->reqs = nctx->gctx->reqs->next;
         talloc_free(req);
     }
-    return EOK;
+
+    if (step_ctx->returned_to_mainloop) {
+        return EAGAIN;
+    } else {
+        return EOK;
+    }
+
 }
 
 static void setgrent_result_timeout(struct tevent_context *ev,
@@ -2506,7 +2539,7 @@ static void nss_cmd_setgrent_dp_callback(uint16_t err_maj, uint32_t err_min,
     }
 
     ret = nss_cmd_setgrent_step(step_ctx);
-    if (ret) {
+    if (ret != EOK && ret != EAGAIN) {
         /* Notify any waiting processes of failure */
         while(step_ctx->nctx->gctx->reqs) {
             tevent_req_error(gctx->reqs->req, ret);
diff --git a/src/responder/nss/nsssrv_private.h b/src/responder/nss/nsssrv_private.h
index 4d9f947..3eaaa9b 100644
--- a/src/responder/nss/nsssrv_private.h
+++ b/src/responder/nss/nsssrv_private.h
@@ -87,6 +87,8 @@ struct setent_step_ctx {
     bool enum_cached;
     bool check_next;
 
+    bool returned_to_mainloop;
+
     /* Netgroup-specific */
     char *name;
 };
-- 
1.7.3.3

-------------- next part --------------
From d550222679bc09bf784caa9b509ab21164c5df4a Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Tue, 4 Jan 2011 10:38:40 +0100
Subject: [PATCH 3/3] Remove unused enumeration cache timeout checks

The existence of the getent_ctx is used to track the enumeration cache
timeout.
---
 src/responder/nss/nsssrv.h         |    2 --
 src/responder/nss/nsssrv_cmd.c     |   32 ++------------------------------
 src/responder/nss/nsssrv_private.h |    1 -
 3 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/src/responder/nss/nsssrv.h b/src/responder/nss/nsssrv.h
index 0b7265b..062d937 100644
--- a/src/responder/nss/nsssrv.h
+++ b/src/responder/nss/nsssrv.h
@@ -49,8 +49,6 @@ struct nss_ctx {
     int cache_refresh_percent;
 
     int enum_cache_timeout;
-    time_t last_user_enum;
-    time_t last_group_enum;
 
     struct getent_ctx *pctx;
     struct getent_ctx *gctx;
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index c9c7920..2153d64 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -981,8 +981,6 @@ struct tevent_req *nss_cmd_setpwent_send(TALLOC_CTX *mem_ctx,
     struct setent_ctx *state;
     struct sss_domain_info *dom;
     struct setent_step_ctx *step_ctx;
-    bool enum_cached = false;
-    time_t now = time(NULL);
 
     DEBUG(4, ("Received setpwent request\n"));
     nctx = talloc_get_type(client->rctx->pvt_ctx, struct nss_ctx);
@@ -1000,13 +998,6 @@ struct tevent_req *nss_cmd_setpwent_send(TALLOC_CTX *mem_ctx,
     state->nctx = nctx;
     state->client = client;
 
-    /* do not query backends if we have a recent enumeration */
-    if (nctx->enum_cache_timeout) {
-        if (nctx->last_user_enum + nctx->enum_cache_timeout > now) {
-            enum_cached = true;
-        }
-    }
-
     state->dctx = talloc_zero(state, struct nss_dom_ctx);
     if (!state->dctx) {
         ret = ENOMEM;
@@ -1080,7 +1071,6 @@ struct tevent_req *nss_cmd_setpwent_send(TALLOC_CTX *mem_ctx,
     step_ctx->nctx = state->nctx;
     step_ctx->getent_ctx = state->getent_ctx;
     step_ctx->rctx = client->rctx;
-    step_ctx->enum_cached = enum_cached;
     step_ctx->returned_to_mainloop = false;
 
     ret = nss_cmd_setpwent_step(step_ctx);
@@ -1134,11 +1124,7 @@ static errno_t nss_cmd_setpwent_step(struct setent_step_ctx *step_ctx)
         if (dom != dctx->domain) {
             /* make sure we reset the check_provider flag when we check
              * a new domain */
-            if (step_ctx->enum_cached) {
-                dctx->check_provider = false;
-            } else {
-                dctx->check_provider = NEED_CHECK_PROVIDER(dom->provider);
-            }
+            dctx->check_provider = NEED_CHECK_PROVIDER(dom->provider);
         }
 
         /* make sure to update the dctx if we changed domain */
@@ -2250,8 +2236,6 @@ struct tevent_req *nss_cmd_setgrent_send(TALLOC_CTX *mem_ctx,
     struct setent_ctx *state;
     struct sss_domain_info *dom;
     struct setent_step_ctx *step_ctx;
-    bool enum_cached = false;
-    time_t now = time(NULL);
 
     DEBUG(4, ("Received setgrent request\n"));
     nctx = talloc_get_type(client->rctx->pvt_ctx, struct nss_ctx);
@@ -2269,13 +2253,6 @@ struct tevent_req *nss_cmd_setgrent_send(TALLOC_CTX *mem_ctx,
     state->nctx = nctx;
     state->client = client;
 
-    /* do not query backends if we have a recent enumeration */
-    if (nctx->enum_cache_timeout) {
-        if (nctx->last_user_enum + nctx->enum_cache_timeout > now) {
-            enum_cached = true;
-        }
-    }
-
     state->dctx = talloc_zero(state, struct nss_dom_ctx);
     if (!state->dctx) {
         ret = ENOMEM;
@@ -2349,7 +2326,6 @@ struct tevent_req *nss_cmd_setgrent_send(TALLOC_CTX *mem_ctx,
     step_ctx->nctx = state->nctx;
     step_ctx->getent_ctx = state->getent_ctx;
     step_ctx->rctx = client->rctx;
-    step_ctx->enum_cached = enum_cached;
     step_ctx->returned_to_mainloop = false;
 
     ret = nss_cmd_setgrent_step(step_ctx);
@@ -2403,11 +2379,7 @@ static errno_t nss_cmd_setgrent_step(struct setent_step_ctx *step_ctx)
         if (dom != dctx->domain) {
             /* make sure we reset the check_provider flag when we check
              * a new domain */
-            if (step_ctx->enum_cached) {
-                dctx->check_provider = false;
-            } else {
-                dctx->check_provider = NEED_CHECK_PROVIDER(dom->provider);
-            }
+            dctx->check_provider = NEED_CHECK_PROVIDER(dom->provider);
         }
 
         /* make sure to update the dctx if we changed domain */
diff --git a/src/responder/nss/nsssrv_private.h b/src/responder/nss/nsssrv_private.h
index 3eaaa9b..52c4ace 100644
--- a/src/responder/nss/nsssrv_private.h
+++ b/src/responder/nss/nsssrv_private.h
@@ -84,7 +84,6 @@ struct setent_step_ctx {
     struct nss_dom_ctx *dctx;
     struct getent_ctx *getent_ctx;
     struct resp_ctx *rctx;
-    bool enum_cached;
     bool check_next;
 
     bool returned_to_mainloop;
-- 
1.7.3.3



More information about the sssd-devel mailing list