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-5...
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-1...
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-e... --- 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); + 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)
--- base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37 change-id: 20241204-fix_warnings_pointer_masking_tests-3860e4f35429
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-5...
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-1...
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-e...
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-5...
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-1...
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-e...
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-5...
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-1...
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-e...
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
linux-kselftest-mirror@lists.linaro.org
-
Andrew Jones -
Charlie Jenkins