diff mbox series

Use __nonnull for the epoll_wait(2) family of syscalls

Message ID 20230522220120.37208-1-alx@kernel.org
State New
Headers show
Series Use __nonnull for the epoll_wait(2) family of syscalls | expand

Commit Message

Alejandro Colomar May 22, 2023, 10:01 p.m. UTC 
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(-)

Comments

Adhemerval Zanella Netto May 23, 2023, 12:27 p.m. UTC | #1 
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
Adhemerval Zanella Netto May 29, 2023, 5:39 p.m. UTC | #2 
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
Alejandro Colomar May 29, 2023, 11:17 p.m. UTC | #3 
Hi Adhemerval!

On 5/29/23 19:39, Adhemerval Zanella Netto wrote:
> 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.

I didn't catch it either.  I didn't run tests, since I thought if anything failed, it
would probably be at compilation stage, so I only compiled.  Not sure about the
buildbots, though; those should have caught it.

> 
> 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).

I suggest a slightly different approach: passing the address of a dummy variable.
It is even simpler.  See the suggested diff below.  If you like it, I'll send a
revision with it (which IIRC, should be v2).

Cheers,
Alex

$ git diff
diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
index 66f091c202..abda45c427 100644
--- a/sysdeps/unix/sysv/linux/tst-epoll.c
+++ b/sysdeps/unix/sysv/linux/tst-epoll.c
@@ -180,6 +180,8 @@ epoll_pwait2_check (int epfd, struct epoll_event *ev, int maxev, int tmo,
 static int
 do_test (void)
 {
+  struct epoll_event ev;
+
   {
     struct sigaction sa;
     sa.sa_handler = handler;
@@ -191,7 +193,7 @@ do_test (void)
     xsigaction (SIGCHLD, &sa, NULL);
   }
 
-  int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
+  int r = epoll_pwait2 (-1, &ev, 0, NULL, NULL);
   TEST_COMPARE (r, -1);
   bool pwait2_supported = errno != ENOSYS;
Adhemerval Zanella Netto May 30, 2023, 11:41 a.m. UTC | #4 
On 29/05/23 20:17, Alejandro Colomar wrote:
> Hi Adhemerval!
> 
> On 5/29/23 19:39, Adhemerval Zanella Netto wrote:
>> 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.
> 
> I didn't catch it either.  I didn't run tests, since I thought if anything failed, it
> would probably be at compilation stage, so I only compiled.  Not sure about the
> buildbots, though; those should have caught it.

We consider tests build failures as regression as well, at least the fix is
catching bogus usages.  You can just build the tests without running by
issuing 'make check run-built-tests=no', which speed up tests for this
kind of changes.

> 
>>
>> 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).
> 
> I suggest a slightly different approach: passing the address of a dummy variable.
> It is even simpler.  See the suggested diff below.  If you like it, I'll send a
> revision with it (which IIRC, should be v2).

Either version works for me, thanks!

> 
> Cheers,
> Alex
> 
> $ git diff
> diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
> index 66f091c202..abda45c427 100644
> --- a/sysdeps/unix/sysv/linux/tst-epoll.c
> +++ b/sysdeps/unix/sysv/linux/tst-epoll.c
> @@ -180,6 +180,8 @@ epoll_pwait2_check (int epfd, struct epoll_event *ev, int maxev, int tmo,
>  static int
>  do_test (void)
>  {
> +  struct epoll_event ev;
> +
>    {
>      struct sigaction sa;
>      sa.sa_handler = handler;
> @@ -191,7 +193,7 @@ do_test (void)
>      xsigaction (SIGCHLD, &sa, NULL);
>    }
>  
> -  int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
> +  int r = epoll_pwait2 (-1, &ev, 0, NULL, NULL);
>    TEST_COMPARE (r, -1);
>    bool pwait2_supported = errno != ENOSYS;
>  
>
diff mbox series

Patch

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