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!