4:01 a.m.
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.