Thread (10 messages) 10 messages, 4 authors, 2020-08-11

Re: [PATCH bpf-next] bpf: fix compilation warning of selftests

From: Andrii Nakryiko <hidden>
Date: 2020-07-31 17:40:12
Also in: bpf, lkml

On Thu, Jul 30, 2020 at 11:18 PM Jianlin Lv [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Clang compiler version: 12.0.0
The following warning appears during the selftests/bpf compilation:

prog_tests/send_signal.c:51:3: warning: ignoring return value of ‘write’,
declared with attribute warn_unused_result [-Wunused-result]
   51 |   write(pipe_c2p[1], buf, 1);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~
prog_tests/send_signal.c:54:3: warning: ignoring return value of ‘read’,
declared with attribute warn_unused_result [-Wunused-result]
   54 |   read(pipe_p2c[0], buf, 1);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~
......

prog_tests/stacktrace_build_id_nmi.c:13:2: warning: ignoring return value
of ‘fscanf’,declared with attribute warn_unused_result [-Wunused-resul]
   13 |  fscanf(f, "%llu", &sample_freq);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

test_tcpnotify_user.c:133:2: warning:ignoring return value of ‘system’,
declared with attribute warn_unused_result [-Wunused-result]
  133 |  system(test_script);
      |  ^~~~~~~~~~~~~~~~~~~
test_tcpnotify_user.c:138:2: warning:ignoring return value of ‘system’,
declared with attribute warn_unused_result [-Wunused-result]
  138 |  system(test_script);
      |  ^~~~~~~~~~~~~~~~~~~
test_tcpnotify_user.c:143:2: warning:ignoring return value of ‘system’,
declared with attribute warn_unused_result [-Wunused-result]
  143 |  system(test_script);
      |  ^~~~~~~~~~~~~~~~~~~

Add code that fix compilation warning about ignoring return value and
handles any errors; Check return value of library`s API make the code
more secure.

Signed-off-by: Jianlin Lv <redacted>
---
 .../selftests/bpf/prog_tests/send_signal.c    | 37 ++++++++++++++-----
 .../bpf/prog_tests/stacktrace_build_id_nmi.c  |  3 +-
 .../selftests/bpf/test_tcpnotify_user.c       | 15 ++++++--
 3 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 504abb7bfb95..7a5272e4e810 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -48,22 +48,31 @@ static void test_send_signal_common(struct perf_event_attr *attr,
                close(pipe_p2c[1]); /* close write */

                /* notify parent signal handler is installed */
-               write(pipe_c2p[1], buf, 1);
+               if (CHECK_FAIL(write(pipe_c2p[1], buf, 1) != 1)) {
+                       perror("Child: write pipe error");
+                       goto close_out;
+               }
Please don't use CHECK_FAIL. Using CHECK is better for many reasons,
but it will also be shorter here (while still recording failure):


CHECK(write(pipe_c2p[1], buf, 1) != 1, "pipe_write", "err %d\n", -errno);

                /* make sure parent enabled bpf program to send_signal */
-               read(pipe_p2c[0], buf, 1);
+               if (CHECK_FAIL(read(pipe_p2c[0], buf, 1) != 1)) {
+                       perror("Child: read pipe error");
+                       goto close_out;
+               }
[...]
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help