| Message ID | 20241205-fix_warnings_pointer_masking_tests-v4-1-0c77eb725486@rivosinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] riscv: selftests: Fix warnings pointer masking test | expand |
On Thu, Dec 05, 2024 at 01:49:31PM -0800, Charlie Jenkins wrote: > When compiling the pointer masking tests with -Wall this warning > is present: > > pointer_masking.c: In function ‘test_tagged_addr_abi_sysctl’: > pointer_masking.c:203:9: warning: ignoring return value of ‘pwrite’ > declared with attribute ‘warn_unused_result’ [-Wunused-result] > 203 | pwrite(fd, &value, 1, 0); | > ^~~~~~~~~~~~~~~~~~~~~~~~ pointer_masking.c:208:9: warning: > ignoring return value of ‘pwrite’ declared with attribute > ‘warn_unused_result’ [-Wunused-result] > 208 | pwrite(fd, &value, 1, 0); > > I came across this on riscv64-linux-gnu-gcc (Ubuntu > 11.4.0-1ubuntu1~22.04). > > Fix this by checking that the number of bytes written equal the expected > number of bytes written. > > Fixes: 7470b5afd150 ("riscv: selftests: Add a pointer masking test") > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > --- > Changes in v4: > - Skip sysctl_enabled test if first pwrite failed > - Link to v3: https://lore.kernel.org/r/20241205-fix_warnings_pointer_masking_tests-v3-1-5c28b0f9640d@rivosinc.com > > Changes in v3: > - Fix sysctl enabled test case (Drew/Alex) > - Move pwrite err condition into goto (Drew) > - Link to v2: https://lore.kernel.org/r/20241204-fix_warnings_pointer_masking_tests-v2-1-1bf0c5095f58@rivosinc.com > > Changes in v2: > - I had ret != 2 for testing, I changed it to be ret != 1. > - Link to v1: https://lore.kernel.org/r/20241204-fix_warnings_pointer_masking_tests-v1-1-ea1e9665ce7a@rivosinc.com > --- > tools/testing/selftests/riscv/abi/pointer_masking.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/riscv/abi/pointer_masking.c b/tools/testing/selftests/riscv/abi/pointer_masking.c > index dee41b7ee3e3..759445d5f265 100644 > --- a/tools/testing/selftests/riscv/abi/pointer_masking.c > +++ b/tools/testing/selftests/riscv/abi/pointer_masking.c > @@ -189,6 +189,8 @@ static void test_tagged_addr_abi_sysctl(void) > { > char value; > int fd; > + int ret; > + char *err_pwrite_msg = "failed to write to /proc/sys/abi/tagged_addr_disabled\n"; > > ksft_print_msg("Testing tagged address ABI sysctl\n"); > > @@ -200,18 +202,32 @@ static void test_tagged_addr_abi_sysctl(void) > } > > value = '1'; > - pwrite(fd, &value, 1, 0); > + ret = pwrite(fd, &value, 1, 0); > + if (ret != 1) { > + ksft_test_result_skip(err_pwrite_msg); It seems like we should have a better way to keep the count balanced than to require a ksft_test_result_skip() call for each test on each error path. Every time we add a test we'll have to go add skips everywhere else. > + goto err_pwrite; > + } > + > ksft_test_result(set_tagged_addr_ctrl(min_pmlen, true) == -EINVAL, > "sysctl disabled\n"); > > value = '0'; > - pwrite(fd, &value, 1, 0); > + ret = pwrite(fd, &value, 1, 0); > + if (ret != 1) > + goto err_pwrite; > + > ksft_test_result(set_tagged_addr_ctrl(min_pmlen, true) == 0, > "sysctl enabled\n"); > > set_tagged_addr_ctrl(0, false); > > close(fd); > + > + return; > + > +err_pwrite: > + close(fd); > + ksft_test_result_fail(err_pwrite_msg); > } I don't think the goto reduces much code or improves readability much. A wrapper function should do better. I was thinking something like static bool pwrite_wrapper(int fd, void *buf, size_t count, const char *msg) { int ret = pwrite(fd, buf, count, 0); if (ret != count) { ksft_perror(msg); return false; } return true; } value = '1'; if (!pwrite_wrapper(fd, &value, 1, "write '1'")) ksft_test_result_fail(...); value = '0'; if (!pwrite_wrapper(fd, &value, 1, "write '0'")) ksft_test_result_fail(...); > > static void test_tagged_addr_abi_pmlen(int pmlen) > > --- > base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37 > change-id: 20241204-fix_warnings_pointer_masking_tests-3860e4f35429 > -- > - Charlie > Thanks, drew
On Fri, Dec 06, 2024 at 10:15:17AM +0100, Andrew Jones wrote: > On Thu, Dec 05, 2024 at 01:49:31PM -0800, Charlie Jenkins wrote: > > When compiling the pointer masking tests with -Wall this warning > > is present: > > > > pointer_masking.c: In function ‘test_tagged_addr_abi_sysctl’: > > pointer_masking.c:203:9: warning: ignoring return value of ‘pwrite’ > > declared with attribute ‘warn_unused_result’ [-Wunused-result] > > 203 | pwrite(fd, &value, 1, 0); | > > ^~~~~~~~~~~~~~~~~~~~~~~~ pointer_masking.c:208:9: warning: > > ignoring return value of ‘pwrite’ declared with attribute > > ‘warn_unused_result’ [-Wunused-result] > > 208 | pwrite(fd, &value, 1, 0); > > > > I came across this on riscv64-linux-gnu-gcc (Ubuntu > > 11.4.0-1ubuntu1~22.04). > > > > Fix this by checking that the number of bytes written equal the expected > > number of bytes written. > > > > Fixes: 7470b5afd150 ("riscv: selftests: Add a pointer masking test") > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > --- > > Changes in v4: > > - Skip sysctl_enabled test if first pwrite failed > > - Link to v3: https://lore.kernel.org/r/20241205-fix_warnings_pointer_masking_tests-v3-1-5c28b0f9640d@rivosinc.com > > > > Changes in v3: > > - Fix sysctl enabled test case (Drew/Alex) > > - Move pwrite err condition into goto (Drew) > > - Link to v2: https://lore.kernel.org/r/20241204-fix_warnings_pointer_masking_tests-v2-1-1bf0c5095f58@rivosinc.com > > > > Changes in v2: > > - I had ret != 2 for testing, I changed it to be ret != 1. > > - Link to v1: https://lore.kernel.org/r/20241204-fix_warnings_pointer_masking_tests-v1-1-ea1e9665ce7a@rivosinc.com > > --- > > tools/testing/selftests/riscv/abi/pointer_masking.c | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/riscv/abi/pointer_masking.c b/tools/testing/selftests/riscv/abi/pointer_masking.c > > index dee41b7ee3e3..759445d5f265 100644 > > --- a/tools/testing/selftests/riscv/abi/pointer_masking.c > > +++ b/tools/testing/selftests/riscv/abi/pointer_masking.c > > @@ -189,6 +189,8 @@ static void test_tagged_addr_abi_sysctl(void) > > { > > char value; > > int fd; > > + int ret; > > + char *err_pwrite_msg = "failed to write to /proc/sys/abi/tagged_addr_disabled\n"; > > > > ksft_print_msg("Testing tagged address ABI sysctl\n"); > > > > @@ -200,18 +202,32 @@ static void test_tagged_addr_abi_sysctl(void) > > } > > > > value = '1'; > > - pwrite(fd, &value, 1, 0); > > + ret = pwrite(fd, &value, 1, 0); > > + if (ret != 1) { > > + ksft_test_result_skip(err_pwrite_msg); > > It seems like we should have a better way to keep the count balanced than > to require a ksft_test_result_skip() call for each test on each error > path. Every time we add a test we'll have to go add skips everywhere else. It's only a problem if there are multiple tests in a single test function like there is here. Since the tests disable then reenable it makes sense to have them in one function, but does require us to do the skipping. > > > + goto err_pwrite; > > + } > > + > > ksft_test_result(set_tagged_addr_ctrl(min_pmlen, true) == -EINVAL, > > "sysctl disabled\n"); > > > > value = '0'; > > - pwrite(fd, &value, 1, 0); > > + ret = pwrite(fd, &value, 1, 0); > > + if (ret != 1) > > + goto err_pwrite; > > + > > ksft_test_result(set_tagged_addr_ctrl(min_pmlen, true) == 0, > > "sysctl enabled\n"); > > > > set_tagged_addr_ctrl(0, false); > > > > close(fd); > > + > > + return; > > + > > +err_pwrite: > > + close(fd); > > + ksft_test_result_fail(err_pwrite_msg); > > } > > I don't think the goto reduces much code or improves readability much. A > wrapper function should do better. I was thinking something like > > static bool pwrite_wrapper(int fd, void *buf, size_t count, const char *msg) > { > int ret = pwrite(fd, buf, count, 0); > if (ret != count) { > ksft_perror(msg); > return false; > } > return true; > } > > > value = '1'; > if (!pwrite_wrapper(fd, &value, 1, "write '1'")) > ksft_test_result_fail(...); > > value = '0'; > if (!pwrite_wrapper(fd, &value, 1, "write '0'")) > ksft_test_result_fail(...); > > Will do, thanks! - Charlie > > > > static void test_tagged_addr_abi_pmlen(int pmlen) > > > > --- > > base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37 > > change-id: 20241204-fix_warnings_pointer_masking_tests-3860e4f35429 > > -- > > - Charlie > > > > Thanks, > drew
On Fri, Dec 06, 2024 at 09:21:50AM -0800, Charlie Jenkins wrote: > On Fri, Dec 06, 2024 at 10:15:17AM +0100, Andrew Jones wrote: > > On Thu, Dec 05, 2024 at 01:49:31PM -0800, Charlie Jenkins wrote: > > > When compiling the pointer masking tests with -Wall this warning > > > is present: > > > > > > pointer_masking.c: In function ‘test_tagged_addr_abi_sysctl’: > > > pointer_masking.c:203:9: warning: ignoring return value of ‘pwrite’ > > > declared with attribute ‘warn_unused_result’ [-Wunused-result] > > > 203 | pwrite(fd, &value, 1, 0); | > > > ^~~~~~~~~~~~~~~~~~~~~~~~ pointer_masking.c:208:9: warning: > > > ignoring return value of ‘pwrite’ declared with attribute > > > ‘warn_unused_result’ [-Wunused-result] > > > 208 | pwrite(fd, &value, 1, 0); > > > > > > I came across this on riscv64-linux-gnu-gcc (Ubuntu > > > 11.4.0-1ubuntu1~22.04). > > > > > > Fix this by checking that the number of bytes written equal the expected > > > number of bytes written. > > > > > > Fixes: 7470b5afd150 ("riscv: selftests: Add a pointer masking test") > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > > --- > > > Changes in v4: > > > - Skip sysctl_enabled test if first pwrite failed > > > - Link to v3: https://lore.kernel.org/r/20241205-fix_warnings_pointer_masking_tests-v3-1-5c28b0f9640d@rivosinc.com > > > > > > Changes in v3: > > > - Fix sysctl enabled test case (Drew/Alex) > > > - Move pwrite err condition into goto (Drew) > > > - Link to v2: https://lore.kernel.org/r/20241204-fix_warnings_pointer_masking_tests-v2-1-1bf0c5095f58@rivosinc.com > > > > > > Changes in v2: > > > - I had ret != 2 for testing, I changed it to be ret != 1. > > > - Link to v1: https://lore.kernel.org/r/20241204-fix_warnings_pointer_masking_tests-v1-1-ea1e9665ce7a@rivosinc.com > > > --- > > > tools/testing/selftests/riscv/abi/pointer_masking.c | 20 ++++++++++++++++++-- > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > diff --git a/tools/testing/selftests/riscv/abi/pointer_masking.c b/tools/testing/selftests/riscv/abi/pointer_masking.c > > > index dee41b7ee3e3..759445d5f265 100644 > > > --- a/tools/testing/selftests/riscv/abi/pointer_masking.c > > > +++ b/tools/testing/selftests/riscv/abi/pointer_masking.c > > > @@ -189,6 +189,8 @@ static void test_tagged_addr_abi_sysctl(void) > > > { > > > char value; > > > int fd; > > > + int ret; > > > + char *err_pwrite_msg = "failed to write to /proc/sys/abi/tagged_addr_disabled\n"; > > > > > > ksft_print_msg("Testing tagged address ABI sysctl\n"); > > > > > > @@ -200,18 +202,32 @@ static void test_tagged_addr_abi_sysctl(void) > > > } > > > > > > value = '1'; > > > - pwrite(fd, &value, 1, 0); > > > + ret = pwrite(fd, &value, 1, 0); > > > + if (ret != 1) { > > > + ksft_test_result_skip(err_pwrite_msg); > > > > It seems like we should have a better way to keep the count balanced than > > to require a ksft_test_result_skip() call for each test on each error > > path. Every time we add a test we'll have to go add skips everywhere else. > > It's only a problem if there are multiple tests in a single test > function like there is here. Since the tests disable then reenable it > makes sense to have them in one function, but does require us to do the > skipping. I guess it is sufficient to leave out the skip here, if the first one fails we can just continue and let the second one fail too. - Charlie > > > > > > + goto err_pwrite; > > > + } > > > + > > > ksft_test_result(set_tagged_addr_ctrl(min_pmlen, true) == -EINVAL, > > > "sysctl disabled\n"); > > > > > > value = '0'; > > > - pwrite(fd, &value, 1, 0); > > > + ret = pwrite(fd, &value, 1, 0); > > > + if (ret != 1) > > > + goto err_pwrite; > > > + > > > ksft_test_result(set_tagged_addr_ctrl(min_pmlen, true) == 0, > > > "sysctl enabled\n"); > > > > > > set_tagged_addr_ctrl(0, false); > > > > > > close(fd); > > > + > > > + return; > > > + > > > +err_pwrite: > > > + close(fd); > > > + ksft_test_result_fail(err_pwrite_msg); > > > } > > > > I don't think the goto reduces much code or improves readability much. A > > wrapper function should do better. I was thinking something like > > > > static bool pwrite_wrapper(int fd, void *buf, size_t count, const char *msg) > > { > > int ret = pwrite(fd, buf, count, 0); > > if (ret != count) { > > ksft_perror(msg); > > return false; > > } > > return true; > > } > > > > > > value = '1'; > > if (!pwrite_wrapper(fd, &value, 1, "write '1'")) > > ksft_test_result_fail(...); > > > > value = '0'; > > if (!pwrite_wrapper(fd, &value, 1, "write '0'")) > > ksft_test_result_fail(...); > > > > > > Will do, thanks! > > - Charlie > > > > > > > static void test_tagged_addr_abi_pmlen(int pmlen) > > > > > > --- > > > base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37 > > > change-id: 20241204-fix_warnings_pointer_masking_tests-3860e4f35429 > > > -- > > > - Charlie > > > > > > > Thanks, > > drew
diff --git a/tools/testing/selftests/riscv/abi/pointer_masking.c b/tools/testing/selftests/riscv/abi/pointer_masking.c index dee41b7ee3e3..759445d5f265 100644 --- a/tools/testing/selftests/riscv/abi/pointer_masking.c +++ b/tools/testing/selftests/riscv/abi/pointer_masking.c @@ -189,6 +189,8 @@ static void test_tagged_addr_abi_sysctl(void) { char value; int fd; + int ret; + char *err_pwrite_msg = "failed to write to /proc/sys/abi/tagged_addr_disabled\n"; ksft_print_msg("Testing tagged address ABI sysctl\n"); @@ -200,18 +202,32 @@ static void test_tagged_addr_abi_sysctl(void) } value = '1'; - pwrite(fd, &value, 1, 0); + ret = pwrite(fd, &value, 1, 0); + if (ret != 1) { + ksft_test_result_skip(err_pwrite_msg); + goto err_pwrite; + } + ksft_test_result(set_tagged_addr_ctrl(min_pmlen, true) == -EINVAL, "sysctl disabled\n"); value = '0'; - pwrite(fd, &value, 1, 0); + ret = pwrite(fd, &value, 1, 0); + if (ret != 1) + goto err_pwrite; + ksft_test_result(set_tagged_addr_ctrl(min_pmlen, true) == 0, "sysctl enabled\n"); set_tagged_addr_ctrl(0, false); close(fd); + + return; + +err_pwrite: + close(fd); + ksft_test_result_fail(err_pwrite_msg); } static void test_tagged_addr_abi_pmlen(int pmlen)
When compiling the pointer masking tests with -Wall this warning is present: pointer_masking.c: In function ‘test_tagged_addr_abi_sysctl’: pointer_masking.c:203:9: warning: ignoring return value of ‘pwrite’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 203 | pwrite(fd, &value, 1, 0); | ^~~~~~~~~~~~~~~~~~~~~~~~ pointer_masking.c:208:9: warning: ignoring return value of ‘pwrite’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 208 | pwrite(fd, &value, 1, 0); I came across this on riscv64-linux-gnu-gcc (Ubuntu 11.4.0-1ubuntu1~22.04). Fix this by checking that the number of bytes written equal the expected number of bytes written. Fixes: 7470b5afd150 ("riscv: selftests: Add a pointer masking test") Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> --- Changes in v4: - Skip sysctl_enabled test if first pwrite failed - Link to v3: https://lore.kernel.org/r/20241205-fix_warnings_pointer_masking_tests-v3-1-5c28b0f9640d@rivosinc.com Changes in v3: - Fix sysctl enabled test case (Drew/Alex) - Move pwrite err condition into goto (Drew) - Link to v2: https://lore.kernel.org/r/20241204-fix_warnings_pointer_masking_tests-v2-1-1bf0c5095f58@rivosinc.com Changes in v2: - I had ret != 2 for testing, I changed it to be ret != 1. - Link to v1: https://lore.kernel.org/r/20241204-fix_warnings_pointer_masking_tests-v1-1-ea1e9665ce7a@rivosinc.com --- tools/testing/selftests/riscv/abi/pointer_masking.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) --- base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37 change-id: 20241204-fix_warnings_pointer_masking_tests-3860e4f35429