[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