From acf735847001a50391edc2b04707910336b7a76e Mon Sep 17 00:00:00 2001 From: Stephen Gallagher Date: Thu, 13 Aug 2009 11:17:08 -0400 Subject: [PATCH 1/2] Don't go to the backend for identical cache entry requests Currently, if an additional request comes in for a cache entry while that same entry is already in the process of being refreshed, we start a duplicate cache update request. This patch adds allows the cache to maintain a hash table of all in-progress requests and queue up multiple callbacks for updates in progress. Once the data is returned, all of these callbacks will fire. Because requests can carry multiple callbacks, we cannot rely on the life of the current caller to manage the memory for the request. We'll manage the memory ourselves internally and remove the mem_ctx argument from nss_dp_send_acct_req() so there's no ambiguity. --- server/Makefile.am | 10 +- server/responder/common/responder.h | 5 +- server/responder/common/responder_dp.c | 335 ++++++++++++++++++++++++++------ server/responder/nss/nsssrv_cmd.c | 20 +- server/responder/pam/pamsrv_cmd.c | 6 +- 5 files changed, 306 insertions(+), 70 deletions(-) diff --git a/server/Makefile.am b/server/Makefile.am index 9616a39..8b55255 100644 --- a/server/Makefile.am +++ b/server/Makefile.am @@ -91,6 +91,12 @@ INI_CFG_LIBS = \ -L$(builddir)/../common/ini/.libs/ \ -lini_config +DHASH_CFLAGS = \ + -I$(srcdir)/../common/dhash +DHASH_LIBS = \ + -L$(builddir)/../common/dhash/.libs/ \ + -ldhash + AM_CPPFLAGS = -Wall \ -Iinclude \ -I.. \ @@ -107,7 +113,8 @@ AM_CPPFLAGS = -Wall \ $(PCRE_CFLAGS) \ $(REPLACE_CFLAGS) \ $(COLLECTION_CFLAGS) \ - $(INI_CFG_CFLAGS)\ + $(INI_CFG_CFLAGS) \ + $(DHASH_CFLAGS) \ -DLIBDIR=\"$(libdir)\" \ -DVARDIR=\"$(localstatedir)\" \ -DSHLIBEXT=\"$(SHLIBEXT)\" \ @@ -174,6 +181,7 @@ SSSD_LIBS = \ $(PCRE_LIBS) \ $(INI_CFG_LIBS) \ $(COLLECTION_LIBS) \ + $(DHASH_LIBS) \ $(REPLACE_LIBS) \ $(NSS_LIBS) \ libsss_crypt.la diff --git a/server/responder/common/responder.h b/server/responder/common/responder.h index 881c330..444056b 100644 --- a/server/responder/common/responder.h +++ b/server/responder/common/responder.h @@ -29,10 +29,13 @@ #include "talloc.h" #include "tevent.h" #include "ldb.h" +#include "dhash.h" #include "sbus/sssd_dbus.h" #include "../sss_client/sss_cli.h" #include "util/btreemap.h" +extern hash_table_t *dp_requests; + /* if there is a provider other than the special local */ #define NEED_CHECK_PROVIDER(provider) \ (provider != NULL && strcmp(provider, "local") != 0) @@ -132,7 +135,7 @@ int sss_dp_init(struct resp_ctx *rctx, struct sbus_interface *intf, typedef void (*nss_dp_callback_t)(uint16_t err_maj, uint32_t err_min, const char *err_msg, void *ptr); -int nss_dp_send_acct_req(struct resp_ctx *rctx, TALLOC_CTX *memctx, +int nss_dp_send_acct_req(struct resp_ctx *rctx, nss_dp_callback_t callback, void *callback_ctx, int timeout, const char *domain, int type, const char *opt_name, uint32_t opt_id); diff --git a/server/responder/common/responder_dp.c b/server/responder/common/responder_dp.c index 847bedb..ec476e7 100644 --- a/server/responder/common/responder_dp.c +++ b/server/responder/common/responder_dp.c @@ -21,6 +21,8 @@ struct sss_dp_pvt_ctx { int retries; }; +hash_table_t *dp_requests = NULL; + static int sss_dp_conn_destructor(void *data); static void sss_dp_reconnect(struct tevent_context *ev, struct tevent_timer *te, @@ -118,6 +120,7 @@ int sss_dp_init(struct resp_ctx *rctx, struct sbus_interface *dp_intf, uint16_t cli_type, uint16_t cli_version, const char *cli_name, const char *cli_domain) { + int ret; struct sss_dp_pvt_ctx *pvt; pvt = talloc_zero(rctx, struct sss_dp_pvt_ctx); @@ -132,79 +135,183 @@ int sss_dp_init(struct resp_ctx *rctx, struct sbus_interface *dp_intf, pvt->cli_domain = talloc_strdup(pvt, cli_domain); if (!pvt->cli_domain) return ENOMEM; + /* Create a hash table to handle queued update requests */ + ret = hash_create(10, &dp_requests, NULL); + if (ret != HASH_SUCCESS) { + fprintf(stderr, "cannot create hash table (%s)\n", hash_error_string(ret)); + talloc_zfree(pvt); + return EIO; + } + sss_dp_conn_reconnect(pvt); return EOK; } - -struct nss_dp_req { +struct nss_dp_callback { + struct nss_dp_callback *prev; + struct nss_dp_callback *next; nss_dp_callback_t callback; void *callback_ctx; - DBusPendingCall *pending_reply; }; -static int nss_dp_req_destructor(void *ptr) -{ - struct nss_dp_req *req = talloc_get_type(ptr, struct nss_dp_req); +struct nss_dp_req { + struct tevent_context *ev; + struct nss_dp_callback *cb_list; + DBusPendingCall *pending_reply; - if (req->pending_reply) { - dbus_pending_call_cancel(req->pending_reply); - } + char *key; + int32_t refcount; + dbus_uint16_t err_maj; + dbus_uint32_t err_min; + char *err_msg; +}; - return 0; -} +struct sss_dp_callback_ctx { + struct nss_dp_req *ndp_req; + struct nss_dp_callback *cb; +}; static int nss_dp_get_reply(DBusPendingCall *pending, dbus_uint16_t *err_maj, dbus_uint32_t *err_min, - const char **err_msg); + char **err_msg); + +static void sss_dp_invoke_callback(struct tevent_context *ev, + struct tevent_timer *te, + struct timeval t, void *ptr) +{ + struct sss_dp_callback_ctx *cb_ctx; + + cb_ctx = talloc_get_type(ptr, struct sss_dp_callback_ctx); + if (!cb_ctx) { + /* We didn't receive an sss_dp_callback_ctx? */ + return; + } + + cb_ctx->cb->callback(cb_ctx->ndp_req->err_maj, + cb_ctx->ndp_req->err_min, + cb_ctx->ndp_req->err_msg, + cb_ctx->cb->callback_ctx); + cb_ctx->ndp_req->refcount--; + if (cb_ctx->ndp_req->refcount <= 0) { + /* This was the last pending callback + * It should be safe to talloc_free ndp_req now. + * cb_ctx is a talloc child of ndp_req + */ + hash_key_t key; + key.type = HASH_KEY_STRING; + key.str = cb_ctx->ndp_req->key; + int hret = hash_delete(dp_requests, &key); + if (hret != HASH_SUCCESS) { + DEBUG(0, ("Could not clear entry from request queue\n")); + /* This should never happen */ + } + talloc_free(cb_ctx->ndp_req); + cb_ctx = NULL; + } + talloc_free(cb_ctx); /* talloc_free() is a noop on a NULL pointer */ +} static void nss_dp_send_acct_callback(DBusPendingCall *pending, void *ptr) { - struct nss_dp_req *ndp_req; - dbus_uint16_t err_maj; - dbus_uint32_t err_min; - const char *err_msg; int ret; + struct nss_dp_req *ndp_req; + struct nss_dp_callback *cb; + struct sss_dp_callback_ctx *cb_ctx; + struct timeval tv; + struct tevent_timer *te; ndp_req = talloc_get_type(ptr, struct nss_dp_req); /* Remove request destructor */ talloc_set_destructor(ndp_req, NULL); - ret = nss_dp_get_reply(pending, &err_maj, &err_min, &err_msg); + ret = nss_dp_get_reply(pending, + &ndp_req->err_maj, + &ndp_req->err_min, + &ndp_req->err_msg); if (ret != EOK) { if (ret == ETIME) { - err_maj = DP_ERR_TIMEOUT; - err_min = ret; - err_msg = "Request timed out"; + ndp_req->err_maj = DP_ERR_TIMEOUT; + ndp_req->err_min = ret; + ndp_req->err_msg = talloc_strdup(ndp_req, "Request timed out"); } else { - err_maj = DP_ERR_FATAL; - err_min = ret; - err_msg = "Failed to get reply from Data Provider"; + ndp_req->err_maj = DP_ERR_FATAL; + ndp_req->err_min = ret; + ndp_req->err_msg = + talloc_strdup(ndp_req, + "Failed to get reply from Data Provider"); } } - ndp_req->callback(err_maj, err_min, err_msg, ndp_req->callback_ctx); + /* Queue up all callbacks */ + cb = ndp_req->cb_list; + ndp_req->refcount = 0; + if (cb == NULL) { + /* No callbacks to invoke. Destroy the hash entry */ + hash_key_t key; + key.type = HASH_KEY_STRING; + key.str = ndp_req->key; + int hret = hash_delete(dp_requests, &key); + if (hret != HASH_SUCCESS) { + DEBUG(0, ("Could not clear entry from request queue\n")); + /* This should never happen */ + } + talloc_free(ndp_req); + return; + } + while(cb != NULL) { + cb_ctx = talloc_zero(ndp_req, struct sss_dp_callback_ctx); + if(!cb_ctx) { + /* Out of memory */ + goto error; + } + cb_ctx->ndp_req = ndp_req; + cb_ctx->ndp_req->refcount++; + cb_ctx->cb = cb; + + tv = tevent_timeval_current(); + te = tevent_add_timer(ndp_req->ev, cb_ctx, tv, + sss_dp_invoke_callback, cb_ctx); + if (!te) { + /* Out of memory or other serious error */ + goto error; + } + + cb = cb->next; + } + + return; + +error: talloc_free(ndp_req); } -int nss_dp_send_acct_req(struct resp_ctx *rctx, TALLOC_CTX *memctx, +static int nss_dp_send_acct_req_create(struct resp_ctx *rctx, + const char *domain, + uint32_t be_type, + char *filter, + int timeout, + nss_dp_callback_t callback, + void *callback_ctx, + struct nss_dp_req **ndp); + +int nss_dp_send_acct_req(struct resp_ctx *rctx, nss_dp_callback_t callback, void *callback_ctx, int timeout, const char *domain, int type, const char *opt_name, uint32_t opt_id) { - struct nss_dp_req *ndp_req; - DBusMessage *msg; - DBusPendingCall *pending_reply; - DBusConnection *dbus_conn; - dbus_bool_t ret; + int ret, hret; uint32_t be_type; - const char *attrs = "core"; char *filter; + hash_key_t key; + hash_value_t value; + TALLOC_CTX *tmp_ctx; + struct nss_dp_req *ndp_req; + struct nss_dp_callback *cb; /* either, or, not both */ if (opt_name && opt_id) { @@ -215,6 +322,11 @@ int nss_dp_send_acct_req(struct resp_ctx *rctx, TALLOC_CTX *memctx, return EINVAL; } + tmp_ctx = talloc_new(NULL); + if(!tmp_ctx) { + return ENOMEM; + } + switch (type) { case NSS_DP_USER: be_type = BE_REQ_USER; @@ -229,17 +341,111 @@ int nss_dp_send_acct_req(struct resp_ctx *rctx, TALLOC_CTX *memctx, return EINVAL; } + key.type = HASH_KEY_STRING; + key.str = NULL; + if (opt_name) { - filter = talloc_asprintf(memctx, "name=%s", opt_name); + filter = talloc_asprintf(tmp_ctx, "name=%s", opt_name); + key.str = talloc_asprintf(tmp_ctx, "%d%s@%s", type, opt_name, domain); } else if (opt_id) { - filter = talloc_asprintf(memctx, "idnumber=%u", opt_id); + filter = talloc_asprintf(tmp_ctx, "idnumber=%u", opt_id); + key.str = talloc_asprintf(tmp_ctx, "%d%d@%s", type, opt_id, domain); } else { - filter = talloc_strdup(memctx, "name=*"); + filter = talloc_strdup(tmp_ctx, "name=*"); + key.str = talloc_asprintf(tmp_ctx, "%d*@%s", type, domain); } - if (!filter) { + if (!filter || !key.str) { + talloc_free(tmp_ctx); return ENOMEM; } + /* Check whether there's already a request in progress */ + hret = hash_lookup(dp_requests, &key, &value); + switch (hret) { + case HASH_SUCCESS: + /* Request already in progress + * Add an additional callback if needed and return + */ + DEBUG(1, ("Identical request in progress\n")); + if(callback) { + /* We have a new request asking for a callback */ + ndp_req = talloc_get_type(value.ptr, struct nss_dp_req); + if(!ndp_req) { + DEBUG(0, ("Could not retrieve DP request context\n")); + ret = EIO; + goto done; + } + + cb = talloc_zero(ndp_req, struct nss_dp_callback); + if (!cb) { + ret = ENOMEM; + goto done; + } + + cb->callback = callback; + cb->callback_ctx = callback_ctx; + + DLIST_ADD(ndp_req->cb_list, cb); + } + ret = EOK; + goto done; + + case HASH_ERROR_KEY_NOT_FOUND: + /* No such request in progress + * Create a new request + */ + ret = nss_dp_send_acct_req_create(rctx, domain, be_type, + filter, timeout, + callback, callback_ctx, + &ndp_req); + if (ret == EOK) { + value.type = HASH_VALUE_PTR; + value.ptr = ndp_req; + hret = hash_enter(dp_requests, &key, &value); + if (hret != HASH_SUCCESS) { + DEBUG(0, ("Could not store request query (%s)", + hash_error_string(hret))); + talloc_free(tmp_ctx); + ret = EIO; + goto done; + } + + ndp_req->key = talloc_strdup(ndp_req, key.str); + } + break; + + default: + DEBUG(0,("Could not query request list (%s)\n", + hash_error_string(hret))); + ret = EIO; + goto done; + } + + ret = EOK; + +done: + talloc_free(tmp_ctx); + return ret; +} + +static int nss_dp_send_acct_req_create(struct resp_ctx *rctx, + const char *domain, + uint32_t be_type, + char *filter, + int timeout, + nss_dp_callback_t callback, + void *callback_ctx, + struct nss_dp_req **ndp) +{ + DBusConnection *dbus_conn; + DBusMessage *msg; + DBusPendingCall *pending_reply; + dbus_bool_t dbret; + struct nss_dp_callback *cb; + struct nss_dp_req *ndp_req; + + const char *attrs = "core"; + /* double check dp_ctx has actually been initialized. * in some pathological cases it may happen that nss starts up before * dp connection code is actually able to establish a connection. @@ -264,20 +470,20 @@ int nss_dp_send_acct_req(struct resp_ctx *rctx, TALLOC_CTX *memctx, DEBUG(4, ("Sending request for [%s][%u][%s][%s]\n", domain, be_type, attrs, filter)); - ret = dbus_message_append_args(msg, - DBUS_TYPE_STRING, &domain, - DBUS_TYPE_UINT32, &be_type, - DBUS_TYPE_STRING, &attrs, - DBUS_TYPE_STRING, &filter, - DBUS_TYPE_INVALID); - if (!ret) { + dbret = dbus_message_append_args(msg, + DBUS_TYPE_STRING, &domain, + DBUS_TYPE_UINT32, &be_type, + DBUS_TYPE_STRING, &attrs, + DBUS_TYPE_STRING, &filter, + DBUS_TYPE_INVALID); + if (!dbret) { DEBUG(1,("Failed to build message\n")); return EIO; } - ret = dbus_connection_send_with_reply(dbus_conn, msg, - &pending_reply, timeout); - if (!ret || pending_reply == NULL) { + dbret = dbus_connection_send_with_reply(dbus_conn, msg, + &pending_reply, timeout); + if (!dbret || pending_reply == NULL) { /* * Critical Failure * We can't communicate on this connection @@ -288,31 +494,50 @@ int nss_dp_send_acct_req(struct resp_ctx *rctx, TALLOC_CTX *memctx, return EIO; } - ndp_req = talloc_zero(memctx, struct nss_dp_req); + ndp_req = talloc_zero(NULL, struct nss_dp_req); if (!ndp_req) { dbus_message_unref(msg); return ENOMEM; } - ndp_req->callback = callback; - ndp_req->callback_ctx = callback_ctx; - /* set up destructor */ - ndp_req->pending_reply = pending_reply; - talloc_set_destructor((TALLOC_CTX *)ndp_req, nss_dp_req_destructor); + ndp_req->ev = rctx->ev; + + if (callback) { + cb = talloc_zero(ndp_req, struct nss_dp_callback); + if (!cb) { + dbus_message_unref(msg); + talloc_free(ndp_req); + return ENOMEM; + } + cb->callback = callback; + cb->callback_ctx = callback_ctx; + + DLIST_ADD(ndp_req->cb_list, cb); + } /* Set up the reply handler */ - dbus_pending_call_set_notify(pending_reply, - nss_dp_send_acct_callback, - ndp_req, NULL); + dbret = dbus_pending_call_set_notify(pending_reply, + nss_dp_send_acct_callback, + ndp_req, NULL); + if (!dbret) { + DEBUG(0, ("Could not queue up pending request!")); + talloc_free(ndp_req); + dbus_pending_call_cancel(pending_reply); + dbus_message_unref(msg); + return EIO; + } + dbus_message_unref(msg); + *ndp = ndp_req; + return EOK; } static int nss_dp_get_reply(DBusPendingCall *pending, dbus_uint16_t *err_maj, dbus_uint32_t *err_min, - const char **err_msg) + char **err_msg) { DBusMessage *reply; DBusError dbus_error; diff --git a/server/responder/nss/nsssrv_cmd.c b/server/responder/nss/nsssrv_cmd.c index 2cfdd99..6fe2a49 100644 --- a/server/responder/nss/nsssrv_cmd.c +++ b/server/responder/nss/nsssrv_cmd.c @@ -331,7 +331,7 @@ static void nss_cmd_getpwnam_callback(void *ptr, int status, dctx->res = talloc_steal(dctx, res); } - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_getpwnam_dp_callback, dctx, timeout, dctx->domain->name, NSS_DP_USER, cmdctx->name, 0); @@ -703,7 +703,7 @@ static void nss_cmd_getpwuid_callback(void *ptr, int status, dctx->res = talloc_steal(dctx, res); } - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_getpwuid_dp_callback, dctx, timeout, dctx->domain->name, NSS_DP_USER, NULL, cmdctx->id); @@ -1035,7 +1035,7 @@ static void nss_cmd_setpwent_callback(void *ptr, int status, if (dctx->check_provider) { timeout = SSS_CLI_SOCKET_TIMEOUT; - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_setpw_dp_callback, dctx, timeout, dom->name, NSS_DP_USER, NULL, 0); @@ -1168,7 +1168,7 @@ static int nss_cmd_setpwent_ext(struct cli_ctx *cctx, bool immediate) if (dctx->check_provider) { timeout = SSS_CLI_SOCKET_TIMEOUT; - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_setpw_dp_callback, dctx, timeout, dom->name, NSS_DP_USER, NULL, 0); @@ -1728,7 +1728,7 @@ static void nss_cmd_getgrnam_callback(void *ptr, int status, dctx->res = talloc_steal(dctx, res); } - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_getgrnam_dp_callback, dctx, timeout, dctx->domain->name, NSS_DP_GROUP, cmdctx->name, 0); @@ -2085,7 +2085,7 @@ static void nss_cmd_getgrgid_callback(void *ptr, int status, dctx->res = talloc_steal(dctx, res); } - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_getgrgid_dp_callback, dctx, timeout, dctx->domain->name, NSS_DP_GROUP, NULL, cmdctx->id); @@ -2408,7 +2408,7 @@ static void nss_cmd_setgrent_callback(void *ptr, int status, if (dctx->check_provider) { timeout = SSS_CLI_SOCKET_TIMEOUT; - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_setgr_dp_callback, dctx, timeout, dom->name, NSS_DP_GROUP, NULL, 0); @@ -2541,7 +2541,7 @@ static int nss_cmd_setgrent_ext(struct cli_ctx *cctx, bool immediate) if (dctx->check_provider) { timeout = SSS_CLI_SOCKET_TIMEOUT; - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_setgr_dp_callback, dctx, timeout, dom->name, NSS_DP_GROUP, NULL, 0); @@ -2908,7 +2908,7 @@ static void nss_cmd_getinit_callback(void *ptr, int status, dctx->res = talloc_steal(dctx, res); } - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_getinitnam_dp_callback, dctx, timeout, dctx->domain->name, NSS_DP_USER, cmdctx->name, 0); @@ -3002,7 +3002,7 @@ static void nss_cmd_getinit_callback(void *ptr, int status, case 1: timeout = SSS_CLI_SOCKET_TIMEOUT/2; - ret = nss_dp_send_acct_req(cctx->rctx, cmdctx, + ret = nss_dp_send_acct_req(cctx->rctx, nss_cmd_getinitgr_callback, dctx, timeout, dctx->domain->name, NSS_DP_INITGROUPS, diff --git a/server/responder/pam/pamsrv_cmd.c b/server/responder/pam/pamsrv_cmd.c index 9b02146..8900b46 100644 --- a/server/responder/pam/pamsrv_cmd.c +++ b/server/responder/pam/pamsrv_cmd.c @@ -478,7 +478,7 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd) preq->check_provider = false; timeout = SSS_CLI_SOCKET_TIMEOUT/2; - ret = nss_dp_send_acct_req(preq->cctx->rctx, preq, + ret = nss_dp_send_acct_req(preq->cctx->rctx, pam_check_user_dp_callback, preq, timeout, preq->domain->name, NSS_DP_USER, preq->pd->user, 0); @@ -595,7 +595,7 @@ static void pam_check_user_callback(void *ptr, int status, preq->data = talloc_steal(preq, res); } - ret = nss_dp_send_acct_req(preq->cctx->rctx, preq, + ret = nss_dp_send_acct_req(preq->cctx->rctx, pam_check_user_dp_callback, preq, timeout, preq->domain->name, NSS_DP_USER, preq->pd->user, 0); @@ -663,7 +663,7 @@ static void pam_check_user_callback(void *ptr, int status, /* no need to re-check later on */ preq->check_provider = false; - ret = nss_dp_send_acct_req(preq->cctx->rctx, preq, + ret = nss_dp_send_acct_req(preq->cctx->rctx, pam_check_user_dp_callback, preq, timeout, preq->domain->name, -- 1.6.2.5