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() 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.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel Might be it's hard
to understand what I proposed to be changed, should I
prepare the patch myself?