Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On 12/03/2013 22:19, Dan Xu wrote: I understand now. Here is the updated webrev to directly map IO_Append to handleWrite in *nix platforms, http://cr.openjdk.java.net/~dxu/8001334/webrev.03/. I checked FileOutputStream.java source code, and we do guarantee the consistency of append flag between open and write operations. Thanks! -Dan Thanks for sorting that one out. I've checked that part of the new webrev and it looks good to me now. -Alan.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On 12.03.2013 3:43, Dan Xu wrote: Thanks for all your comments. I have updated the fix accordingly. Please see the webrev at http://cr.openjdk.java.net/~dxu/8001334/webrev.02/. For the language concern in getLastErrorString(char *buf, size_t len) function, I will log another bug and address it later. Thanks! Ok. That is a common place for Windows platform: you got a problem each time you are using ASCII API with Java. I spent some time while resolving that kind of problems in Java deployment. -uta -Dan On Thu 07 Mar 2013 05:28:15 AM PST, Alan Bateman wrote: On 07/03/2013 13:21, Alexey Utkin wrote: Can I say two word about the file http://cr.openjdk.java.net/~dxu/8001334/webrev.01/src/windows/native/java/io/io_util_md.c.frames.html and function getLastErrorString(char *buf, size_t len) Here is the documentation for [FormatMessage]: http://msdn.microsoft.com/en-gb/library/windows/desktop/ms679351%28v=vs.85%29.aspx if you are using ASCII version of FormatMessage that is a good idea to have the direct reference to [FormatMessageA], not define. The second word is about the third world countries: Russia and China. Windows OS has localized version. The error messages on that systems would contains only [?] in the worst case. It's good to re-examine this but just to mention that this is just bringing over what is in os::lasterror (in hotspot/src/os/windows/vm/os_windows.cpp). -Alan
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
Thank you for the review. I will push it today. -Dan On 03/13/2013 03:39 AM, Alan Bateman wrote: On 12/03/2013 22:19, Dan Xu wrote: I understand now. Here is the updated webrev to directly map IO_Append to handleWrite in *nix platforms, http://cr.openjdk.java.net/~dxu/8001334/webrev.03/. I checked FileOutputStream.java source code, and we do guarantee the consistency of append flag between open and write operations. Thanks! -Dan Thanks for sorting that one out. I've checked that part of the new webrev and it looks good to me now. -Alan.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On 03/12/2013 08:19 AM, Alan Bateman wrote: On 11/03/2013 23:43, Dan Xu wrote: Thanks for all your comments. I have updated the fix accordingly. Please see the webrev at http://cr.openjdk.java.net/~dxu/8001334/webrev.02/. For the language concern in getLastErrorString(char *buf, size_t len) function, I will log another bug and address it later. Thanks! -Dan You've addressed all my comments but I think I may have confused you on one point when I mentioned O_APPEND. You've changed handleAppend to use fcntl(F_GETFL) and check if the flag is set but this will happen on every write in append mode and we don't want that. I think you can simply change IO_Append to be handleWrite or else have handleAppend call handleWrite. The jboolean flag isn't needed for the *nix case. -Alan. Hi Alan, Do you mean directly map IO_Append to handleWrite in io_util_md.h for the *nix case? And then where do we check the O_APPEND flag in our code? Or do we require users to open the file with O_APPEND flag? Thanks! -Dan
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On 12/03/2013 18:01, Dan Xu wrote: Hi Alan, Do you mean directly map IO_Append to handleWrite in io_util_md.h for the *nix case? And then where do we check the O_APPEND flag in our code? Or do we require users to open the file with O_APPEND flag? Thanks! Yes, either IO_Append is defined to be handleWrite or else add handleAppend that simply calls handleWrite. There's no need to check O_APPEND after the file is opened for append, not on *nix anyway. -Alan
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
I understand now. Here is the updated webrev to directly map IO_Append to handleWrite in *nix platforms, http://cr.openjdk.java.net/~dxu/8001334/webrev.03/. I checked FileOutputStream.java source code, and we do guarantee the consistency of append flag between open and write operations. Thanks! -Dan On 03/12/2013 02:22 PM, Alan Bateman wrote: On 12/03/2013 18:01, Dan Xu wrote: Hi Alan, Do you mean directly map IO_Append to handleWrite in io_util_md.h for the *nix case? And then where do we check the O_APPEND flag in our code? Or do we require users to open the file with O_APPEND flag? Thanks! Yes, either IO_Append is defined to be handleWrite or else add handleAppend that simply calls handleWrite. There's no need to check O_APPEND after the file is opened for append, not on *nix anyway. -Alan
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
Thanks for all your comments. I have updated the fix accordingly. Please see the webrev at http://cr.openjdk.java.net/~dxu/8001334/webrev.02/. For the language concern in getLastErrorString(char *buf, size_t len) function, I will log another bug and address it later. Thanks! -Dan On Thu 07 Mar 2013 05:28:15 AM PST, Alan Bateman wrote: On 07/03/2013 13:21, Alexey Utkin wrote: Can I say two word about the file http://cr.openjdk.java.net/~dxu/8001334/webrev.01/src/windows/native/java/io/io_util_md.c.frames.html and function getLastErrorString(char *buf, size_t len) Here is the documentation for [FormatMessage]: http://msdn.microsoft.com/en-gb/library/windows/desktop/ms679351%28v=vs.85%29.aspx if you are using ASCII version of FormatMessage that is a good idea to have the direct reference to [FormatMessageA], not define. The second word is about the third world countries: Russia and China. Windows OS has localized version. The error messages on that systems would contains only [?] in the worst case. It's good to re-examine this but just to mention that this is just bringing over what is in os::lasterror (in hotspot/src/os/windows/vm/os_windows.cpp). -Alan
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On 05/03/2013 18:39, Dan Xu wrote: Hi All, Thanks for your good suggestions. I have updated this fix and put the new webrev at http://cr.openjdk.java.net/~dxu/8001334/webrev.01/. Please help review it. Thanks! -Dan I've looked at the latest webrev and it looks quite good. There are several other things that should be done, like the O_CLOEXEC topic that we discussed here, but they can be done later. The main thing is that we've removed the dependency on the JVM_* functions and so finally being the interruptible I/O story to to end. For naming then I probably should chosen something other than handle* for the *nix code but I guess what you have is okay. A few comments on the *nix handleOpen: - it doesn't look like flag is needed as you can pass oflag to open64. - it looks like close could set errno. At least for the EISDIR case you probably should set this after the close. - I assume fstat64 should use RESTARTABLE. A small comment on handleRead/handleWrite is that the return from read/write is normally ssize_t. Something for another day but we would re-examine handleAppend as the file should be open for O_APPEND already. Minor nit in handleAvailable is that the last if-then-else is missing braces around the return 0. Minor nit in the RESTARTABLE macro (io_util_md.c), probably should use 4-space indent. That's all I have. -Alan.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On 07/03/2013 13:21, Alexey Utkin wrote: Can I say two word about the file http://cr.openjdk.java.net/~dxu/8001334/webrev.01/src/windows/native/java/io/io_util_md.c.frames.html and function getLastErrorString(char *buf, size_t len) Here is the documentation for [FormatMessage]: http://msdn.microsoft.com/en-gb/library/windows/desktop/ms679351%28v=vs.85%29.aspx if you are using ASCII version of FormatMessage that is a good idea to have the direct reference to [FormatMessageA], not define. The second word is about the third world countries: Russia and China. Windows OS has localized version. The error messages on that systems would contains only [?] in the worst case. It's good to re-examine this but just to mention that this is just bringing over what is in os::lasterror (in hotspot/src/os/windows/vm/os_windows.cpp). -Alan
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
Can I say two word about the file http://cr.openjdk.java.net/~dxu/8001334/webrev.01/src/windows/native/java/io/io_util_md.c.frames.html and function getLastErrorString(char *buf, size_t len) Here is the documentation for [FormatMessage]: http://msdn.microsoft.com/en-gb/library/windows/desktop/ms679351%28v=vs.85%29.aspx if you are using ASCII version of FormatMessage that is a good idea to have the direct reference to [FormatMessageA], not define. The second word is about the third world countries: Russia and China. Windows OS has localized version. The error messages on that systems would contains only [?] in the worst case. === dwLanguageId [in] The language identifier for the requested message. This parameter is ignored if dwFlags includes FORMAT_MESSAGE_FROM_STRING. If you pass a specific LANGID in this parameter, FormatMessage will return a message for that LANGID only. If the function cannot find a message for that LANGID, it sets Last-Error to ERROR_RESOURCE_LANG_NOT_FOUND. If you pass in zero, FormatMessage looks for a message for LANGIDs in the following order: Language neutral Thread LANGID, based on the thread's locale value User default LANGID, based on the user's default locale value System default LANGID, based on the system default locale value US English - If FormatMessage does not locate a message for any of the preceding LANGIDs, it returns any language message string that is present. If that fails, it returns ERROR_RESOURCE_LANG_NOT_FOUND. === On 07.03.2013 16:40, Alan Bateman wrote: On 05/03/2013 18:39, Dan Xu wrote: Hi All, Thanks for your good suggestions. I have updated this fix and put the new webrev at http://cr.openjdk.java.net/~dxu/8001334/webrev.01/. Please help review it. Thanks! -Dan I've looked at the latest webrev and it looks quite good. There are several other things that should be done, like the O_CLOEXEC topic that we discussed here, but they can be done later. The main thing is that we've removed the dependency on the JVM_* functions and so finally being the interruptible I/O story to to end. For naming then I probably should chosen something other than handle* for the *nix code but I guess what you have is okay. A few comments on the *nix handleOpen: - it doesn't look like flag is needed as you can pass oflag to open64. - it looks like close could set errno. At least for the EISDIR case you probably should set this after the close. - I assume fstat64 should use RESTARTABLE. A small comment on handleRead/handleWrite is that the return from read/write is normally ssize_t. Something for another day but we would re-examine handleAppend as the file should be open for O_APPEND already. Minor nit in handleAvailable is that the last if-then-else is missing braces around the return 0. Minor nit in the RESTARTABLE macro (io_util_md.c), probably should use 4-space indent. That's all I have. -Alan.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
Hi All, Thanks for your good suggestions. I have updated this fix and put the new webrev at http://cr.openjdk.java.net/~dxu/8001334/webrev.01/. Please help review it. Thanks! -Dan On 02/01/2013 01:25 PM, Alan Bateman wrote: On 01/02/2013 18:12, Martin Buchholz wrote: : My comments are all very high level. The history of generic C-level infrastructure in the JDK is unsuccessful. The JVM_ functions were apparently a failure, but who is willing to own the problem of a suitable replacement? Leaving the problem up to individual component teams is a recipe for each component having different interesting bugs using the same functionality. The obvious examples are: all internal file operations in the JDK should be using LARGEFILE variants on 32-bit platforms. And all file descriptor creations should be using O_CLOEXEC by default (much less important). Does anyone own this problem? The JVM/HPI was useful and important (particularly to I/O and networking areas) when we had different threading models. We've required native threading support for a long time now so the need to go through the VM has mostly gone away. Also in recent years we are making using of highly platform specific I/O facilities that aren't intended for porting to other platforms. We don't have a replacement and it's a good discussion point. A porting interface for the libraries would help in some areas, although not all because of the broad set of services and interfaces that are used. Without such an interface then it does mean a little bit of duplication and potential for inconsistencies. Common operations like we are discussing here could be easily supported as utility functions in libjava or elsewhere, we just haven't had any discussion here about this topic. Anyway, on the specifics then we should be using open64 or open with the LARGEFILE flag everywhere. You pointed out issue a few days ago with the launcher parsing the JAR manifest. You should push the patch for that. Also shout/propose patches if you find others. I think the close-on-exec issue does need a bit of thought. We have several places that open files or sockets and they aren't using it so we are dependent on the exec code to close the file descriptors in the child. I haven't come across any bug reports so it's possible that there aren't too many people doing fork/equivalent in native code. I do agree we should look at it, although it less important as you point you. -Alan
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On 01/02/2013 02:23, Martin Buchholz wrote: You could operate in paranoid mode and do *both* : use O_CLOEXEC and use fcntl to set the bit after creating it, perhaps after verifying via fcntl whether the bit was successfully set by open. Martin Alternatively, just leave this code out. We open sockets and files in many other places in the libraries and don't explicitly enable the close-on-exec flag (we do set the inheritance flag on Windows so I guess we do have an inconsistency). Anyway, my point is that the Process code already handles this and closes the file descriptors in the child so I assume it doesn't matter if we have enabled the close-on-exec flag or not. There might of course be applications or other libraries that use fork+exec or variants directly, in which case there will be an issue if they expect all file descriptors to have the close-on-exec flag set. However, I assume we have this issue already as we just enable it consistently. -Alan.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
Yes, the current Process code deals fairly well with other people's file descriptors that are not close-on-exec. But this code is brittle, and long-term it would be cleaner for all open's in the jdk to use O_CLOEXEC by default. We can probably never remove the Process code that closes all file descriptors, because native user code could also create file descriptors. In the near term, until all the posixoid systems we (might) run on support O_CLOEXEC, it seems better to leave this code as is. Or for general cleanliness, add O_CLOEXEC by reflex whenever opening a file, without expectation that users will ever see the benefit, unless they roll their own fork-exec. On Fri, Feb 1, 2013 at 1:51 AM, Alan Bateman alan.bate...@oracle.comwrote: On 01/02/2013 02:23, Martin Buchholz wrote: You could operate in paranoid mode and do *both* : use O_CLOEXEC and use fcntl to set the bit after creating it, perhaps after verifying via fcntl whether the bit was successfully set by open. Martin Alternatively, just leave this code out. We open sockets and files in many other places in the libraries and don't explicitly enable the close-on-exec flag (we do set the inheritance flag on Windows so I guess we do have an inconsistency). Anyway, my point is that the Process code already handles this and closes the file descriptors in the child so I assume it doesn't matter if we have enabled the close-on-exec flag or not. There might of course be applications or other libraries that use fork+exec or variants directly, in which case there will be an issue if they expect all file descriptors to have the close-on-exec flag set. However, I assume we have this issue already as we just enable it consistently. -Alan.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On 01/02/2013 17:45, Martin Buchholz wrote: Yes, the current Process code deals fairly well with other people's file descriptors that are not close-on-exec. But this code is brittle, and long-term it would be cleaner for all open's in the jdk to use O_CLOEXEC by default. We can probably never remove the Process code that closes all file descriptors, because native user code could also create file descriptors. In the near term, until all the posixoid systems we (might) run on support O_CLOEXEC, it seems better to leave this code as is. Or for general cleanliness, add O_CLOEXEC by reflex whenever opening a file, without expectation that users will ever see the benefit, unless they roll their own fork-exec. There are lots of places in the JDK that open files or sockets, java.io is just one. It may be better if we separate this from Dan's clean-up and decide (as part of a separate piece of work) whether we want everywhere to enable close-on-exec on its file descriptors or just leave it to Process.exec as we do now. -Alan
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On Fri, Feb 1, 2013 at 9:51 AM, Alan Bateman alan.bate...@oracle.comwrote: There are lots of places in the JDK that open files or sockets, java.iois just one. It may be better if we separate this from Dan's clean-up and decide (as part of a separate piece of work) whether we want everywhere to enable close-on-exec on its file descriptors or just leave it to Process.exec as we do now. My comments are all very high level. The history of generic C-level infrastructure in the JDK is unsuccessful. The JVM_ functions were apparently a failure, but who is willing to own the problem of a suitable replacement? Leaving the problem up to individual component teams is a recipe for each component having different interesting bugs using the same functionality. The obvious examples are: all internal file operations in the JDK should be using LARGEFILE variants on 32-bit platforms. And all file descriptor creations should be using O_CLOEXEC by default (much less important). Does anyone own this problem?
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
On 01/02/2013 18:12, Martin Buchholz wrote: : My comments are all very high level. The history of generic C-level infrastructure in the JDK is unsuccessful. The JVM_ functions were apparently a failure, but who is willing to own the problem of a suitable replacement? Leaving the problem up to individual component teams is a recipe for each component having different interesting bugs using the same functionality. The obvious examples are: all internal file operations in the JDK should be using LARGEFILE variants on 32-bit platforms. And all file descriptor creations should be using O_CLOEXEC by default (much less important). Does anyone own this problem? The JVM/HPI was useful and important (particularly to I/O and networking areas) when we had different threading models. We've required native threading support for a long time now so the need to go through the VM has mostly gone away. Also in recent years we are making using of highly platform specific I/O facilities that aren't intended for porting to other platforms. We don't have a replacement and it's a good discussion point. A porting interface for the libraries would help in some areas, although not all because of the broad set of services and interfaces that are used. Without such an interface then it does mean a little bit of duplication and potential for inconsistencies. Common operations like we are discussing here could be easily supported as utility functions in libjava or elsewhere, we just haven't had any discussion here about this topic. Anyway, on the specifics then we should be using open64 or open with the LARGEFILE flag everywhere. You pointed out issue a few days ago with the launcher parsing the JAR manifest. You should push the patch for that. Also shout/propose patches if you find others. I think the close-on-exec issue does need a bit of thought. We have several places that open files or sockets and they aren't using it so we are dependent on the exec code to close the file descriptors in the child. I haven't come across any bug reports so it's possible that there aren't too many people doing fork/equivalent in native code. I do agree we should look at it, although it less important as you point you. -Alan
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
Dan,
I had a question on this comment.
Should we fix this in hotspot?
So you mention recent Linux open() documentation.
How does this behave on Solaris and Mac? I assume the library code is shared
code across platforms.
Also - what is the oldest Linux we support for JDK8?
thanks,
Karen
On Jan 29, 2013, at 6:55 AM, Alan Bateman wrote:
- 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.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
Hi Karen,
In my opinion, it is recommemded to use O_CLOEXEC flag directly in
open() function than setting it later via fcntl(). And according to the
man page on Solaris and Mac, open() behaves the same on those platforms.
I only find the support platform list for jdk7 at
http://www.oracle.com/technetwork/java/javase/config-417990.html.
Because Linux 2.6.23 was released long ago on Oct 9, 2007, most
platforms should already have the support to O_CLOEXEC flag.
Thanks,
-Dan
On 01/31/2013 07:30 AM, Karen Kinnear wrote:
Dan,
I had a question on this comment.
Should we fix this in hotspot?
So you mention recent Linux open() documentation.
How does this behave on Solaris and Mac? I assume the library code is
shared code across platforms.
Also - what is the oldest Linux we support for JDK8?
thanks,
Karen
On Jan 29, 2013, at 6:55 AM, Alan Bateman wrote:
- 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.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
You could operate in paranoid mode and do *both* : use O_CLOEXEC and use
fcntl to set the bit after creating it, perhaps after verifying via fcntl
whether the bit was successfully set by open.
Martin
On Thu, Jan 31, 2013 at 12:07 PM, Dan Xu dan...@oracle.com wrote:
Hi Karen,
In my opinion, it is recommemded to use O_CLOEXEC flag directly in open()
function than setting it later via fcntl(). And according to the man page
on Solaris and Mac, open() behaves the same on those platforms.
I only find the support platform list for jdk7 at http://www.oracle.com/**
technetwork/java/javase/**config-417990.htmlhttp://www.oracle.com/technetwork/java/javase/config-417990.html.
Because Linux 2.6.23 was released long ago on Oct 9, 2007, most platforms
should already have the support to O_CLOEXEC flag.
Thanks,
-Dan
On 01/31/2013 07:30 AM, Karen Kinnear wrote:
Dan,
I had a question on this comment.
Should we fix this in hotspot?
So you mention recent Linux open() documentation.
How does this behave on Solaris and Mac? I assume the library code is
shared code across platforms.
Also - what is the oldest Linux we support for JDK8?
thanks,
Karen
On Jan 29, 2013, at 6:55 AM, Alan Bateman wrote:
- 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.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
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.libCreating 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.
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
. 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 tofalse 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
Re: Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
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. - 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. - 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. - 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). - handleOpen - looks like a bug where it should use mode instead of the 0666. - I don't think the O_CLOEXEC code is needed in handleOpen, was this copied from HotSpot? - 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. - 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. - src/solaris/native/java/io/io_util_md.h - minor nit, alignment of RESTARTABLE. - 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. That's all I have. I assume you will run all tests on all platforms before this is pushed. -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 tofalse 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
Review Request: JDK-8001334 - Remove use of JVM_* functions from java.io code
Moving to core-libs-dev to allow for wider review. As Dan mentions, we've been looking to remove the Solairs-specific interruptible I/O for a long time. 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 tofalse 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
