[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] accept4 tests: fix specified flags
From: |
Pino Toscano |
Subject: |
Re: [PATCH] accept4 tests: fix specified flags |
Date: |
Fri, 09 Oct 2015 15:38:05 +0200 |
User-agent: |
KMail/4.14.9 (Linux/4.1.6-100.fc21.x86_64; KDE/4.14.9; x86_64; ; ) |
On Thursday 08 October 2015 15:46:42 Pádraig Brady wrote:
> On 08/10/15 13:47, Pino Toscano wrote:
> > Pass only SOCK_* flags to accept4, as they are the only documented
> > ones, and passing others may trigger EINVAL.
> > * tests/test-accept4.c: (main): Pass SOCK_CLOEXEC instead of
> > O_CLOEXEC | O_BINARY to accept4.
> > ---
> > ChangeLog | 7 +++++++
> > tests/test-accept4.c | 4 ++--
> > 2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/ChangeLog b/ChangeLog
> > index 02d8bf8..3601eda 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -1,3 +1,10 @@
> > +2015-10-08 Pino Toscano <address@hidden>
> > +
> > + Pass only SOCK_* flags to accept4, as they are the only documented
> > + ones, and passing others may trigger EINVAL.
> > + * tests/test-accept4.c: (main): Pass SOCK_CLOEXEC instead of
> > + O_CLOEXEC | O_BINARY to accept4.
> > +
> > 2015-10-06 Pavel Raiskup <address@hidden>
> >
> > gnulib-tool: fix tests of 'extensions' module
> > diff --git a/tests/test-accept4.c b/tests/test-accept4.c
> > index b24af0b..b2e6fa8 100644
> > --- a/tests/test-accept4.c
> > +++ b/tests/test-accept4.c
> > @@ -43,7 +43,7 @@ main (void)
> >
> > errno = 0;
> > ASSERT (accept4 (-1, (struct sockaddr *) &addr, &addrlen,
> > - O_CLOEXEC | O_BINARY)
> > + SOCK_CLOEXEC)
> > == -1);
> > ASSERT (errno == EBADF);
> > }
> > @@ -54,7 +54,7 @@ main (void)
> > close (99);
> > errno = 0;
> > ASSERT (accept4 (99, (struct sockaddr *) &addr, &addrlen,
> > - O_CLOEXEC | O_BINARY)
> > + SOCK_CLOEXEC)
> > == -1);
> > ASSERT (errno == EBADF);
> > }
>
> That change looks good, though it also suggests that
> the implementation doesn't assume the availability of SOCK_CLOEXEC etc.
> I think we also may need the following included in your patch,
> to ensure the test compiles on all platforms, and that those
> constants are defined appropriately on all platforms?
The idea seems ok -- should I merge it with my patch, or can/should it
go as separate patch?
> diff --git a/lib/accept4.c b/lib/accept4.c
> index adf0989..992dfd0 100644
> --- a/lib/accept4.c
> +++ b/lib/accept4.c
> @@ -24,10 +24,6 @@
> #include "binary-io.h"
> #include "msvc-nothrow.h"
>
> -#ifndef SOCK_CLOEXEC
> -# define SOCK_CLOEXEC 0
> -#endif
> -
> int
> accept4 (int sockfd, struct sockaddr *addr, socklen_t *addrlen, int flags)
> {
> diff --git a/lib/sys_socket.in.h b/lib/sys_socket.in.h
> index d29cc09..2d9df45 100644
> --- a/lib/sys_socket.in.h
> +++ b/lib/sys_socket.in.h
> @@ -654,10 +654,16 @@ _GL_WARN_ON_USE (shutdown, "shutdown is not always
> POSIX compliant - "
>
> #if @GNULIB_ACCEPT4@
> /* Accept a connection on a socket, with specific opening flags.
> - The flags are a bitmask, possibly including O_CLOEXEC (defined in
> <fcntl.h>)
> - and O_TEXT, O_BINARY (defined in "binary-io.h").
> + The flags are a bitmask, possibly including SOCK_NONBLOCK,
> + SOCK_CLOEXEC, and O_TEXT, O_BINARY (defined in "binary-io.h").
> See also the Linux man page at
> <http://www.kernel.org/doc/man-pages/online/pages/man2/accept4.2.html>.
> */
> +# ifndef SOCK_CLOEXEC
> +# define SOCK_CLOEXEC O_CLOEXEC
> +# endif
> +# ifndef SOCK_NONBLOCK
> +# define SOCK_NONBLOCK O_NONBLOCK
> +# endif
> # if @HAVE_ACCEPT4@
> # if !(defined __cplusplus && defined GNULIB_NAMESPACE)
> # define accept4 rpl_accept4
SOCK_CLOEXEC is used only in src/accept4.c, so that seems ok.
OTOH, SOCK_NONBLOCK is checked in tests/test-nonblocking.c, where it
would enable the code passing extra flags to the socket type; defining
could make that check failing in case the OS does not implement accept4
(and thus we are providing SOCK_*). What do you think?
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.