Re: [PATCH 3/3] selftests/powerpc: Add transactional syscall test

[Michael Ellerman <mpe@ellerman.id.au>]

On Tue, 2015-03-24 at 12:52 +1100, Sam Bobroff wrote:
> On 20/03/15 20:25, Anshuman Khandual wrote:
> > On 03/19/2015 10:13 AM, Sam Bobroff wrote:
> >> Check that a syscall made during an active transaction will fail with
> >> the correct failure code and that one made during a suspended
> >> transaction will succeed.
> >>
> >> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> > 
> > The test works.
> 
> Great :-)
> 
> >> +
> >> +int tm_syscall(void)
> >> +{
> >> +	SKIP_IF(!((long)get_auxv_entry(AT_HWCAP2) & PPC_FEATURE2_HTM));
> >> +	setbuf(stdout, 0);
> >> +	FAIL_IF(!t_active_getppid_test());
> >> +	printf("%d active transactions correctly aborted.\n", TM_TEST_RUNS);
> >> +	FAIL_IF(!t_suspended_getppid_test());
> >> +	printf("%d suspended transactions succeeded.\n", TM_TEST_RUNS);
> >> +	return 0;
> >> +}
> >> +
> >> +int main(void)
> >> +{
> >> +	return test_harness(tm_syscall, "tm_syscall");
> >> +}
> >> +
> > 
> > There is an extra blank line at the end of this file. Interchanging return
> > codes of 0 and 1 for various functions make it very confusing along with
> > negative FAIL_IF checks in the primary test function. Control flow structures
> > like these can use some in-code documentation for readability.
> > 
> > +	for (i = 0; i < TM_RETRIES; i++) {
> > +		if (__builtin_tbegin(0)) {
> > +			getppid();
> > +			__builtin_tend(0);
> > +			return 1;
> > +		}
> > +		if (t_failure_persistent())
> > +			return 0;
> > 
> > or
> > 
> > +		if (__builtin_tbegin(0)) {
> > +			__builtin_tsuspend();
> > +			getppid();
> > +			__builtin_tresume();
> > +			__builtin_tend(0);
> > +			return 1;
> > +		}
> > +		if (t_failure_persistent())
> > +			return 0;
> > 
> 
> Good points. I'll remove the blank line and comment the code.
> 
> I'm not sure I can do any better with the FAIL_IF() macro: I wanted it
> to read "fail if the test failed", but I can see what you mean about a
> double negative. Maybe it would be better to introduce a different
> macro, more like a standard assert: TEST(XXX) which fails if XXX is
> false. However, I think "TEST" would be too generic a name and I'm not
> should what would be better. Any comments/suggestions?

FAIL_IF() is designed for things that return 0 for OK and !0 for failure. Like
most things in C.

So I think it would be improved if you inverted your return codes in your test
routines.

Even better to return ESOMETHING in the error cases, and zero otherwise.

cheers