RFR: 8237830: support O_CLOEXEC in os::open on other OS than Linux

[Thomas Stüfe]

Hi Matthias:

+ // Validate that the use of the O_CLOEXEC flag on open above worked.

+  static sig_atomic_t O_CLOEXEC_is_known_to_work = 0;+  if
(!O_CLOEXEC_is_known_to_work) {
     int flags = ::fcntl(fd, F_GETFD);-    if (flags != -1)+    if
(flags != -1) {+      if ((flags & FD_CLOEXEC) != 0) {+
O_CLOEXEC_is_known_to_work = 1;+      } else {
       ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
   }-#endif+    }+  }


This is either too complicated or not complicated enough :)

if we know for a fact that open(..O_CLOEXEC) did work at least once, we
never again check.
if we know for a fact that open(..O_CLOEXEC) failed to work, we recheck
again and again. Do we expect it to suddenly start working? If yes, could
it also suddenly start failing? Should we then not check always?

I propose the following change:

1) If open(..O_CLOEXEC) could sometimes fail and sometimes not, or we are
just not sure, I would do the fcntl test every time. Or, even just always
set the flag again with fcntl. So, just remove the
O_CLOEXEC_is_known_to_work conditional variable.

Which would reduce the code to a one liner:

+ ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);

2) If you are sure that it either always works or fails, you need a
conditional with three states: unknown, works, does not work. And only
check if unknown.

I personally would prefer (1) because it is way simpler and fcntl is not
such an expensive operation that we have to save time (also, how often do
we open files anyway?).

----

Otherwise looks fine. This is a useful fix.


Cheers, Thomas





On Tue, Jan 28, 2020 at 12:32 PM Baesken, Matthias <matthias.baesken at sap.com>
wrote:

> New webrev :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8237830.1/
>
> (one 'c' got lost somehow in the older webrev)
>
> Best regards, Matthias
>
>
> >
> > Hi David , on AIX  I see  O_CLOEXEC  since   AIX 7.1.  Seems it was not
> > supported in 6.1, at least I cannot find it there .
> >
> > Regarding macOS,   this thread
> >
> > https://groups.google.com/forum/#!topic/golang-dev/7mmT7o9GYb4
> >
> > claims it is there since  10.7 .
> > Maybe someone can provide more detailed information ?
> >
> > Best regards, Matthias
> >
> >
> >
>
>