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 > > > >>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
