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.bobr...@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
