On Fri, Feb 27, 2015 at 12:04:59PM -0700, Daniel Hjorth wrote:
On Fri, Feb 27, 2015 at 10:13:28AM +0100, Jakub Hrozek wrote:
On Fri, Feb 27, 2015 at 09:59:57AM +0100, Sumit Bose wrote:
done:
- talloc_free(tmp_ctx); if (krberr != 0) KRB5_SYSLOG(krberr); if (keytab) krb5_kt_close(context, keytab); if (context) krb5_free_context(context);
- if (ccname_file_dummy) unlink(ccname_file_dummy);
It would be nice if you can check the return code of unlink(). I know it is a bit useless because we can't do anything if unlink() fails. But we try to keep the SSSD build free of warning even with high warning levels and when using static analyzers like Coverity or the one from clang. I would expect that at least some of them will complain about the unchecked return value. Additionally a debug message telling why unlink() failed might help debug issue when the dummy file is not removed properly.
bye, Sumit
That's a good point, I think using the 'ret' variable would be fine, since we return krberr.
The modified patch is attached. I used the same DEBUG that is being used in the rename but changed the failure to MINOR because it doesn't seem like a CRIT to me. Thoughts?
Daniel
ACK
I tested by tricking ldap_child into failure in gdb: 417 krberr = krb5_get_init_creds_keytab(context, &my_creds, kprinc, (gdb) n 419 krb5_kt_close(context, keytab); (gdb) 420 keytab = NULL; (gdb) 421 if (krberr) { (gdb) set krberr=123 (gdb) p krberr $1 = 123 (gdb) n 422 DEBUG(SSSDBG_FATAL_FAILURE, (gdb) 425 sss_log(SSS_LOG_ERR, (gdb) 430 goto done; (gdb) 499 if (krberr != 0) KRB5_SYSLOG(krberr); (gdb) 500 if (keytab) krb5_kt_close(context, keytab); (gdb) 501 if (context) krb5_free_context(context); (gdb) 502 if (ccname_file_dummy) { (gdb) 503 DEBUG(SSSDBG_TRACE_INTERNAL, "Unlinking [%s]\n", ccname_file_dummy); (gdb) 504 ret = unlink(ccname_file_dummy); (gdb) 505 if (ret == -1) { (gdb) 511 talloc_free(tmp_ctx); (gdb) c Continuing.
Thanks for the patch again!