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