* nonnull on epoll_wait(2) syscall wrappers?
@ 2023-05-19 22:50 Alejandro Colomar
2023-05-22 20:51 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 12+ messages in thread
From: Alejandro Colomar @ 2023-05-19 22:50 UTC (permalink / raw)
To: Adhemerval Zanella, GNU C Library
[-- Attachment #1.1: Type: text/plain, Size: 2138 bytes --]
Hi Adhemerval,
I was checking the epoll_pwait2(2) man page to see if it needs some
updating for the wrapper you added recently. It seems all's good.
<https://sourceware.org/bugzilla/show_bug.cgi?id=27359>
However, I noticed that there's a difference between the current
manual page and the glibc wrappers for the 3 related functions:
$ grepc -x /sys/epoll.h$ epoll_wait /usr/include/
/usr/include/x86_64-linux-gnu/sys/epoll.h:124:
extern int epoll_wait (int __epfd, struct epoll_event *__events,
int __maxevents, int __timeout)
__attr_access ((__write_only__, 2, 3));
$ grepc -x /sys/epoll.h$ epoll_pwait /usr/include/
/usr/include/x86_64-linux-gnu/sys/epoll.h:134:
extern int epoll_pwait (int __epfd, struct epoll_event *__events,
int __maxevents, int __timeout,
const __sigset_t *__ss)
__attr_access ((__write_only__, 2, 3));
$ grepc -x /sys/epoll.h$ epoll_pwait2 /usr/include/
/usr/include/x86_64-linux-gnu/sys/epoll.h:144:
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));
/usr/include/x86_64-linux-gnu/sys/epoll.h:157:
# define epoll_pwait2 __epoll_pwait2_time64
The man page synopsis is:
SYNOPSIS
#include <sys/epoll.h>
int epoll_wait(int epfd, struct epoll_event *events,
int maxevents, int timeout);
int epoll_pwait(int epfd, struct epoll_event *events,
int maxevents, int timeout,
const sigset_t *_Nullable sigmask);
int epoll_pwait2(int epfd, struct epoll_event *events,
int maxevents, const struct timespec *_Nullable timeout,
const sigset_t *_Nullable sigmask);
I didn't use _Nullable in the events parameter, because I don't think
it can be NULL. Does it make any sense in any case having a NULL
there? Should we use nonnull in glibc?
Cheers,
Alex
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: nonnull on epoll_wait(2) syscall wrappers?
2023-05-19 22:50 nonnull on epoll_wait(2) syscall wrappers? Alejandro Colomar
@ 2023-05-22 20:51 ` Adhemerval Zanella Netto
2023-05-22 22:01 ` [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls Alejandro Colomar
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Adhemerval Zanella Netto @ 2023-05-22 20:51 UTC (permalink / raw)
To: Alejandro Colomar, GNU C Library
On 19/05/23 19:50, Alejandro Colomar wrote:
> Hi Adhemerval,
>
> I was checking the epoll_pwait2(2) man page to see if it needs some
> updating for the wrapper you added recently. It seems all's good.
>
> <https://sourceware.org/bugzilla/show_bug.cgi?id=27359>
>
> However, I noticed that there's a difference between the current
> manual page and the glibc wrappers for the 3 related functions:
>
>
> $ grepc -x /sys/epoll.h$ epoll_wait /usr/include/
> /usr/include/x86_64-linux-gnu/sys/epoll.h:124:
> extern int epoll_wait (int __epfd, struct epoll_event *__events,
> int __maxevents, int __timeout)
> __attr_access ((__write_only__, 2, 3));
> $ grepc -x /sys/epoll.h$ epoll_pwait /usr/include/
> /usr/include/x86_64-linux-gnu/sys/epoll.h:134:
> extern int epoll_pwait (int __epfd, struct epoll_event *__events,
> int __maxevents, int __timeout,
> const __sigset_t *__ss)
> __attr_access ((__write_only__, 2, 3));
> $ grepc -x /sys/epoll.h$ epoll_pwait2 /usr/include/
> /usr/include/x86_64-linux-gnu/sys/epoll.h:144:
> 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));
>
>
> /usr/include/x86_64-linux-gnu/sys/epoll.h:157:
> # define epoll_pwait2 __epoll_pwait2_time64
>
>
> The man page synopsis is:
>
>
> SYNOPSIS
> #include <sys/epoll.h>
>
> int epoll_wait(int epfd, struct epoll_event *events,
> int maxevents, int timeout);
> int epoll_pwait(int epfd, struct epoll_event *events,
> int maxevents, int timeout,
> const sigset_t *_Nullable sigmask);
> int epoll_pwait2(int epfd, struct epoll_event *events,
> int maxevents, const struct timespec *_Nullable timeout,
> const sigset_t *_Nullable sigmask);
>
>
> I didn't use _Nullable in the events parameter, because I don't think
> it can be NULL. Does it make any sense in any case having a NULL
> there? Should we use nonnull in glibc?
It does make sense to mark the epoll_events argument as nonnull, since
either the kernel will return EINVAL for maxevents equal to 0 or
EFAULT for invalid events:
fs/eventpoll.c
2284 * Implement the event wait interface for the eventpoll file. It is the kernel
2285 * part of the user space epoll_wait(2).
2286 */
2287 static int do_epoll_wait(int epfd, struct epoll_event __user *events,
2288 int maxevents, struct timespec64 *to)
2289 {
2290 int error;
2291 struct fd f;
2292 struct eventpoll *ep;
2293
2294 /* The maximum number of event must be greater than zero */
2295 if (maxevents <= 0 || maxevents > EP_MAX_EVENTS)
2296 return -EINVAL;
2297
2298 /* Verify that the area passed by the user is writeable */
2299 if (!access_ok(events, maxevents * sizeof(struct epoll_event)))
2300 return -EFAULT;
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls
2023-05-22 20:51 ` Adhemerval Zanella Netto
@ 2023-05-22 22:01 ` Alejandro Colomar
2023-05-23 12:27 ` Adhemerval Zanella Netto
2023-05-31 20:44 ` [PATCH v2 1/2] Fix invalid use of NULL in epoll_pwait2(2) test Alejandro Colomar
2023-05-31 20:44 ` [PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls Alejandro Colomar
2 siblings, 1 reply; 12+ messages in thread
From: Alejandro Colomar @ 2023-05-22 22:01 UTC (permalink / raw)
To: libc-alpha; +Cc: Alejandro Colomar, Adhemerval Zanella Netto
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
--
2.40.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls
2023-05-22 22:01 ` [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls Alejandro Colomar
@ 2023-05-23 12:27 ` Adhemerval Zanella Netto
2023-05-29 17:39 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 12+ messages in thread
From: Adhemerval Zanella Netto @ 2023-05-23 12:27 UTC (permalink / raw)
To: Alejandro Colomar, libc-alpha; +Cc: Alejandro Colomar
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls
2023-05-23 12:27 ` Adhemerval Zanella Netto
@ 2023-05-29 17:39 ` Adhemerval Zanella Netto
2023-05-29 23:17 ` Alejandro Colomar
0 siblings, 1 reply; 12+ messages in thread
From: Adhemerval Zanella Netto @ 2023-05-29 17:39 UTC (permalink / raw)
To: Alejandro Colomar, libc-alpha; +Cc: Alejandro Colomar
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls
2023-05-29 17:39 ` Adhemerval Zanella Netto
@ 2023-05-29 23:17 ` Alejandro Colomar
2023-05-30 11:41 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 12+ messages in thread
From: Alejandro Colomar @ 2023-05-29 23:17 UTC (permalink / raw)
To: Adhemerval Zanella Netto, libc-alpha; +Cc: Alejandro Colomar
[-- Attachment #1.1: Type: text/plain, Size: 3574 bytes --]
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;
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls
2023-05-29 23:17 ` Alejandro Colomar
@ 2023-05-30 11:41 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 12+ messages in thread
From: Adhemerval Zanella Netto @ 2023-05-30 11:41 UTC (permalink / raw)
To: Alejandro Colomar, libc-alpha; +Cc: Alejandro Colomar
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;
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] Fix invalid use of NULL in epoll_pwait2(2) test
2023-05-22 20:51 ` Adhemerval Zanella Netto
2023-05-22 22:01 ` [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls Alejandro Colomar
@ 2023-05-31 20:44 ` Alejandro Colomar
2023-06-01 17:59 ` Adhemerval Zanella Netto
2023-05-31 20:44 ` [PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls Alejandro Colomar
2 siblings, 1 reply; 12+ messages in thread
From: Alejandro Colomar @ 2023-05-31 20:44 UTC (permalink / raw)
To: libc-alpha; +Cc: Alejandro Colomar, Adhemerval Zanella Netto
epoll_pwait2(2)'s second argument should be nonnull. We're going to add
__nonnull to the prototype, so let's fix the test accordingly. We can
use a dummy variable to avoid passing NULL.
Reported-by: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
sysdeps/unix/sysv/linux/tst-epoll.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
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;
--
2.40.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls
2023-05-22 20:51 ` Adhemerval Zanella Netto
2023-05-22 22:01 ` [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls Alejandro Colomar
2023-05-31 20:44 ` [PATCH v2 1/2] Fix invalid use of NULL in epoll_pwait2(2) test Alejandro Colomar
@ 2023-05-31 20:44 ` Alejandro Colomar
2023-06-01 18:00 ` Adhemerval Zanella Netto
2 siblings, 1 reply; 12+ messages in thread
From: Alejandro Colomar @ 2023-05-31 20:44 UTC (permalink / raw)
To: libc-alpha; +Cc: Alejandro Colomar, Adhemerval Zanella Netto
Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
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
--
2.40.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] Fix invalid use of NULL in epoll_pwait2(2) test
2023-05-31 20:44 ` [PATCH v2 1/2] Fix invalid use of NULL in epoll_pwait2(2) test Alejandro Colomar
@ 2023-06-01 17:59 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 12+ messages in thread
From: Adhemerval Zanella Netto @ 2023-06-01 17:59 UTC (permalink / raw)
To: Alejandro Colomar, libc-alpha; +Cc: Alejandro Colomar
On 31/05/23 17:44, Alejandro Colomar wrote:
> epoll_pwait2(2)'s second argument should be nonnull. We're going to add
> __nonnull to the prototype, so let's fix the test accordingly. We can
> use a dummy variable to avoid passing NULL.
>
> Reported-by: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> sysdeps/unix/sysv/linux/tst-epoll.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> 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;
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls
2023-05-31 20:44 ` [PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls Alejandro Colomar
@ 2023-06-01 18:00 ` Adhemerval Zanella Netto
2023-06-01 23:46 ` Alejandro Colomar
0 siblings, 1 reply; 12+ messages in thread
From: Adhemerval Zanella Netto @ 2023-06-01 18:00 UTC (permalink / raw)
To: Alejandro Colomar, libc-alpha; +Cc: Alejandro Colomar
On 31/05/23 17:44, Alejandro Colomar wrote:
> Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
LGTM, although the buildbot failed to apply the patch for some reason [1].
It does apply if I use git-pw.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
[1] https://patchwork.sourceware.org/project/glibc/patch/20230531204422.18052-2-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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls
2023-06-01 18:00 ` Adhemerval Zanella Netto
@ 2023-06-01 23:46 ` Alejandro Colomar
0 siblings, 0 replies; 12+ messages in thread
From: Alejandro Colomar @ 2023-06-01 23:46 UTC (permalink / raw)
To: Adhemerval Zanella Netto, libc-alpha; +Cc: Alejandro Colomar
[-- Attachment #1.1: Type: text/plain, Size: 894 bytes --]
On 6/1/23 20:00, Adhemerval Zanella Netto wrote:
>
>
> On 31/05/23 17:44, Alejandro Colomar wrote:
>> Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
>> Signed-off-by: Alejandro Colomar <alx@kernel.org>
>
>
> LGTM, although the buildbot failed to apply the patch for some reason [1].
Don't know; maybe I forgot to rebase to the latest master, and some context
changed, and it applied with fuzz... git can apply it, but the buildbot is
probably more cautious.
> It does apply if I use git-pw.
>
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Thank you!
(you forgot to actually put this tag in 1/2 :)
Cheers,
Alex
>
> [1] https://patchwork.sourceware.org/project/glibc/patch/20230531204422.18052-2-alx@kernel.org/
>
--
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-06-01 23:46 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-19 22:50 nonnull on epoll_wait(2) syscall wrappers? Alejandro Colomar
2023-05-22 20:51 ` Adhemerval Zanella Netto
2023-05-22 22:01 ` [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls Alejandro Colomar
2023-05-23 12:27 ` Adhemerval Zanella Netto
2023-05-29 17:39 ` Adhemerval Zanella Netto
2023-05-29 23:17 ` Alejandro Colomar
2023-05-30 11:41 ` Adhemerval Zanella Netto
2023-05-31 20:44 ` [PATCH v2 1/2] Fix invalid use of NULL in epoll_pwait2(2) test Alejandro Colomar
2023-06-01 17:59 ` Adhemerval Zanella Netto
2023-05-31 20:44 ` [PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls Alejandro Colomar
2023-06-01 18:00 ` Adhemerval Zanella Netto
2023-06-01 23:46 ` Alejandro Colomar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).