On Fri, May 29, 2015 at 11:17:01AM +0200, Pavel Reichl wrote: > > > 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).
If there is something wrong at all, I would say is it in c). First the current test ck_assert_str_eq(sss_strerror(ERR_LAST), "ERR_LAST"); does in fact two things. First it calls sss_strerror() with a valid input (any int is a valid input here), second it compares the returned value with the expected one. The test is completely deterministic because only if the returned value matches the expected value the test passed. In all other cases (including segfaults) the test must be considered failed because it is expected that sss_strerror() always returns a string and never NULL. The segfault is caused by calling sss_strerror() with ERR_LAST as argument. Since this is a valid input it is sss_strerror() which acts non-deterministic here and not the test. So sss_strerror() needs fixing to make it deterministic by checking if the list of error codes and the list of error messages have the same size before accessing the list of error messages. HTH bye, Sumit > > > > >>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 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
