[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] accept4 tests: fix specified flags
From: |
Pádraig Brady |
Subject: |
Re: [PATCH] accept4 tests: fix specified flags |
Date: |
Mon, 12 Oct 2015 12:35:25 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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 <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?
Done at:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commit;h=f982bc8d
thanks,
Pádraig.
- Prev by Date:
Re: [PATCH] accept4 tests: fix specified flags
- Next by Date:
[PATCH] binary-io, math, pthread, sys_socket, u64, unistd: port to strict C
- Previous by thread:
Re: [PATCH] accept4 tests: fix specified flags
- Next by thread:
[PATCH] binary-io, math, pthread, sys_socket, u64, unistd: port to strict C
- Index(es):