On 02/23/2015 02:54 PM, Jakub Hrozek wrote:
On Mon, Feb 23, 2015 at 01:51:36PM +0100, Pavel Reichl wrote:
Your patch adds at 3 places another empty line. Is there a reason for that?
I suppose it's just a typo...
Yes, it's a copy-n-paste error, thanks for catching that. Attached is a
new patch.

btw please note that every KRB5_DEBUG call also means there's a
sss_log() call that emits to syslog, I hope that's acceptable as well
since those errors are fatal..


_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I am Sorry I didn't notice it before but adding KRB5_CHILD_DEBUG() might be duplicit as It's called every time the create_ccache() call fails - as you can see at attached snippet.

static krb5_error_code get_and_save_tgt_with_keytab(krb5_context ctx,
--
    }

    /* Use the updated principal in the creds in case canonicalized */
    kerr = create_ccache(ccname, &creds);
    if (kerr != 0) {
        KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
Here.
        goto done;
    }
    kerr = 0;

done:
    krb5_free_cred_contents(ctx, &creds);

    return kerr;
--
static krb5_error_code get_and_save_tgt(struct krb5_req *kr,
--
    }

    /* Use the updated principal in the creds in case canonicalized */
    kerr = create_ccache(cc_name, kr->creds);
    if (kerr != 0) {
        KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
Here.
        goto done;
    }

    /* Successfull authentication! Check if ccache contains the
     * right principal...
     */
    kerr = sss_krb5_check_ccache_princ(kr->ctx, kr->ccname, kr->creds->client);
    if (kerr) {
--
static errno_t create_empty_ccache(struct krb5_req *kr)
--
        DEBUG(SSSDBG_TRACE_LIBS, "Creating empty ccache\n");
        kerr = create_empty_cred(kr->ctx, kr->princ, &creds);
        if (kerr == 0) {
            kerr = create_ccache(kr->ccname, creds);
        }
    } else {
        DEBUG(SSSDBG_TRACE_LIBS, "Existing ccache still valid, reusing\n");
        kerr = 0;
    }

    if (kerr != 0) {
        KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
Here.
    } else {
        kerr = k5c_attach_ccname_msg(kr);
(END)

If I may propose solution I would remove  KRB5_CHILD_DEBUG() from the code pasted above and I would move KRB5_CHILD_DEBUG() in create_ccache() after 'done:' to save pasting it explicitly into 6 if statement.

    kerr = handle_randomized(ccname);
    if (kerr != 0) {
        DEBUG(SSSDBG_CRIT_FAILURE,
              "Cannot create randomized ccache name: %d\n", kerr);
        goto done;
    }
The only if statement that doesn't contain in KRB5_CHILD_DEBUG() can return directly, which would be IMO good thing as it returns errno value but function create_ccache() is supposed to return  krb5_error_code, so I think that explicit conversion would be nice.

Thanks for consideration.