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

[Baesken, Matthias]

New webrev  :


http://cr.openjdk.java.net/~mbaesken/webrevs/8237830.3/

   * this time ,  only touching  os_aix.cpp  ( I leave os_bsd.cpp as it is , might

     make the discussion   more  simple )

   * Added handling of  failing O_CLOEXEC   ( O_CLOEXEC_is_known_to_work

     == -1  … case )

 Best regards, Matthias




From: Baesken, Matthias
Sent: Donnerstag, 6. Februar 2020 08:55
To: 'Thomas Stüfe' <thomas.stuefe at gmail.com>
Cc: David Holmes <david.holmes at oracle.com>; hotspot-dev at openjdk.java.net
Subject: RE: RFR: 8237830: support O_CLOEXEC in os::open on other OS than Linux

Hi Thomas , the coding was borrowed from linux, see :

https://hg.openjdk.java.net/jdk/jdk/file/7b57401deb0c/src/hotspot/os/linux/os_linux.cpp#l5616

I would be fine with  always calling fcntl (your suggestion 1) ,  do you think  it was done otherwise  back then   in os_linux because of performance issues ?
Probably os::open in os_linux  should be adjusted then  to the  new “always call fcntl” approach too, right ?


Best regards, Matthias



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<mailto: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