On Tue, 2012-11-06 at 09:08 +0100, Jakub Hrozek wrote: > On Wed, Oct 31, 2012 at 06:37:31PM -0400, Simo Sorce wrote: > > No functionality changes,
[..] > > +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) [..] > 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. Yeah I think this is the least horrible way for now :) > > +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? It is not an if/else like construct. If I make it an if else, I will have to repeat the if condition twice. if (true) { DEBUG } else { assign(); if (true) { DEBUG } } Doesn't look prettier if I try to make it 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. Unless you read it in an editor that wrap lines, then I find it more confusing. I tried to make it fit in 80 chars, I could do it by renaming active_ccache_present to something more compact, however I failed to find a good name at the time and decided to split. However I will prepare a patch with a better name for that variable and with the looong if condition restored to one line as a consequence. [..] > > 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 */ Yeah I like this, I'll change it. > > + 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. True, I'll fix that. > 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. I don't think this function can be easily split w/o reintroducing artificial steps that breaks the tevent_req style. Unfortunately this function is quite complex, and I have the sensation that trying to hide that complexity will lead only to make it more obscure and difficult to follow. I will see if I can get out a function that easily symbolize specific steps, but no promises. Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel