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

Pavel Březina pbrezina at redhat.com
Wed Oct 19 15:38:55 UTC 2011


Dne 19.10.2011 15:15, Stephen Gallagher napsal(a):
> 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);
>          }

Ok, I'm sending two versions :-)

0001-Fixes-debug-tests.c-coverity-issues-NEGATIVE_RETURNS.patch
This patch includes your suggestion.

0001-Fixes-debug-tests.c-new-code-for-error.patch
This one has DEBUG_TEST_ERROR to identify general error and 
DEBUG_TEST_NOK & DEBUG_TEST_NOK_TS to identify unit test failure.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Fixes-debug-tests.c-coverity-issues-NEGATIVE_RETURNS.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111019/0abf5ffb/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Fixes-debug-tests.c-new-code-for-error.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111019/0abf5ffb/attachment-0001.ksh>


More information about the sssd-devel mailing list