On (29/05/15 12:45), Sumit Bose wrote: >On Fri, May 29, 2015 at 12:08:27PM +0200, Lukas Slebodnik wrote: >> On (29/05/15 12:02), Sumit Bose wrote: >> >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. >> I would say segfault was caused by fact that someone add new error code(s) >> but forgot to add message(s). There's nothig to fix in sss_strerror. > >yes, the underlying reason is that both lists are independent. They are dependent but in different files :-)
>What make me think that sss_strerror() should be enhanced as well is >that ERR_LAST is a valid input ERR_LAST is valid input. >and a call should make sure it does not segfault on valid input. I sent patch for checking dot at the of message. So it will be indirectly tested. Did you think something similar? or what do you mean by valid input? LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
