[SSSD] fix debug-tests.c coverity issues: NEGATIVE_RETURNS, FORWARD_NULL

Stephen Gallagher sgallagh at redhat.com
Wed Oct 19 13:15:53 UTC 2011


On Mon, 2011-10-17 at 16:51 +0200, Pavel Březina wrote:
> Dne 17.10.2011 16:30, Stephen Gallagher napsal(a):
> > On Mon, 2011-10-17 at 16:12 +0200, Pavel Březina wrote:
> >> Dne 17.10.2011 15:07, Stephen Gallagher napsal(a):
> >>> On Mon, 2011-10-17 at 15:03 +0200, Pavel Březina wrote:
> >>>> https://fedorahosted.org/sssd/ticket/1046
> >>>
> >>>
> >>> Nack
> >>>
> >>> The DEBUG_TEST_NOK stuff is wrong. If it returns DEBUG_TEST_NOK, you're
> >>> not calling fail().
> >>
> >> The if condition is a little bit different than it was in previous
> >> patch, but the scheme is the same:
> >>
> >>           if (result == DEBUG_TEST_NOK&&  errno != 0) {
> >>               error_msg = strerror(errno);
> >>               fail(error_msg);
> >>           }
> >>
> >>           char *msg = NULL;
> >>           msg = talloc_asprintf(NULL, "Test of level %#.4x failed -
> >> message has been written", levels[i]);
> >>           *fail_unless(result == EOK, msg);*
> >>
> >>>
> >>> What I was trying to tell you in the first place is that you cannot mix
> >>> return code types from test_helper_debug_check_message(). It needs to
> >>> either return *only* DEBUG_TEST_* results or *only* errno error codes.
> >>> It cannot return a mixture of the two.
> >>
> >> I hope you'll like it now.
> >
> > Much better, but one little change remains:
> >
> > You can reduce
> >
> >          if (result == DEBUG_TEST_NOK&&  errno != 0) {
> >              error_msg = strerror(errno);
> >              fail(error_msg);
> >          }
> > to
> >          if (result == DEBUG_TEST_NOK) {
> >              error_msg = strerror(errno);
> >              fail(error_msg);
> >          }
> >
> > It's safe to pass 0 to strerror() (It prints "Success"), and you want it
> > to be a failure here even if there's a bug causing errno to have been
> > reset. The DEBUG_TEST_NOK is the definitive failure code.
> 
> It's save but I'm providing a different fail message.
> If errno is set, then some general error has occurred (access denied, 
> ...) and it has nothing to do with the debug level.
> 
> But if DEBUG_TEST_NOK && errno == 0, then the DEBUG(level) itself has 
> failed and I'll print that "Test of level %level failed"


Ok, I see where it falls through now. I don't like it though. It's not
clean. And it doesn't identify the real issue.

Change it to:
        if (result == DEBUG_TEST_NOK) {
            if (errno != 0) {
                error_msg = strerror(errno);
            } else {
                error_msg = "Function failed but errno unset";
            }
            fail(error_msg);
        }

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111019/73ca5e80/attachment.sig>


More information about the sssd-devel mailing list