[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: |
Mon, 12 Oct 2015 13:18:30 +0200 |
User-agent: |
KMail/4.14.9 (Linux/4.1.6-100.fc21.x86_64; KDE/4.14.9; x86_64; ; ) |
On Friday 09 October 2015 15:41:47 Pádraig Brady wrote:
> On 09/10/15 14:38, Pino Toscano wrote:
> > 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?
>
> Yes good point.
>
> accept4() makes no sense without SOCK_CLOEXEC or SOCK_NONBLOCK,
> so my change to define make sense in that respect.
> Defining them to non zero will ensure for example EINVAL is
> returned for SOCK_NONBLOCK on platforms where we replace accept4()
> (though I suppose we could simulate that also).
True: if gnulib is providing accept4 to some platform which does not
provide it, then SOCK_CLOEXEC and SOCK_NONBLOCK ought to be provided
too, as they are part of the interface.
> However SOCK_CLOEXEC and SOCK_NONBLOCK may not be defined
> on platforms with select(), and we probably shouldn't define
> as user space may be doing:
>
> #ifdef SOCK_CLOEXEC
> socket(..., SOCK_CLOEXEC);
> #else
> socket();
> fcntl(..., FD_CLOEXEC);
> #endif
One workaround to that, albeit local to gnulib, would be to define
something like GNULIB_DEFINED_SOCK_NONBLOCK, and adapt
tests/test-nonblocking.c to
# if defined(SOCK_NONBLOCK) && !defined(GNULIB_DEFINED_SOCK_NONBLOCK)
or, even better, add some configure check to test whether
socket(.., .. | SOCK_NONBLOCK) works.
However, the above would not work with 3rd party code doing that check;
one could think that such code may be broken already, as the underlying
kernel could not support those extra flags for socket.
> So let's forget my adjustment and instead I suggest
> merging this into your change:
>
> diff --git a/doc/glibc-functions/accept4.texi
> b/doc/glibc-functions/accept4.texi
> index 20386e9..b4114db 100644
> --- a/doc/glibc-functions/accept4.texi
> +++ b/doc/glibc-functions/accept4.texi
> @@ -16,4 +16,7 @@ programs that spawn child processes.
>
> Portability problems not fixed by Gnulib:
> @itemize
> address@hidden
> +SOCK_CLOEXEC and SOCK_NONBLOCK may not be defined
> +as they're also significant to the socket() function.
> @end itemize
> diff --git a/tests/test-accept4.c b/tests/test-accept4.c
> index b24af0b..5a29680 100644
> --- a/tests/test-accept4.c
> +++ b/tests/test-accept4.c
> @@ -31,6 +31,10 @@ SIGNATURE_CHECK (accept4, int, (int, struct sockaddr *,
> socklen_t *, int));
>
> #include "macros.h"
>
> +#ifndef SOCK_CLOEXEC
> +# define SOCK_CLOEXEC 0
> +#endif
> +
I was not unsure about this, but then I noticed we do the same also in
lib/accept4.c itself, so it will not make things worse (platforms
without accept4 and SOCK_* flags already have an accept4 implementation
which basically works like socket).
Should I merge the above bit in my patch? What about authorship of it?
Thanks,
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.