Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] rtla: subcase SKIP treated as group skip in run_cleanup_cases() - [Medium] rtla: tests: invalid --version check in check-cleanup.sh skips all tests --
commit 39ad3784406cff716e91c58adc60f6311f089ea0 Author: Chaosheng Qu <[email protected]> rtla: add tests for tracefs instance cleanup This commit adds a SIGTERM handler to rtla so tracefs instances are cleaned up on termination, along with a new test suite verifying tracefs instance cleanup across abnormal exit paths. > diff --git a/tools/tracing/rtla/tests/cleanup.t > b/tools/tracing/rtla/tests/cleanup.t > new file mode 100644 > index 0000000000000..f3b495b0aee4e > --- /dev/null > +++ b/tools/tracing/rtla/tests/cleanup.t > @@ -0,0 +1,71 @@ [ ... ] > +run_cleanup_cases() { [ ... ] > + out=$($timeout_cmd "$helper" "$RTLA" 2>&1) > + rc=$? > + if [ $rc -eq 0 ]; then > + # The helper may skip individual cases; a group-level SKIP > means it > + # could not run at all. > + skip=$(printf '%s\n' "$out" | sed -n 's/^SKIP: //p' | head -n 1) > + if [ -n "$skip" ]; then > + echo "ok $ctr - instances cleaned up on SIGINT, timeout > and action failure # SKIP $skip" > + return > + fi > + printf '%s\n' "$out" | grep '^SKIP: ' | while read -r line; do > + echo "# ${line}" > + done > + echo "ok $ctr - instances cleaned up on SIGINT, timeout and > action failure" [Severity: Medium] In run_cleanup_cases(), sed -n 's/^SKIP: //p' matches any line in $out starting with "SKIP: ". When the action-failure subtest in tools/tracing/rtla/tests/scripts/check-cleanup.sh does not cross the stop-total threshold on an idle system, it logs: if [ "$action_rc" -eq 0 ]; then log "SKIP: action-failure: stop-total threshold was not crossed" Won't $skip be populated from that subtest skip even when all other subtests ran and passed, causing run_cleanup_cases() to report the entire TAP test as skipped and leaving the grep '^SKIP: ' loop right below it unreachable? [ ... ] > diff --git a/tools/tracing/rtla/tests/scripts/check-cleanup.sh > b/tools/tracing/rtla/tests/scripts/check-cleanup.sh > new file mode 100755 > index 0000000000000..1b4397123708e > --- /dev/null > +++ b/tools/tracing/rtla/tests/scripts/check-cleanup.sh > @@ -0,0 +1,197 @@ [ ... ] > +# Leave nothing behind from the test before it starts, and make sure the > +# command under test is actually usable. > +cleanup_all > +if ! "$RTLA" --version >/dev/null 2>&1; then > + log "SKIP: $RTLA is not usable" > + exit 0 > +fi [Severity: Medium] Does rtla support the --version flag? Looking at main() in tools/tracing/rtla/src/cli.c: if (strcmp(argv[1], "-h") == 0) rtla_usage(129); else if (strcmp(argv[1], "--help") == 0) rtla_usage(129); retval = run_tool_command(argc, argv, 1); if (retval) exit(0); usage: rtla_usage(129); It looks like passing --version falls through to rtla_usage(129) and exits with status 129. Won't this check always fail and cause check-cleanup.sh to skip the entire test suite on every run? [ ... ] ──── To understand more about how the conclusions weredrawn,Normally: - Stage 1 (goal): VF1 - Stage 2 (implementation): PVF1 ──── -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
