On 12/10/15 12:18, Pino Toscano wrote: > 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 <[email protected]> >>>>> + >>>>> + 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 <[email protected]> >>>>> >>>>> 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 >> +@item >> +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?
Done at: http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commit;h=f982bc8d thanks, Pádraig.
