RFR: 8176717: GC log file handle leaked to child processes

[Leo Korinth]

Hi Thomas,

On 30/03/18 13:16, Thomas Stüfe wrote:
> Hi Leo,
> 
> looks okay. I stated my reservations against adding this function and 
> the platform specific code in its current form before, but I can 
> certainly live with this fix. It follows what we did in os::open(), so 
> it is consistent in that matter.

Thanks for (what I understand is) a review approval. I am still open for 
a different solution if you have one you feel for. Below are some 
alternatives that we have talked about (or I have thought about).

Dividing the fix to os_windows/os_posix will not get rid of the #if and 
splitting it between aix/bsd/linux/solaris/windows will make the 
solution much bigger, redundant and arguably harder to maintain.

I could possibly change it to have a variable or function in each 
architecture that holds only the mode string for fopen (or NULL for no 
support) and in that way change the "#if" to an "if" and remove the 
LINUX_ONLY("e") BSD_ONLY("e") WINDOWS_ONLY("N"). I dislike it because it 
spreads the solution to more files, but at least it reduces the use of 
the preprocessor.

I could try to get the mode flag through autoconf, but that seems _very_ 
hard -- does success of fopen really mean that the mode is supported or 
merely ignored? I believe it to be undefined by the c standard :-(, It 
seems even harder to do an autoconf runtime test for the feature.

I could leave it to be fixed (only) by core-libs, but that I feel is 
wrong for several reasons:
- We have a corresponding fix in os::open that says that we should 
auto-close on exec from hotspot to be sure. And I agree on that.
- core-libs might not be able to fix it for legacy reasons
- it does not protect from when JNI creates new processes (I guess it is 
allowed)

I could make the change log-specific, but that makes little sense as if 
I ought to do it in logging, I ought to do it on every fopen.

I was and still am hoping for more opinions to get a better solution or 
at least a consensus on this solution. If I get none, I will continue 
with the one I propose.

In the end, I will need a sponsor to eventually push the change.

/Leo

> Thanks for fixing this!
> 
> Thomas
> 
> (btw, I am not sure which of my "2" Per meant, since I accidentally 
> miscounted my points in the earlier mail.)
> 
> 
> 
> 
> 
> On Thu, Mar 29, 2018 at 5:41 PM, Leo Korinth <leo.korinth at oracle.com
> <mailto:leo.korinth at oracle.com>> wrote:
> 
>     On 23/03/18 20:03, Leo Korinth wrote:
> 
>         Hi!
> 
>         Cross-posting this to both hotspot-dev and hotspot-runtime-dev
>         to get more attention. Sorry.
> 
>         Original mail conversation can be found here:
>         http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-March/030637.html
>         <http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-March/030637.html>
> 
>         I need feedback to know how to continue.
> 
>         Thanks,
>         Leo
> 
> 
>     Hi!
> 
>     Below is a new webrev with Thomas' and Per's suggested name change
>     from: os::fopen_retain(const char* path, const char* mode)
>     to:   os::fopen(const char* path, const char* mode)
> 
>     Full webrev:
>     http://cr.openjdk.java.net/~lkorinth/8176717/02/
>     <http://cr.openjdk.java.net/~lkorinth/8176717/02/>
> 
>     Review and/or comments please!
> 
>     Thanks,
>     Leo
> 
>