On Wed, Oct 31, 2012 at 06:37:31PM -0400, Simo Sorce wrote:
> No functionality changes,
> just make the code respect the tevent_req style and naming conventions
> and enhance readability by adding some helper functions.
> ---
> src/providers/krb5/krb5_access.c | 6 +-
> src/providers/krb5/krb5_auth.c | 556
> ++++++++++++++++------------------
> src/providers/krb5/krb5_auth.h | 2 +-
> src/providers/krb5/krb5_wait_queue.c | 12 +-
> 4 files changed, 268 insertions(+), 308 deletions(-)
>
> diff --git a/src/providers/krb5/krb5_access.c
> b/src/providers/krb5/krb5_access.c
> index
> 25612807d701a392f697caa82b3f31182416b824..970633eb2f02b68cd3a3e38432c10f6a347edc91
> 100644
> --- a/src/providers/krb5/krb5_access.c
> +++ b/src/providers/krb5/krb5_access.c
> @@ -38,7 +38,7 @@ struct krb5_access_state {
> bool access_allowed;
> };
>
> -static void krb5_access_child_done(struct tevent_req *subreq);
> +static void krb5_access_done(struct tevent_req *subreq);
> struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> struct be_ctx *be_ctx,
> @@ -141,7 +141,7 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx,
> goto done;
> }
>
> - tevent_req_set_callback(subreq, krb5_access_child_done, req);
> + tevent_req_set_callback(subreq, krb5_access_done, req);
> return req;
>
> done:
> @@ -154,7 +154,7 @@ done:
> return req;
> }
>
> -static void krb5_access_child_done(struct tevent_req *subreq)
> +static void krb5_access_done(struct tevent_req *subreq)
> {
> struct tevent_req *req = tevent_req_callback_data(subreq, struct
> tevent_req);
> struct krb5_access_state *state = tevent_req_data(req,
> diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
> index
> f2e00fac164c4a8ff40e67edb3fa400a3f339b7c..c56dfb60a5c37c6cd6c825355fcad1e49767657a
> 100644
> --- a/src/providers/krb5/krb5_auth.c
> +++ b/src/providers/krb5/krb5_auth.c
> @@ -270,12 +270,116 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct
> pam_data *pd,
> return EOK;
> }
>
> -static void krb5_resolve_kdc_done(struct tevent_req *subreq);
> -static void krb5_resolve_kpasswd_done(struct tevent_req *subreq);
> -static void krb5_find_ccache_step(struct tevent_req *req);
> -static void krb5_save_ccname_done(struct tevent_req *req);
> -static void krb5_child_done(struct tevent_req *req);
> -static void krb5_pam_handler_cache_auth_step(struct tevent_req *req);
> +
> +static void krb5_auth_cache_creds(struct krb5_ctx *krb5_ctx,
> + struct sysdb_ctx *sysdb,
> + struct confdb_ctx *cdb,
> + struct pam_data *pd, uid_t uid,
> + int *pam_status, int *dp_err)
> +{
> + errno_t ret;
> +
> + ret = sysdb_cache_auth(sysdb, pd->user, pd->authtok,
> + pd->authtok_size, cdb, true, NULL,
> + NULL);
> + if (ret != EOK) {
> + DEBUG(1, ("Offline authentication failed\n"));
> + *pam_status = PAM_SYSTEM_ERR;
> + *dp_err = DP_ERR_OK;
> + return;
> + }
> +
> + ret = add_user_to_delayed_online_authentication(krb5_ctx, pd, uid);
> + if (ret != EOK) {
> + /* This error is not fatal */
> + DEBUG(1, ("add_user_to_delayed_online_authentication failed.\n"));
> + }
> + *pam_status = PAM_AUTHINFO_UNAVAIL;
> + *dp_err = DP_ERR_OFFLINE;
> +}
> +
I'm not quite sure I like a function that returns void but has two
output parameters..but then again, the intent of the function is to
return a (pam_status, dp_err) tuple.
> +static errno_t krb5_auth_prepare_ccache_file(struct krb5child_req *kr,
> + struct be_ctx *be_ctx,
> + int *pam_status, int *dp_err)
> +{
> + const char *ccname_template;
> + bool private_path = false;
> + errno_t ret;
> +
> + if (!kr->is_offline) {
> + kr->is_offline = be_is_offline(be_ctx);
> + }
> + if (kr->is_offline) {
> + DEBUG(9, ("Preparing for offline operation.\n"));
> + }
> +
Can you merge these two conditions into an if-else?
> + /* The ccache file should be (re)created if one of the following
> conditions
> + * is true:
> + * - it doesn't exist (kr->ccname == NULL)
> + * - the backend is online and the current ccache file is not used, i.e
> + * the related user is currently not logged in and it is not a renewal
> + * request
> + * (!kr->is_offline && !kr->active_ccache_present &&
> + * kr->pd->cmd != SSS_CMD_RENEW)
> + * - the backend is offline and the current cache file not used and
> + * it does not contain a valid tgt
> + * (kr->is_offline &&
> + * !kr->active_ccache_present && !kr->valid_tgt_present)
> + */
> + if (kr->ccname == NULL ||
> + (kr->is_offline &&
> + !kr->active_ccache_present &&
> + !kr->valid_tgt_present) ||
> + (!kr->is_offline &&
> + !kr->active_ccache_present &&
> + kr->pd->cmd != SSS_CMD_RENEW)) {
Now that every condition is on a separate line, the whole if no longer
looks like OR of three cases at first sight. The comment above it helps
a little, but maybe this would be an acceptable case for breaking the 80
chars limit. I think this:
if (kr->ccname == NULL ||
(kr->is_offline && !kr->active_ccache_present && !kr->valid_tgt_present) ||
(!kr->is_offline && !kr->active_ccache_present && kr->pd->cmd !=
SSS_CMD_RENEW)) {
Is more clear.
Maybe just split the if a little differently to make it clear the line
continues:
if (kr->ccname == NULL ||
(kr->is_offline && !kr->active_ccache_present && \
!kr->valid_tgt_present) ||
(!kr->is_offline && !kr->active_ccache_present && \
kr->pd->cmd != SSS_CMD_RENEW)) {
> -
> -static void krb5_child_done(struct tevent_req *subreq)
> +static void krb5_auth_done(struct tevent_req *subreq)
> {
> struct tevent_req *req = tevent_req_callback_data(subreq, struct
> tevent_req);
> struct krb5_auth_state *state = tevent_req_data(req, struct
> krb5_auth_state);
> -
> struct krb5child_req *kr = state->kr;
> struct pam_data *pd = state->pd;
> int ret;
> @@ -750,18 +764,54 @@ static void krb5_child_done(struct tevent_req *subreq)
> ssize_t len = -1;
> struct krb5_child_response *res;
> const char *store_ccname;
> + struct fo_server *search_srv;
> + char *password = NULL;
>
> ret = handle_child_recv(subreq, pd, &buf, &len);
> talloc_zfree(subreq);
> - if (ret != EOK) {
> + if (ret != EOK && ret != ETIMEDOUT) {
> DEBUG(1, ("child failed (%d [%s])\n", ret, strerror(ret)));
> - if (ret == ETIMEDOUT) {
> - if (krb5_next_server(req) == NULL) {
> - tevent_req_error(req, ENOMEM);
> + tevent_req_error(req, ret);
> + return;
> + }
> + if (ret == ETIMEDOUT) {
> +
I find it more readable to keep the handling of a single return code in
a single if statement, ie:
if (ret == ETIMEDOUT) {
/* Handle time out */
} else if (ret != EOK) {
/* Handle error */
}
/* EOK */
> + DEBUG(1, ("child timed out!\n"));
> +
> + switch (pd->cmd) {
> + case SSS_PAM_AUTHENTICATE:
> + case SSS_CMD_RENEW:
> + state->search_kpasswd = false;
> + search_srv = kr->srv;
> + break;
> + case SSS_PAM_CHAUTHTOK:
> + case SSS_PAM_CHAUTHTOK_PRELIM:
> + if (state->kr->kpasswd_srv) {
> + state->search_kpasswd = true;
> + search_srv = kr->kpasswd_srv;
> + break;
> + } else {
> + state->search_kpasswd = false;
> + search_srv = kr->srv;
> + break;
> }
> - } else {
> - tevent_req_error(req, ret);
> + default:
> + DEBUG(1, ("Unexpected PAM task\n"));
> + tevent_req_error(req, EINVAL);
> + return;
The error handling in the function is not consistent. Some branches use
tevent_req_error(req, errno) and some use goto done.
The function is also quite big (300+ lines). I would suggest to also
split it into some helper functions. The timeout handling and password
caching operations seem like a good start.
> }
> +
> + be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
> + search_srv, PORT_NOT_WORKING);
> + subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
> +
> state->krb5_ctx->kpasswd_service->name,
> + search_srv == NULL ? true : false);
> + if (subreq == NULL) {
> + DEBUG(1, ("Failed resolved request.\n"));
> + tevent_req_error(req, ENOMEM);
> + return;
> + }
> + tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
> return;
> }
>
> @@ -871,9 +921,16 @@ static void krb5_child_done(struct tevent_req *subreq)
> state->krb5_ctx->service->name,
> kr->kpasswd_srv, PORT_NOT_WORKING);
> /* ..try to resolve next kpasswd server */
> - if (krb5_next_kpasswd(req) == NULL) {
> + state->search_kpasswd = true;
> + subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
> + state->krb5_ctx->kpasswd_service->name,
> + state->kr->kpasswd_srv == NULL ? true :
> false);
> + if (subreq == NULL) {
> + DEBUG(1, ("Resolver request failed.\n"));
> tevent_req_error(req, ENOMEM);
> + return;
> }
> + tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
> return;
> } else {
> be_fo_set_port_status(state->be_ctx,
> @@ -891,9 +948,16 @@ static void krb5_child_done(struct tevent_req *subreq)
> be_fo_set_port_status(state->be_ctx,
> state->krb5_ctx->service->name,
> kr->srv, PORT_NOT_WORKING);
> /* ..try to resolve next KDC */
> - if (krb5_next_kdc(req) == NULL) {
> + state->search_kpasswd = false;
> + subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
> + state->krb5_ctx->service->name,
> + kr->srv == NULL ? true : false);
> + if (subreq == NULL) {
> + DEBUG(1, ("Resolver request failed.\n"));
> tevent_req_error(req, ENOMEM);
> + return;
> }
> + tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
> return;
> }
> } else if (kr->srv != NULL) {
> @@ -949,104 +1013,19 @@ static void krb5_child_done(struct tevent_req *subreq)
> }
> }
>
> - krb5_save_ccname_done(req);
> -
> - return;
> -
> -done:
> - if (ret == EOK) {
> - tevent_req_done(req);
> - } else {
> - tevent_req_error(req, ret);
> - }
> -}
> -
> -static struct tevent_req *krb5_next_server(struct tevent_req *req)
> -{
> - struct krb5_auth_state *state = tevent_req_data(req, struct
> krb5_auth_state);
> - struct pam_data *pd = state->pd;
> - struct tevent_req *next_req = NULL;
> -
> - switch (pd->cmd) {
> - case SSS_PAM_AUTHENTICATE:
> - case SSS_CMD_RENEW:
> - be_fo_set_port_status(state->be_ctx,
> state->krb5_ctx->service->name,
> - state->kr->srv, PORT_NOT_WORKING);
> - next_req = krb5_next_kdc(req);
> - break;
> - case SSS_PAM_CHAUTHTOK:
> - case SSS_PAM_CHAUTHTOK_PRELIM:
> - if (state->kr->kpasswd_srv) {
> - be_fo_set_port_status(state->be_ctx,
> state->krb5_ctx->service->name,
> - state->kr->kpasswd_srv,
> PORT_NOT_WORKING);
> - next_req = krb5_next_kpasswd(req);
> - break;
> - } else {
> - be_fo_set_port_status(state->be_ctx,
> state->krb5_ctx->service->name,
> - state->kr->srv, PORT_NOT_WORKING);
> - next_req = krb5_next_kdc(req);
> - break;
> - }
> - default:
> - DEBUG(1, ("Unexpected PAM task\n"));
> - }
> -
> - return next_req;
> -}
> -
> -static struct tevent_req *krb5_next_kdc(struct tevent_req *req)
> -{
> - struct tevent_req *next_req;
> - struct krb5_auth_state *state = tevent_req_data(req, struct
> krb5_auth_state);
> -
> - next_req = be_resolve_server_send(state, state->ev,
> - state->be_ctx,
> - state->krb5_ctx->service->name,
> - state->kr->srv == NULL ? true : false);
> - if (next_req == NULL) {
> - DEBUG(1, ("be_resolve_server_send failed.\n"));
> - return NULL;
> - }
> - tevent_req_set_callback(next_req, krb5_resolve_kdc_done, req);
> -
> - return next_req;
> -}
> -
> -static struct tevent_req *krb5_next_kpasswd(struct tevent_req *req)
> -{
> - struct tevent_req *next_req;
> - struct krb5_auth_state *state = tevent_req_data(req, struct
> krb5_auth_state);
> -
> - next_req = be_resolve_server_send(state, state->ev,
> - state->be_ctx,
> - state->krb5_ctx->kpasswd_service->name,
> - state->kr->kpasswd_srv == NULL ? true :
> false);
> - if (next_req == NULL) {
> - DEBUG(1, ("be_resolve_server_send failed.\n"));
> - return NULL;
> - }
> - tevent_req_set_callback(next_req, krb5_resolve_kpasswd_done, req);
> -
> - return next_req;
> -}
> -
> -static void krb5_save_ccname_done(struct tevent_req *req)
> -{
> - struct krb5_auth_state *state = tevent_req_data(req, struct
> krb5_auth_state);
> - struct krb5child_req *kr = state->kr;
> - struct pam_data *pd = state->pd;
> - int ret;
> - char *password = NULL;
> -
> if (kr->is_offline) {
> - if
> (dp_opt_get_bool(kr->krb5_ctx->opts,KRB5_STORE_PASSWORD_IF_OFFLINE)) {
> - krb5_pam_handler_cache_auth_step(req);
> - return;
> + if (dp_opt_get_bool(kr->krb5_ctx->opts,
> + KRB5_STORE_PASSWORD_IF_OFFLINE)) {
> + krb5_auth_cache_creds(state->kr->krb5_ctx,
> + state->be_ctx->sysdb,
> + state->be_ctx->cdb,
> + state->pd, state->kr->uid,
> + &state->pam_status, &state->dp_err);
> + } else {
> + DEBUG(4, ("Backend is marked offline, retry later!\n"));
> + state->pam_status = PAM_AUTHINFO_UNAVAIL;
> + state->dp_err = DP_ERR_OFFLINE;
> }
> -
> - DEBUG(4, ("Backend is marked offline, retry later!\n"));
> - state->pam_status = PAM_AUTHINFO_UNAVAIL;
> - state->dp_err = DP_ERR_OFFLINE;
> ret = EOK;
> goto done;
> }
> @@ -1115,32 +1094,15 @@ done:
>
> }
>
> -static void krb5_pam_handler_cache_auth_step(struct tevent_req *req)
> +int krb5_auth_recv(struct tevent_req *req, int *pam_status, int *dp_err)
> {
> struct krb5_auth_state *state = tevent_req_data(req, struct
> krb5_auth_state);
> - struct pam_data *pd = state->pd;
> - struct krb5_ctx *krb5_ctx = state->kr->krb5_ctx;
> - int ret;
> + *pam_status = state->pam_status;
> + *dp_err = state->dp_err;
>
> - ret = sysdb_cache_auth(state->sysdb, pd->user, pd->authtok,
> - pd->authtok_size, state->be_ctx->cdb, true, NULL,
> - NULL);
> - if (ret != EOK) {
> - DEBUG(1, ("Offline authentication failed\n"));
> - state->pam_status = PAM_SYSTEM_ERR;
> - state->dp_err = DP_ERR_OK;
> - } else {
> - ret = add_user_to_delayed_online_authentication(krb5_ctx, pd,
> - state->kr->uid);
> - if (ret != EOK) {
> - /* This error is not fatal */
> - DEBUG(1, ("add_user_to_delayed_online_authentication
> failed.\n"));
> - }
> - state->pam_status = PAM_AUTHINFO_UNAVAIL;
> - state->dp_err = DP_ERR_OFFLINE;
> - }
> + TEVENT_REQ_RETURN_ON_ERROR(req);
>
> - tevent_req_done(req);
> + return EOK;
> }
>
> static void krb_reply(struct be_req *req, int dp_err, int result)
> @@ -1148,8 +1110,8 @@ static void krb_reply(struct be_req *req, int dp_err,
> int result)
> req->fn(req, dp_err, result, NULL);
> }
>
> -void krb5_auth_done(struct tevent_req *req);
> -static void krb5_access_done(struct tevent_req *req);
> +void krb5_pam_handler_auth_done(struct tevent_req *req);
> +static void krb5_pam_handler_access_done(struct tevent_req *req);
>
> void krb5_pam_handler(struct be_req *be_req)
> {
> @@ -1193,7 +1155,7 @@ void krb5_pam_handler(struct be_req *be_req)
> goto done;
> }
>
> - tevent_req_set_callback(req, krb5_auth_done, be_req);
> + tevent_req_set_callback(req, krb5_pam_handler_auth_done, be_req);
> break;
> case SSS_PAM_ACCT_MGMT:
> req = krb5_access_send(be_req, be_req->be_ctx->ev,
> be_req->be_ctx,
> @@ -1203,7 +1165,7 @@ void krb5_pam_handler(struct be_req *be_req)
> goto done;
> }
>
> - tevent_req_set_callback(req, krb5_access_done, be_req);
> + tevent_req_set_callback(req, krb5_pam_handler_access_done,
> be_req);
> break;
> case SSS_PAM_SETCRED:
> case SSS_PAM_OPEN_SESSION:
> @@ -1225,7 +1187,7 @@ done:
> krb_reply(be_req, dp_err, pd->pam_status);
> }
>
> -void krb5_auth_done(struct tevent_req *req)
> +void krb5_pam_handler_auth_done(struct tevent_req *req)
> {
> int ret;
> struct be_req *be_req = tevent_req_callback_data(req, struct be_req);
> @@ -1255,7 +1217,7 @@ void krb5_auth_done(struct tevent_req *req)
> krb_reply(be_req, dp_err, pd->pam_status);
> }
>
> -static void krb5_access_done(struct tevent_req *req)
> +static void krb5_pam_handler_access_done(struct tevent_req *req)
> {
> int ret;
> struct be_req *be_req = tevent_req_callback_data(req, struct be_req);
> diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h
> index
> 9133472abb9ab0a42f1bac861a52f867d393e644..6f9679d1641d2689b87e10ef49d6124531f332cf
> 100644
> --- a/src/providers/krb5/krb5_auth.h
> +++ b/src/providers/krb5/krb5_auth.h
> @@ -61,6 +61,7 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
> struct krb5_ctx *krb5_ctx, struct krb5child_req
> **krb5_req);
>
> void krb5_pam_handler(struct be_req *be_req);
> +void krb5_pam_handler_auth_done(struct tevent_req *req);
>
> struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> @@ -68,7 +69,6 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
> struct pam_data *pd,
> struct krb5_ctx *krb5_ctx);
> int krb5_auth_recv(struct tevent_req *req, int *pam_status, int *dp_err);
> -void krb5_auth_done(struct tevent_req *req);
>
> struct tevent_req *handle_child_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> diff --git a/src/providers/krb5/krb5_wait_queue.c
> b/src/providers/krb5/krb5_wait_queue.c
> index
> 3863b1bdc1386739ca0651278c9d941d85e46909..da1e35b7db8956950e9b1164ae983d21994dde69
> 100644
> --- a/src/providers/krb5/krb5_wait_queue.c
> +++ b/src/providers/krb5/krb5_wait_queue.c
> @@ -41,20 +41,18 @@ struct queue_entry {
> static void wait_queue_auth(struct tevent_context *ev, struct tevent_timer
> *te,
> struct timeval current_time, void *private_data)
> {
> - struct queue_entry *queue_entry = talloc_get_type(private_data,
> - struct queue_entry);
> + struct queue_entry *qe = talloc_get_type(private_data, struct
> queue_entry);
> struct tevent_req *req;
>
> - req = krb5_auth_send(queue_entry->be_req,
> queue_entry->be_req->be_ctx->ev,
> - queue_entry->be_req->be_ctx, queue_entry->pd,
> - queue_entry->krb5_ctx);
> + req = krb5_auth_send(qe->be_req, qe->be_req->be_ctx->ev,
> + qe->be_req->be_ctx, qe->pd, qe->krb5_ctx);
> if (req == NULL) {
> DEBUG(1, ("krb5_auth_send failed.\n"));
> } else {
> - tevent_req_set_callback(req, krb5_auth_done, queue_entry->be_req);
> + tevent_req_set_callback(req, krb5_pam_handler_auth_done, qe->be_req);
> }
>
> - talloc_zfree(queue_entry);
> + talloc_zfree(qe);
> }
>
> static void wait_queue_del_cb(hash_entry_t *entry, hash_destroy_enum type,
> --
> 1.7.1
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
