[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