[PATCH] SOCK_CLOEXEC for opening sockets

[Ivan Gerasimov]

Hi Chris!

A couple of minor comments.

1)
if (s != -1 || (s == -1 && errno != EINVAL)) {

This can be simplified to

if (s != -1 || errno != EINVAL) {

2)
What about sockets created with accept():  Shouldn't NET_Accept be 
modified to set O_CLOEXEC as well?
On Linux accept4() can be used to pass SOCK_CLOEXEC flag.

With kind regards,
Ivan

On 7/25/18 5:49 AM, Chris Hegarty wrote:
> Alan,
>
>> On 25 Jul 2018, at 08:24, Alan Bateman <Alan.Bateman at oracle.com> wrote:
>>
>> ...
>>
>> As I said previously, the patch isn't complete so native code calling fork/exec may still have to deal with other file descriptors that are inherited into the child. I don't object to doing this in phases of course but somehow we have managed to get by for 20 years without this being an issue.
> I added the following to the JIRA description to make this clear:
>
> "This JIRA issue, by itself, does not completely resolve the problem
> of native code calling fork/exec, it may still have to deal with other
> file descriptors that are inherited by the child. Instead this JIRA
> issue is targeted at the socket and channels areas only, other areas
> should be tackled on a phased approach though separate JIRA
> issues."
>
>> The updates to the various site to use the NET_* functions are fine. However, I think the new functions in net_util_md.c could be cleaner. I think it would be better to fallback to socket/socketpair + fcntl when the initial call fails with EINVAL.
> Agreed. How about this ( trying to reduce the ifdef blocks, and
> keep them relatively clean ) :
>
> ---
> JNIEXPORT int JNICALL
> NET_Socket(int domain, int type, int protocol) {
>      int s;
> #if defined(SOCK_CLOEXEC)
>      s = socket(domain, type | SOCK_CLOEXEC, protocol);
>      if (s != -1 || (s == -1 && errno != EINVAL)) {
>          return s;
>      }
> #endif
>      s = socket(domain, type, protocol);
>      if (s != -1) {
>          // Best effort - return value is intentionally ignored since the socket
>          // was successfully created.
>          fcntl(s, F_SETFD, FD_CLOEXEC);
>      }
>      return s;
> }
> ---
>
> Updated webrev:
>    http://cr.openjdk.java.net/~chegar/8207335/webrev.01/
>
> -Chris.

-- 
With kind regards,
Ivan Gerasimov