On 05/28/2015 06:52 PM, Sumit Bose wrote:
On Thu, May 28, 2015 at 06:26:43PM +0200, Pavel Reichl wrote:
On 05/28/2015 06:12 PM, Sumit Bose wrote:
On Thu, May 28, 2015 at 05:57:35PM +0200, Lukas Slebodnik wrote:
On (28/05/15 17:53), Pavel Reichl wrote:
On 05/28/2015 05:45 PM, Lukas Slebodnik wrote:
On (28/05/15 17:25), Pavel Reichl wrote:
Hello,
please see simple attached patch. Although test catches the bug and is no
doubt useful I believe that we should avoid segfaulting even in tests.
Thanks for considering!
>From a509c49fbc55664170d96e0e29bf6263c8d38a2a Mon Sep 17 00:00:00 2001
From: Pavel Reichl <[email protected]>
Date: Thu, 28 May 2015 11:13:47 -0400
Subject: [PATCH] TESTS: fix segfault in test_sss_strerror_err_last
If there were more error codes than error messages. This
test segfaulted.
When test crashed then it failed.
I don't think that relying on undefined behavior is a good practice.
It's not undefined behaviour.
becuase you should not dereference NULL pointer.
So the problem was not in test but in sssd code.
Well technically the test is part of SSSD code suite IMO.
Test can crash if the bug is in the code.
result of make check is non zero -> failed.
I agree with Lukas here. The crash is not in the test. but in the code.
If I see it correctly from you patch sss_strerror() crashes if the error
code is valid but there is no matching error message, because the array
is too short. Instead of doing the check in the test the check should be
done in sss_strerror() and if there is a mismatch a special error
should be return. The test then checks for proper behaviour and fails id
the special error is returned indication a mismatch in error codes and
error messages.
HTH
bye,
Sumit
Sumit, thanks for comment. But I'm little confused here, do you agree with
Lukas that this test is implemented just fine or do you agree with me that
I think the test is implemented fine, it checks if the last error
message is the expected message. If the test crashes, it is not the
fault of the test but the tested call, sss_strerror() has an issue.
Thanks for explaining. I'll probably send the patch on sss_strerror() then.
segfault is not good test output?
I think it is neither good or bad, it just indicates that the test
discovered an error.
bye,
Sumit
OK, apparently I'm on the wrong side of this discussion. I yield, I just
want to learn from my mistakes, so can you please tell me when I was wrong:
a) Dereferencing NULL pointer is from C language pov undefined behavior
and virtually anything can happen => test might pass.
b) It's not good practice to expect linux specific ability to handle
dereferencing NULL pointer by segfaulting.
c) Considering test that does only one thing - comparing number of error
messages to error codes to be in need for improvement because it's not
deterministic (a).
I like your proposal how to improve the test and I'm happy to fix it that
way if agreed.
Thanks!
IMHO it's sufficient to test that the last error code is the same.
IMHO it's not good if test is based on occurrence of segfault.
I do not agree with exporting global variables just for test.
It's very BAD practice.
If it was not clear then:
_ _ _ ____ _ __
| \ | | / \ / ___| |/ /
| \| | / _ \| | | ' /
| |\ |/ ___ \ |___| . \
|_| \_/_/ \_\____|_|\_\
LS
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
