[SSSD] [PATCH] LDAP: unlink ccname_file_dummy if there is an error
Jakub Hrozek
jhrozek at redhat.com
Wed Mar 4 13:25:54 UTC 2015
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!
More information about the sssd-devel
mailing list