Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] The `check_name()` function evaluates negative error codes as test successes due to `EXPECT_TRUE()` behavior. - [Medium] The test fails rather than gracefully skipping on older kernels lacking `PR_SET_EXT_NAME` support. --
commit 56060d6784146397018fe8a302145c26fb029757 Author: André Almeida <[email protected]> Subject: selftests: prctl: Add test for long thread names This commit adds tests for setting and getting long thread names using the new extended PR_SET_EXT_NAME and PR_GET_EXT_NAME prctl interfaces. It also adjusts the check_is_name_correct() function to prevent negative error codes from being evaluated as successes by EXPECT_TRUE(). > diff --git a/tools/testing/selftests/prctl/set-process-name.c > b/tools/testing/selftests/prctl/set-process-name.c > --- a/tools/testing/selftests/prctl/set-process-name.c > +++ b/tools/testing/selftests/prctl/set-process-name.c [ ... ] > @@ -80,10 +118,15 @@ int check_name(void) > TEST(rename_process) { > > EXPECT_GE(set_name(CHANGE_NAME), 0); > - EXPECT_TRUE(check_is_name_correct(CHANGE_NAME)); > + EXPECT_FALSE(check_is_name_correct(CHANGE_NAME)); > + > + EXPECT_GE(set_ext_name(LONG_NAME), 0); [Severity: Medium] Will this test cause a failure rather than gracefully skipping on older kernels? If the test runs on a kernel where the PR_SET_EXT_NAME interface is unsupported, set_ext_name() returns a negative error code like -EINVAL. The EXPECT_GE assertion will fail the test entirely. Should this test gracefully skip when run on kernels lacking support for these new extensions? > + EXPECT_FALSE(check_is_ext_name_correct(LONG_NAME)); > + EXPECT_FALSE(check_is_name_correct(LONG_NAME_CAP)); > + EXPECT_TRUE(check_name()); [Severity: Medium] Does this new test invocation suffer from the same logic error being fixed in check_is_name_correct()? The commit message explicitly mentions fixing check_is_name_correct() because EXPECT_TRUE() evaluates negative error codes as success. While the flawed logic in the check_name() function is a pre-existing issue, it returns -EIO on file open failure and -errno on prctl failure: tools/testing/selftests/prctl/set-process-name.c:check_name() { ... if (!fptr) return -EIO; ... if (res < 0) return -errno; ... } Since this patch introduces a new EXPECT_TRUE(check_name()) call here, won't any file open or prctl failure be falsely evaluated as a test success? Could we update check_name() as well to prevent this flaw from propagating? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
