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

Pavel Březina pbrezina at redhat.com
Fri Nov 30 10:01:40 UTC 2012


On 11/29/2012 03:45 PM, Simo Sorce wrote:
> On Thu, 2012-11-29 at 11:47 +0100, Pavel Březina wrote:
>> 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?
>
> As I said already I just copied this code verbatim, I do not know why it
> was done that way, and I would not be able to put a comment in there.
>
> Please ask the original author if it is really important, but this piece
> of code while odd is not changing from the original so shouldn't be
> really holding up the review IMO.
>
>>> +    /* 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.
>
> Uhmm I though I did ... odd, maybe this pattern was repeated and didn't
> realize it. Again this is just moved around code, and I prefer not to
> touch it unless there is a good reason, because changing arbitrarily
> code while refactoring is just a good recipe for getting bugs in.
> But in this case it is clear I can do the change if it really is
> important to you.
>
>>> +
>>> +            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.
>
> Ok, let me kinow how strong you feel about point 2, for point 1 let me
> know what comment would you put in there, I don' know what to put there,
> doesn't makes much sense to me and could be just bad code, but didn't
> want to bother during a style refactoring as I said above.
>
> Simo.
>

OK. I don't insist since you are only moving the code around.

Ack.





More information about the sssd-devel mailing list