Hi Alan,
On 01/25/2013 06:53 AM, Alan Bateman wrote:
Dan,
I've taken a pass over this and overall this is good clean-up and
something that was just not possible before jdk8 because of the need
to keep -XX:+UseVMInterruptibleIO working on Solaris.
A few comments:
- I don't understand why these changes require the make file changes
to link io_util_md and canonicalize_md into nio.dll.
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.
- In the definition of ISNANF on Solaris it uses isnand. I'll bet the
comment in globalDefinitions_xxx dates back to the original port and
might not be true anymore (I'm not suggesting you change it without
intensive testing, just a comment that I'll bet it is not an issue now).
- "FD" is probably okay, I just wonder about how easy it might be to
get a conflict.
FD is defined to jlong on windows and to jint on other platforms. The
way of using FD is the same as the way of defining IO_Read, IO_write,
and other similar functions. I think the conflict is minimum. This makes
the code easier to maintain and understand.
- 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.
- 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?
- handleOpen - looks like a bug where it should use "mode" instead of
the 0666.
You are right. I will fix it.
- 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.
- 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?
- getLastErrorString => I wonder if we should move to strerror_r (less
ambiguity as to whether it is thread-safe). Probably best to use
strncpy too.
I will change to use strerror_r.
- src/solaris/native/java/io/io_util_md.h - minor nit, alignment of
RESTARTABLE.
I will fix it.
- 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.
That's all I have. I assume you will run all tests on all platforms
before this is pushed.\
Sure, I will run it.
Thanks,
-Dan
-Alan
:
-------- Original Message --------
Subject: Review Request: JDK-8001334 - Remove use of JVM_*
functions from java.io code
Date: Mon, 21 Jan 2013 23:25:25 -0800
From: Dan Xu <dan...@oracle.com>
To: nio-dev <nio-...@openjdk.java.net>
Hi,
Interruptible I/O on Solaris has been highly problematicand undercut
portability. And the long term plan is to remove it completely from
JDK. In JDK6, the VM option, UseVMInterruptibleIO, was introduced as
part of a phase-out plan to allow developers/customers to disable it.
In JDK7, the default value of UseVMInterruptibleIO flag was changed
to"false" to make Solaris blockingI/O operations uninterruptible by
Java thread interruptsby default.
The last stepof this phase-out plan is to remove the feature
completely. And it is good to do it now in JDK8. In this fix, I
removed all related codes in IO area by replacing JVM_* functions
with direct system calls. Please help review the proposed fix.
Similar changes in networking area will be done via a different bug.
Bug: https://jbs.oracle.com/bugs/browse/JDK-8001334
webrev: http://cr.openjdk.java.net/~dxu/8001334/webrev.00/
<http://cr.openjdk.java.net/%7Edxu/8001334/webrev.00/>
Best,
-Dan
