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@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel
From 352e3b0e29831ca6f785f645360db34bc1a82f4e Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@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
From e79cbd7cb25e69efda83be9122f1c16073b2e236 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@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
From d550222679bc09bf784caa9b509ab21164c5df4a Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@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
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel