[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