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.