> On Fri, 2011-10-14 at 16:10 +0200, Jan Zelený wrote: > > https://fedorahosted.org/sssd/ticket/1038 > > > > With the change I also implemented what Stephen suggested. I schedule all > > callbacks to fire at tevent_timeval_current() directly, instead of > > scheduling them one by one. > > Nack. > > Don't remove the debug message in sss_dp_req_destructor() if > hash_delete() fails. > > > > There's no point to the block: > /* Free any remaining callback */ > if (sdp_req->err_maj == DP_ERR_OK) { > sdp_req->err_maj = DP_ERR_FATAL; > sdp_req->err_min = EIO; > sdp_req->err_msg = discard_const_p(char, "Internal Error"); > } > Because we're no longer passing this info into callbacks (and sdp_req is > being freed). > > > Otherwise it looks fine. No obvious regressions while testing, either.
Thanks for that tip, I'm sending updated patch. Jan
From 43ceffafd7a0f945f4702e03b3289e8003e62b25 Mon Sep 17 00:00:00 2001 From: Jan Zeleny <[email protected]> Date: Thu, 13 Oct 2011 10:52:56 -0400 Subject: [PATCH] Fixed timeout handling in responders --- src/responder/common/responder_dp.c | 146 +++++++++++++++++------------------ 1 files changed, 72 insertions(+), 74 deletions(-) diff --git a/src/responder/common/responder_dp.c b/src/responder/common/responder_dp.c index 11986f06e41a2e575760b970597713a70385c844..48777799fb3c7e2c82948afe0346bf84209d9a99 100644 --- a/src/responder/common/responder_dp.c +++ b/src/responder/common/responder_dp.c @@ -68,7 +68,6 @@ static int sss_dp_callback_destructor(void *ptr) static int sss_dp_req_destructor(void *ptr) { struct sss_dp_req *sdp_req = talloc_get_type(ptr, struct sss_dp_req); - struct sss_dp_callback *cb, *next; hash_key_t key; /* Cancel Dbus pending reply if still pending */ @@ -77,31 +76,15 @@ static int sss_dp_req_destructor(void *ptr) sdp_req->pending_reply = NULL; } - /* Destroy the hash entry */ + /* Destroy the hash entry + * There are some situations when the entry has already + * been destroyed to avoid race condition, so don't check + * the result */ key.type = HASH_KEY_STRING; key.str = sdp_req->key; int hret = hash_delete(dp_requests, &key); if (hret != HASH_SUCCESS) { - /* This should never happen */ - DEBUG(0, ("Could not clear entry from request queue\n")); - } - - /* Free any remaining callback */ - if (sdp_req->err_maj == DP_ERR_OK) { - sdp_req->err_maj = DP_ERR_FATAL; - sdp_req->err_min = EIO; - sdp_req->err_msg = discard_const_p(char, "Internal Error"); - } - - cb = sdp_req->cb_list; - while (cb) { - next = cb->next; - /* It is the responsibility of the callback to free cb */ - cb->callback(sdp_req->err_maj, - sdp_req->err_min, - sdp_req->err_msg, - cb->callback_ctx); - cb = next; + DEBUG(SSSDBG_TRACE_INTERNAL, ("Could not clear entry from request queue\n")); } return 0; @@ -133,24 +116,6 @@ void handle_requests_after_reconnect(void) } } -static void sdp_req_timeout(struct tevent_context *ev, - struct tevent_timer *te, - struct timeval t, void *ptr) -{ - struct sss_dp_req *sdp_req = talloc_get_type(ptr, struct sss_dp_req); - - sdp_req->err_maj = DP_ERR_FATAL; - sdp_req->err_min = ETIMEDOUT; - sdp_req->err_msg = discard_const_p(char, "Timed out"); - - /* steal te on NULL because it will be freed as soon as the handler - * returns. Causing a double free if we don't, as te is allocated on - * sdp_req and we are just going to free it */ - talloc_steal(NULL, te); - - talloc_free(sdp_req); -} - static int sss_dp_get_reply(DBusPendingCall *pending, dbus_uint16_t *err_maj, dbus_uint32_t *err_min, @@ -162,8 +127,6 @@ static void sss_dp_invoke_callback(struct tevent_context *ev, { struct sss_dp_req *sdp_req = talloc_get_type(ptr, struct sss_dp_req); struct sss_dp_callback *cb; - struct timeval tv; - struct tevent_timer *tev; cb = sdp_req->cb_list; /* Remove the callback from the list, the caller may free it, within the @@ -176,36 +139,65 @@ static void sss_dp_invoke_callback(struct tevent_context *ev, sdp_req->err_msg, cb->callback_ctx); - /* Call the next callback if needed */ + /* If there are some more callbacks to be invoked, + * don't destroy the request */ if (sdp_req->cb_list != NULL) { - tv = tevent_timeval_current(); + return; + } + + /* steal te on NULL because it will be freed as soon as the handler + * returns. Causing a double free if we don't, as te is allocated on + * sdp_req and we are just going to free it */ + talloc_steal(NULL, te); + + talloc_zfree(sdp_req); +} + +static void sdp_req_timeout(struct tevent_context *ev, + struct tevent_timer *te, + struct timeval t, void *ptr) +{ + struct sss_dp_req *sdp_req = talloc_get_type(ptr, struct sss_dp_req); + struct sss_dp_callback *cb, *next; + struct timeval tv; + struct tevent_timer *tev; + hash_key_t key; + + sdp_req->err_maj = DP_ERR_FATAL; + sdp_req->err_min = ETIMEDOUT; + sdp_req->err_msg = discard_const_p(char, "Timed out"); + + /* Destroy the hash entry */ + key.type = HASH_KEY_STRING; + key.str = sdp_req->key; + int hret = hash_delete(dp_requests, &key); + if (hret != HASH_SUCCESS) { + /* This should never happen */ + DEBUG(0, ("Could not clear entry from request queue\n")); + } + + /* Queue up all callbacks */ + cb = sdp_req->cb_list; + tv = tevent_timeval_current(); + while (cb) { + next = cb->next; tev = tevent_add_timer(sdp_req->ev, sdp_req, tv, sss_dp_invoke_callback, sdp_req); if (!tev) { - /* Out of memory or other serious error */ - goto done; + return; } - - return; + cb = next; } - - /* No more callbacks to invoke. Destroy the request */ -done: - /* steal te on NULL because it will be freed as soon as the handler - * returns. Causing a double free if we don't, as te is allocated on - * sdp_req and we are just going to free it */ - talloc_steal(NULL, te); - - talloc_zfree(sdp_req); } static void sss_dp_send_acct_callback(DBusPendingCall *pending, void *ptr) { int ret; struct sss_dp_req *sdp_req; - struct sss_dp_callback *cb; + struct sss_dp_callback *cb, *next; struct timeval tv; struct tevent_timer *te; + hash_key_t key; sdp_req = talloc_get_type(ptr, struct sss_dp_req); @@ -232,28 +224,34 @@ static void sss_dp_send_acct_callback(DBusPendingCall *pending, void *ptr) } /* Check whether we need to issue any callbacks */ - cb = sdp_req->cb_list; if (sdp_req->cb_list == NULL) { - if (cb == NULL) { - /* No callbacks to invoke. Destroy the hash entry */ - talloc_zfree(sdp_req); - return; - } + /* No callbacks to invoke. Destroy the hash entry */ + talloc_zfree(sdp_req); + return; + } + + /* Destroy the hash entry */ + key.type = HASH_KEY_STRING; + key.str = sdp_req->key; + int hret = hash_delete(dp_requests, &key); + if (hret != HASH_SUCCESS) { + /* This should never happen */ + DEBUG(0, ("Could not clear entry from request queue\n")); } /* Queue up all callbacks */ + cb = sdp_req->cb_list; tv = tevent_timeval_current(); - te = tevent_add_timer(sdp_req->ev, sdp_req, tv, - sss_dp_invoke_callback, sdp_req); - if (!te) { - /* Out of memory or other serious error */ - goto error; + while (cb) { + next = cb->next; + te = tevent_add_timer(sdp_req->ev, sdp_req, tv, + sss_dp_invoke_callback, sdp_req); + if (!te) { + /* Out of memory or other serious error */ + return; + } + cb = next; } - - return; - -error: - talloc_zfree(sdp_req); } static int sss_dp_send_acct_req_create(struct resp_ctx *rctx, -- 1.7.6.4
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
