On Fri, 2012-03-02 at 13:52 -0500, Stephen Gallagher wrote: > This function alters the memory hierarchy of the be_req > to ensure memory safety during shutdown. It creates a > spy on the be_cli object so that it will free the be_req > if the client is freed. > > It is generally allocated atop the private data context > for the appropriate back-end against which it is being > filed. > > This should resolve https://fedorahosted.org/sssd/ticket/1226
Simo performed the review on IRC and found one issue (the destructors should not be set until both spies are active) and made a minor improvement request. New patch attached.
From a48db16970e9d2b9eb14a186cd178680a18940b2 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher <sgall...@redhat.com> Date: Fri, 2 Mar 2012 13:50:03 -0500 Subject: [PATCH] DP: Reorganize memory hierarchy of requests This function alters the memory hierarchy of the be_req to ensure memory safety during shutdown. It creates a spy on the be_cli object so that it will free the be_req if the client is freed. It is generally allocated atop the private data context for the appropriate back-end against which it is being filed. https://fedorahosted.org/sssd/ticket/1226 --- src/providers/data_provider_be.c | 134 +++++++++++++++++++++++++++++++------- 1 files changed, 110 insertions(+), 24 deletions(-) diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c index 695dee0e5b558dd17c9cb0672473e650578def51..c7bf2f0840dcef40406c7efd13f820901d1a2e4f 100644 --- a/src/providers/data_provider_be.c +++ b/src/providers/data_provider_be.c @@ -132,28 +132,114 @@ static void be_async_req_handler(struct tevent_context *ev, async_req->fn(async_req->req); } -static int be_file_request(struct be_ctx *ctx, - be_req_fn_t fn, - struct be_req *req) +struct be_spy { + TALLOC_CTX *freectx; + struct be_spy *double_agent; +}; + +static int be_spy_destructor(struct be_spy *spy) { + /* If there's a double_agent, set its + * freectx to NULL so that we don't + * try to loop. When that spy fires, + * it will just be a no-op. + */ + spy->double_agent->freectx = NULL; + talloc_zfree(spy->freectx); + return 0; +} + +static errno_t be_spy_create(TALLOC_CTX *mem_ctx, struct be_req *be_req) +{ + errno_t ret; + struct be_spy *cli_spy = NULL; + struct be_spy *req_spy = NULL; + + /* Attach a spy for the be_client so that if it dies, + * we can free the be_req automatically. + */ + cli_spy = talloc_zero(be_req->becli, struct be_spy); + if (!cli_spy) { + ret = ENOMEM; + goto done; + } + cli_spy->freectx = mem_ctx; + talloc_set_destructor(cli_spy, be_spy_destructor); + + /* Also create a spy on the be_req so that we + * can free the other spy when the be_req + * completes successfully. + */ + req_spy = talloc_zero(be_req, struct be_spy); + if (!req_spy) { + ret = ENOMEM; + goto done; + } + req_spy->freectx = cli_spy; + talloc_set_destructor(req_spy, be_spy_destructor); + + /* Create paired spy links to prevent loops */ + cli_spy->double_agent = req_spy; + req_spy->double_agent = cli_spy; + + /* Now create the destructors that will actually free + * the opposing spies. + */ + talloc_set_destructor(cli_spy, be_spy_destructor); + talloc_set_destructor(req_spy, be_spy_destructor); + + + /* Now steal the be_req onto the mem_ctx so that it + * will be guaranteed that this data will be + * available for the full duration of execution. + */ + talloc_steal(mem_ctx, be_req); + + ret = EOK; +done: + if (ret != EOK) { + talloc_free(cli_spy); + talloc_free(req_spy); + } + return ret; +} + +/* This function alters the memory hierarchy of the be_req + * to ensure memory safety during shutdown. It creates a + * spy on the be_cli object so that it will free the be_req + * if the client is freed. + * + * It is generally allocated atop the private data context + * for the appropriate back-end against which it is being + * filed. + */ +static errno_t be_file_request(TALLOC_CTX *mem_ctx, + struct be_req *be_req, + be_req_fn_t fn) +{ + errno_t ret; struct be_async_req *areq; struct tevent_timer *te; struct timeval tv; - if (!fn || !req) return EINVAL; + if (!fn || !be_req) return EINVAL; - areq = talloc(req, struct be_async_req); + ret = be_spy_create(mem_ctx, be_req); + if (ret != EOK) return ret; + + areq = talloc(be_req, struct be_async_req); if (!areq) { return ENOMEM; } areq->fn = fn; - areq->req = req; + areq->req = be_req; /* fire immediately */ tv.tv_sec = 0; tv.tv_usec = 0; - te = tevent_add_timer(ctx->ev, req, tv, be_async_req_handler, areq); + te = tevent_add_timer(be_req->be_ctx->ev, be_req, + tv, be_async_req_handler, areq); if (te == NULL) { return EIO; } @@ -446,9 +532,9 @@ static int be_get_account_info(DBusMessage *message, struct sbus_connection *con /* process request */ - ret = be_file_request(becli->bectx, - becli->bectx->bet_info[BET_ID].bet_ops->handler, - be_req); + ret = be_file_request(becli->bectx->bet_info[BET_ID].pvt_bet_data, + be_req, + becli->bectx->bet_info[BET_ID].bet_ops->handler); if (ret != EOK) { err_maj = DP_ERR_FATAL; err_min = ret; @@ -606,9 +692,9 @@ static int be_pam_handler(DBusMessage *message, struct sbus_connection *conn) be_req->req_data = pd; - ret = be_file_request(becli->bectx, - becli->bectx->bet_info[target].bet_ops->handler, - be_req); + ret = be_file_request(becli->bectx->bet_info[target].pvt_bet_data, + be_req, + becli->bectx->bet_info[target].bet_ops->handler); if (ret != EOK) { DEBUG(7, ("be_file_request failed.\n")); goto done; @@ -811,9 +897,9 @@ static int be_sudo_handler(DBusMessage *message, struct sbus_connection *conn) goto fail; } - ret = be_file_request(be_cli->bectx, - be_cli->bectx->bet_info[BET_SUDO].bet_ops->handler, - be_req); + ret = be_file_request(be_cli->bectx->bet_info[BET_SUDO].pvt_bet_data, + be_req, + be_cli->bectx->bet_info[BET_SUDO].bet_ops->handler); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, ("be_file_request failed.\n")); err_msg = "Cannot file back end request"; @@ -974,9 +1060,9 @@ static int be_autofs_handler(DBusMessage *message, struct sbus_connection *conn) goto done; } - ret = be_file_request(be_cli->bectx, - be_cli->bectx->bet_info[BET_AUTOFS].bet_ops->handler, - be_req); + ret = be_file_request(be_cli->bectx->bet_info[BET_AUTOFS].pvt_bet_data, + be_req, + be_cli->bectx->bet_info[BET_AUTOFS].bet_ops->handler); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, ("be_file_request failed.\n")); err_maj = DP_ERR_FATAL; @@ -1196,9 +1282,9 @@ static int be_host_handler(DBusMessage *message, struct sbus_connection *conn) /* process request */ - ret = be_file_request(becli->bectx, - becli->bectx->bet_info[BET_HOSTID].bet_ops->handler, - be_req); + ret = be_file_request(becli->bectx->bet_info[BET_HOSTID].pvt_bet_data, + be_req, + becli->bectx->bet_info[BET_HOSTID].bet_ops->handler); if (ret != EOK) { err_maj = DP_ERR_FATAL; err_min = ret; @@ -1345,8 +1431,8 @@ static errno_t be_file_check_online_request(struct be_req *req) req->be_ctx->offstat.went_offline = time(NULL); reset_fo(req->be_ctx); - ret = be_file_request(req->be_ctx, - req->be_ctx->bet_info[BET_ID].bet_ops->check_online, req); + ret = be_file_request(req->be_ctx, req, + req->be_ctx->bet_info[BET_ID].bet_ops->check_online); if (ret != EOK) { DEBUG(1, ("be_file_request failed.\n")); } -- 1.7.7.6
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel