[SSSD] [PATCH 1/5] Fix tevent_req style for krb5_auth

Pavel Březina pbrezina at redhat.com
Thu Nov 29 10:47:58 UTC 2012


On 11/28/2012 05:24 AM, Simo Sorce wrote:
> +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);
> +    }

Why do we need this condition? Wouldn't be sufficient to use the 
assignment directly? Like
-    if (!kr->is_offline) {
+        kr->is_offline = be_is_offline(be_ctx);
-    }

If not, can you put a comment there?

> +
> +    /* 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 && 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 && !kr->valid_tgt)
> +     */
> +    if (kr->ccname == NULL ||
> +        (kr->is_offline && !kr->active_ccache && !kr->valid_tgt) ||
> +        (!kr->is_offline && !kr->active_ccache && kr->pd->cmd != SSS_CMD_RENEW)) {
> +            DEBUG(9, ("Recreating  ccache file.\n"));
> +            ccname_template = dp_opt_get_cstring(kr->krb5_ctx->opts,
> +                                                 KRB5_CCNAME_TMPL);
> +            kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
> +                                                be_ctx->domain->case_sensitive,
> +                                                &private_path);
> +            if (kr->ccname == NULL) {
> +                DEBUG(1, ("expand_ccname_template failed.\n"));
> +                return ENOMEM;
> +            }
> +
> +            if (kr->cc_be == NULL) {
> +                kr->cc_be = get_cc_be_ops_ccache(kr->ccname);
> +            }
> +            if (kr->cc_be == NULL) {
> +                DEBUG(SSSDBG_CRIT_FAILURE,
> +                      ("Cannot get operations on new ccache %s\n", kr->ccname));
> +                return EINVAL;
> +            }

Can you insert this condition inside the first one please? You are 
testing returned value of get_cc_be_ops_ccache() more then actual 
content of the variable.

> +
> +            ret = kr->cc_be->create(kr->ccname,
> +                                    kr->krb5_ctx->illegal_path_re,
> +                                    kr->uid, kr->gid, private_path);
> +            if (ret != EOK) {
> +                DEBUG(SSSDBG_OP_FAILURE, ("ccache creation failed.\n"));
> +                return ret;
> +            }
> +    } else {
> +        DEBUG(SSSDBG_MINOR_FAILURE,
> +              ("Saved ccache %s if of different type than ccache in "
> +               "configuration file, reusing the old ccache\n",
> +               kr->old_ccname));
> +
> +        kr->cc_be = get_cc_be_ops_ccache(kr->old_ccname);
> +        if (kr->cc_be == NULL) {
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  ("Cannot get operations on saved ccache %s\n",
> +                   kr->old_ccname));
> +            return EINVAL;
> +        }
> +    }
> +
> +    return EOK;
> +}

Otherwise it looks good.





More information about the sssd-devel mailing list