Re: [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

In fact, checking I am seeing a regression:

../sysdeps/unix/sysv/linux/tst-epoll.c: In function ‘do_test’:
../sysdeps/unix/sysv/linux/tst-epoll.c:194:11: error: argument 2 null where non-null expected [-Werror=nonnull]
  194 |   int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
      |           ^~~~~~~~~~~~
In file included from ../include/sys/epoll.h:2,
                 from ../sysdeps/unix/sysv/linux/tst-epoll.c:27:
../sysdeps/unix/sysv/linux/sys/epoll.h:144:12: note: in a call to function ‘epoll_pwait2’ declared ‘nonnull’
  144 | extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
      |            ^~~~~~~~~~~~
cc1: all warnings being treated as errors

And I am not sure why it was not caught by buildbots.

The check is only for test for epoll_pwait2 support, so I think it would be simpler to
just suppress the warning:

diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
index 66f091c202..e2fd34e0e6 100644
--- a/sysdeps/unix/sysv/linux/tst-epoll.c
+++ b/sysdeps/unix/sysv/linux/tst-epoll.c
@@ -18,12 +18,13 @@

 #include <errno.h>
 #include <intprops.h>
+#include <libc-diag.h>
+#include <stdlib.h>
 #include <support/check.h>
 #include <support/support.h>
 #include <support/xsignal.h>
-#include <support/xunistd.h>
 #include <support/xtime.h>
-#include <stdlib.h>
+#include <support/xunistd.h>
 #include <sys/epoll.h>

 /* The test focus on checking if the timeout argument is correctly handled
@@ -191,7 +192,12 @@ do_test (void)
     xsigaction (SIGCHLD, &sa, NULL);
   }

+  /* The NULL tests here is only to check if epoll_pwait2 is supported by the
+     kernel and to simplify the rest of test.  */
+  DIAG_PUSH_NEEDS_COMMENT;
+  DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
   int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
+  DIAG_POP_NEEDS_COMMENT;
   TEST_COMPARE (r, -1);
   bool pwait2_supported = errno != ENOSYS;

Could you send a v3 with the change?  Another possibility is to remove the
pwait2_supported and handle it on the test itself (it would require more
extensive changes).

On 23/05/23 09:27, Adhemerval Zanella Netto wrote:
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> On 22/05/23 19:01, Alejandro Colomar wrote:
>> Signed-off-by: Alejandro Colomar <alx@kernel.org>
>> ---
>>  include/sys/epoll.h                 | 3 ++-
>>  sysdeps/unix/sysv/linux/sys/epoll.h | 8 ++++----
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/sys/epoll.h b/include/sys/epoll.h
>> index 8049381a26..b23bc9c7c0 100644
>> --- a/include/sys/epoll.h
>> +++ b/include/sys/epoll.h
>> @@ -9,7 +9,8 @@ libc_hidden_proto (epoll_pwait)
>>  #else
>>  extern int __epoll_pwait2_time64 (int fd, struct epoll_event *ev, int maxev,
>>  				  const struct __timespec64 *tmo,
>> -				  const sigset_t *s);
>> +				  const sigset_t *s)
>> +       __nonnull ((2));
>>  libc_hidden_proto (__epoll_pwait2_time64)
>>  #endif
>>  
>> diff --git a/sysdeps/unix/sysv/linux/sys/epoll.h b/sysdeps/unix/sysv/linux/sys/epoll.h
>> index b17d344e79..23872c9438 100644
>> --- a/sysdeps/unix/sysv/linux/sys/epoll.h
>> +++ b/sysdeps/unix/sysv/linux/sys/epoll.h
>> @@ -123,7 +123,7 @@ extern int epoll_ctl (int __epfd, int __op, int __fd,
>>     __THROW.  */
>>  extern int epoll_wait (int __epfd, struct epoll_event *__events,
>>  		       int __maxevents, int __timeout)
>> -	__attr_access ((__write_only__, 2, 3));
>> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>>  
>>  
>>  /* Same as epoll_wait, but the thread's signal mask is temporarily
>> @@ -134,7 +134,7 @@ extern int epoll_wait (int __epfd, struct epoll_event *__events,
>>  extern int epoll_pwait (int __epfd, struct epoll_event *__events,
>>  			int __maxevents, int __timeout,
>>  			const __sigset_t *__ss)
>> -	__attr_access ((__write_only__, 2, 3));
>> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>>  
>>  /* Same as epoll_pwait, but the timeout as a timespec.
>>  
>> @@ -144,7 +144,7 @@ extern int epoll_pwait (int __epfd, struct epoll_event *__events,
>>  extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
>>  			 int __maxevents, const struct timespec *__timeout,
>>  			 const __sigset_t *__ss)
>> -	__attr_access ((__write_only__, 2, 3));
>> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>>  #else
>>  # ifdef __REDIRECT
>>  extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
>> @@ -152,7 +152,7 @@ extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
>>  				      const struct timespec *__timeout,
>>  				      const __sigset_t *__ss),
>>  		       __epoll_pwait2_time64)
>> -	__attr_access ((__write_only__, 2, 3));
>> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>>  # else
>>  #  define epoll_pwait2 __epoll_pwait2_time64
>>  # endif