Re: [PATCH] accept4 tests: fix specified flags

[Pádraig Brady]

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

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
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
+
 int
 main (void)
 {


thanks,
Pádraig