Page MenuHomeFreeBSD

ptrace_test: Add more debug output on test failures
ClosedPublic

Authored by arichardson on Tue, Feb 23, 12:36 PM.

Details

Summary

Mostly automatic, using
CHILD_REQUIRE\(([^|&\n]*) == -> CHILD_REQUIRE_EQ_INT($1,
ATF_REQUIRE\(([^|&\n]*) == -> REQUIRE_EQ_INT($1, followed by
git-clang-format -f and then manually checking ones that contain ||/&&.

PR: 243605

Test Plan

still getting the same failure but now it prints psr.sr_error (0) == EBADF (9) not met instead of just failing without printing the values.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tests/sys/kern/ptrace_test.c
92–105

Per style(9) every line after the #define should be deindented by one tab.

111

s/2/STDERR_FILENO/? Why not use fprintf(stderr)?

128

Ditto.

tests/sys/kern/ptrace_test.c
92–105

I tried to match the macro above, will remove one leading tab in both.

111

I'm not sure why the previous code didn't use fprintf(). I just replaced the write(2, buf, strlen(buf)) with dprintf, but being able to use vfprintf() would be even simpler. I'll defer to @jhb who appears to have written this.

arichardson marked 2 inline comments as done.
  • fix macro indentation

Could you just use ATF_REQUIRE_EQ directly instead of the local REQUIRE_EQ_INT? Then dropping _INT from CHILD_REQUIRE_EQ() would be more symmetric to the ATF API.

tests/sys/kern/ptrace_test.c
97

I would be tempted to drop the _INT suffix since you are using __typeof__, etc. I would otherwise read _INT as meaning the arguments are of type int.

103

Maybe s/long long/intmax_t/? We tend to use [u]intmax_t and %j for printing arbitrary integers rather than long long.

111

The reason it used write(2) previously was to avoid dealing with any unflushed stdio buffers after fork() I think. I feel like I got duplicate output previously as I probably wouldn't have resorted to that just due to paranoia.

arichardson marked 4 inline comments as done.

Apply review suggestions

tests/sys/kern/ptrace_test.c
97

Yeah I was thinking it means "any type that can meaningfully be cast to an integer". Will update the diff to remove the suffix shortly.

Unless there are any further suggestions, I'll commit this tomorrow (since there are two dependent changes that unbreak the testsuite).

tests/sys/kern/ptrace_test.c
123

I think you still don't need this and can just use ATF_REQUIRE_EQ() directly instead?

tests/sys/kern/ptrace_test.c
123

The reason I did this is that ATF_REQUIRE_EQ is stupid and doesn't print any useful information.

jhb added inline comments.
tests/sys/kern/ptrace_test.c
123

Humm, maybe we should fix those to include the values as well as the source name? I guess it is easy to convert these in the future if someone fixes that. A proper ATF_REQUIRE_EQ would have to do some switches on types though to use %p for pointers vs %jd for scalars and something else for floats.

This revision is now accepted and ready to land.Mon, Mar 1, 6:41 PM