On Mon, Feb 23, 2015 at 05:54:59PM +0100, Pavel Reichl wrote:
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(a)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(
I did this.
) in create_ccache() after
'done:' to save pasting it explicitly into 6 if statement.
I didn't do this, because KRB5_CHILD_DEBUG output contains the line
number and moving the statement to DEBUG would make it useless.
> 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.
I explicitly print the error value in handle_randomized now, but I don't
think we need to do any converstion, it's perfectly safe to mix krb5
error codes and errno code, they have a different base.