[SSSD] [PATCH 1/5] Fix tevent_req style for krb5_auth
Simo Sorce
simo at redhat.com
Wed Nov 7 19:16:36 UTC 2012
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
More information about the sssd-devel
mailing list