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 <ptosc...@redhat.com>
>>> +
>>> + 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 <prais...@redhat.com>
>>>
>>> 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).
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
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
+
int
main (void)
{
thanks,
Pádraig
