Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code

[Alan Bateman]

On 28/01/2013 19:02, Dan Xu wrote:
> These two .obj are needed during the link process in windows platform. 
> Because getLastErrorString functions, used in io_util.c, are inside 
> io_util_md.obj. And after adding io_util_md.obj, it also introduces 
> another dependency on getPrefixed function which is implemented in 
> canonicalize_md.obj.
>
> Here is the first exception I got if I keep make files untouched.
>
>     The compilation passed. But the link afterwards failed,
>     c:/PROGRA~2/MICROS~2.0/VC/Bin/AMD64/link -dll
>     -out:C:/jprt/T/P1/183441.dan/s/build/windows-amd64/tmp/java/java.nio/nio/obj64/nio.dll
>     \      
>     "-map:C:/jprt/T/P1/183441.dan/s/build/windows-amd64/tmp/java/java.nio/nio/obj64/nio.map"
>     \       -nologo -opt:REF -incremental:no -debug
>     -LIBPATH:C:/DXSDK/Lib/x64 
>     @C:/jprt/T/P1/183441.dan/s/build/windows-amd64/tmp/java/java.nio/nio/obj64/nio.lcf
>     \        C:/jprt/T/P1/183441.dan/s/build/windows-amd64/lib/jvm.lib
>     ws2_32.lib
>     -libpath:C:/jprt/T/P1/183441.dan/s/build/windows-amd64/lib
>     java.lib
>     C:/jprt/T/P1/183441.dan/s/build/windows-amd64/tmp/java/java.nio/nio/obj64/../../../../sun/java.net/net/obj64/net.lib
>     C:/jprt/T/P1/183441.dan/s/build/windows-amd64/tmp/java/java.nio/nio/obj64/../../../java.lang/java/obj64/io_util.obj
>     C:/jprt/T/P1/183441.dan/s/build/windows-amd64/tmp/java/java.nio/nio/obj64/../../../java.lang/java/obj64/FileDescriptor_md.obj  
>     advapi32.lib    Creating library
>     C:/jprt/T/P1/183441.dan/s/build/windows-amd64/tmp/java/java.nio/nio/obj64/nio.lib
>     and object
>     C:/jprt/T/P1/183441.dan/s/build/windows-amd64/tmp/java/java.nio/nio/obj64/nio.exp
>     io_util.obj : error LNK2019: unresolved external symbol
>     getLastErrorString referenced in function throwFileNotFoundException
>
>
>
> I wonder whether I can avoid these link dependencies if I move those 
> functions into other files. But I found it might cause other issues. 
> Here is the comment for getPrefixed() function.
>
>     The appropriate location of getPrefixed() should be io_util_md.c,
>     but java.lang.instrument package has hardwired canonicalize_md.c
>     into their dll, to avoid complicate solution such as including
>     io_util_md.c into that package, as a workaround we put this method
>     here.
>
I think the link failure you are seeing is because io_util.obj and 
FileDescriptor_md.obj are also being linked into nio.dll, they shouldn't 
be but perhaps someone added them a long time ago to workaround another 
issue.

I checked the build of the JPLIS agent (and instrument.dll specifically) 
and it just links canonicalize_md into that DLL. That is a bit messy, 
and I remember some of the history as to why this was done this way, but 
I don't think it should impact anything here.


> :
>> - src/solaris/native/java/io/UnixFileSystem_md.c, did you test access 
>> to "/", I just wonder if it would be better to keep the existing test.
> The current change works the same as the old logic. The old one assign 
> JVM_EEXIST to fd if it is root directory, and bypass JVM_EEXIST 
> error.In the current logic, it also only handles the condition if it 
> is not the root directory.
Okay, it would good to check if we have any tests because file 
operations on "/" can have subtle difference to other directories, at 
least on Solaris.


>> - handleClose in src/solaris/native/java/io/io_util_md.c, close is 
>> not restartable (I think we have this wrong in a few places).
>>
> I see one of errors that close() may encounter is EINTR in its man 
> page, which means it can be interruptible, right?
It can be interrupted but the issue is that the file descriptor is 
invalid at that point so we can't call close with it again.


>
>>
>> - I don't think the O_CLOEXEC code is needed in handleOpen, was this 
>> copied from HotSpot?
> In the original hotspot code, it has one section to enable the 
> close-on-exec flag for the new file descriptor as the following.
>
>             #ifdef FD_CLOEXEC
>                 {
>                     int flags = ::fcntl(fd, F_GETFD);
>                     if (flags != -1)
>                         ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
>                 }
>             #endif
>
> According to the comment, "All file descriptors that are opened in the 
> JVM and not specifically destined for a subprocess should have the 
> close-on-exec flag set.  If we don't set it, then careless 3rd party 
> native code might fork and exec without closing all appropriate file 
> descriptors (e.g. as we do in closeDescriptors in UNIXProcess.c), and 
> this in turn might:
>      - cause end-of-file to fail to be detected on some file 
> descriptors, resulting in mysterious hangs, or
>      - might cause an fopen in the subprocess to fail on a system 
> suffering from bug 1085341."
>
> And the recent open() function (since Linux 2.6.23) already has 
> O_CLOSEXEC flag to enable this flag by default. And it is a preferred 
> way according to the man page, " Specifying this  flag  permits  a 
> program  to  avoid  additional fcntl(2) F_SETFD operations to set the 
> FD_CLOEXEC flag.  Addi-tionally, use of this flag is essential in some 
> multithreaded programs since using a separate fcntl(2)  F_SETFD  
> operation to set the FD_CLOEXEC flag does not suffice to avoid race 
> conditions where one thread opens a file descriptor at the same time  
> as  another  thread  does  a fork(2) plus execve(2).
> Additionally, if O_CLOEXEC is not supported, F_DUPFD_CLOEXEC is 
> preferred than FD_CLOEXEC, which is what hotspot is using right now.
I don't think we need this because the file descriptor will be closed at 
exec time anyway (there is logic in Runtime.exec to iterate over the 
file descriptors and close them). Also we don't do this in other areas 
of the platform where we use open directly.

>
>>
>> - handleAvailable - assume the file is changing, in which case this 
>> could return a negative available. I see the existing code lseeks to 
>> the end whereas you are using the size from the stat, I think that is 
>> okay, just need to properly handle the case that the file is 
>> truncated between calls.
> I have the code to lseek to the end if size < 0. Should I change it to 
> size < current?
I think it should be (size > current) ? (size - current) : 0. 
Alternatively MAX(size-current, 0).



>>
>> - src/windows/native/java/io/io_util_md.h - it's not obvious to me 
>> why these need JNIEXPORT but this might be tied into my first 
>> question about the make changes.
> Actually, I don't know why the old code needs JNIEXPORT, neither. I 
> only change it to use FD. And I did not touch other parts. And for 
> those new method declarations I added to 
> solaris/native/java/io/io_util_md.h, I did not add JNIEXPORT. I will 
> check whether it can solve the link issue if I remove JNIEXPORT from 
> the method signature.
Thanks, I suspect that some/most of these are not needed.

-Alan.