[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