[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