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

[David Holmes]

On 12/02/2020 6:31 pm, Baesken, Matthias wrote:
> 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 )

Seems okay.

One nit:

!   int oflag_with_o_cloexec = oflag;
!   oflag_with_o_cloexec |= O_CLOEXEC;

could just be:

!   int oflag_with_o_cloexec = oflag | O_CLOEXEC;

Thanks,
David

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