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

[Sam Bobroff <sam.bobroff@au1.ibm.com>]

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?

Thanks for the review!

Cheers,
Sam.