Re: RFR: JDK-8028726 - (prefs) Check src/solaris/native/java/util/FileSystemPreferences.c for JNI pending exceptions
Thank you, Alan. I will add this change into my fix and push it today. Thanks! -Dan On 01/08/2014 01:24 AM, Alan Bateman wrote: On 08/01/2014 00:50, Dan Xu wrote: Hi All, Thanks for your good review. I have dropped the change in FileSystemPreferences.java , and created the new webrev which only changes FileSystemPreferences.c. Please help review it. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8028726/webrev.01/ When changing FileSystemPreferences.c, I noticed the code like JNU_GetStringPlatformChars(env, java_fname, JNI_FALSE). Because JNI_FALSE is passed into this function, I am wondering why our code still release string by calling JNU_ReleaseStringPlatformChars(env, java_fname, fname). Thanks! This is the isCopy parameter to allow the function return whether it has returned a copy or not. So the JNU_ReleaseStringPlatformChars is needed. However the JNI_FALSE isn't right, it does happens to work because JNI_FALSE is defined as 0, same as NULL. Do you mind changing these to NULL as part of the change so that it's a bit more obvious? (no need to publish a new webrev if there are no other changes). Otherwise looks fine to me. -Alan.
Re: RFR: JDK-8028726 - (prefs) Check src/solaris/native/java/util/FileSystemPreferences.c for JNI pending exceptions
Hi Alan, Btw, I am wondering whether I need pass a variable to see if the returned char* is a copy or not, and basing on that to do the release. For example, jboolean isCopy; const char *fname = JNU_GetStringPlatformChars(env, java_fname, isCopy); if (isCopy == JNI_TRUE) JNU_ReleaseStringPlatformChars(env, java_fname, fname); But according to the definition of JNU_GetStringPlatformChars(), it seems always to set *isCopy to JNI_TRUE currently. Because nativeGetStringPlatformChars() does nothing now. Thanks, -Dan On Wed 08 Jan 2014 09:56:40 AM PST, Dan Xu wrote: Thank you, Alan. I will add this change into my fix and push it today. Thanks! -Dan On 01/08/2014 01:24 AM, Alan Bateman wrote: On 08/01/2014 00:50, Dan Xu wrote: Hi All, Thanks for your good review. I have dropped the change in FileSystemPreferences.java , and created the new webrev which only changes FileSystemPreferences.c. Please help review it. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8028726/webrev.01/ When changing FileSystemPreferences.c, I noticed the code like JNU_GetStringPlatformChars(env, java_fname, JNI_FALSE). Because JNI_FALSE is passed into this function, I am wondering why our code still release string by calling JNU_ReleaseStringPlatformChars(env, java_fname, fname). Thanks! This is the isCopy parameter to allow the function return whether it has returned a copy or not. So the JNU_ReleaseStringPlatformChars is needed. However the JNI_FALSE isn't right, it does happens to work because JNI_FALSE is defined as 0, same as NULL. Do you mind changing these to NULL as part of the change so that it's a bit more obvious? (no need to publish a new webrev if there are no other changes). Otherwise looks fine to me. -Alan.
Re: RFR: JDK-8028726 - (prefs) Check src/solaris/native/java/util/FileSystemPreferences.c for JNI pending exceptions
On 08/01/2014 18:48, Dan Xu wrote: Hi Alan, Btw, I am wondering whether I need pass a variable to see if the returned char* is a copy or not, and basing on that to do the release. For example, jboolean isCopy; const char *fname = JNU_GetStringPlatformChars(env, java_fname, isCopy); if (isCopy == JNI_TRUE) JNU_ReleaseStringPlatformChars(env, java_fname, fname); But according to the definition of JNU_GetStringPlatformChars(), it seems always to set *isCopy to JNI_TRUE currently. Because nativeGetStringPlatformChars() does nothing now. As things stand then JNU_GetStringPlatformChars always returned a copy. If it were changed to conditionally return a copy then every usage would need to specify isCopy and handle as you suggest. So it doesn't matter if you specify NULL or isCopy, both will work here. -Alan
Re: RFR: JDK-8028726 - (prefs) Check src/solaris/native/java/util/FileSystemPreferences.c for JNI pending exceptions
On 6 Jan 2014, at 22:29, Dan Xu dan...@oracle.com wrote:
Hi All,
Please review the simple fix for JNI pending exceptions in
FileSystemPreferences.c. Thanks!
Bug: https://bugs.openjdk.java.net/browse/JDK-8028726
Webrev: http://cr.openjdk.java.net/~dxu/8028726/webrev/
Looks good to me Dan.
Trivially, I don’t think you need the if (result != null) {“. If the native
method “fails” and returns NULL, there will be a pending exception which will
be thrown automatically when transitioning back to Java-land. But, what you
have is arguably more robust., so thumbs up from me.
-Chris.
-Dan
Re: RFR: JDK-8028726 - (prefs) Check src/solaris/native/java/util/FileSystemPreferences.c for JNI pending exceptions
On 06/01/2014 22:29, Dan Xu wrote: Hi All, Please review the simple fix for JNI pending exceptions in FileSystemPreferences.c. Thanks! Bug: https://bugs.openjdk.java.net/browse/JDK-8028726 Webrev: http://cr.openjdk.java.net/~dxu/8028726/webrev/ The update to FIleSystemPreferences.c looks okay but if I read it correctly then lockFile0 can never return NULL without a pending exception (meaning that the change to FileSystemPreferences.java could mask an underlying bug if it existed). One passing comment is that this native methods could be completely eliminated here by changing FileSystemPreferences to lock the file via a FileChannel (use a FileLock as the lock handle). Also the chmod usage can be eliminated by mkdirs with Files.createDirectory and specify the permission files when creating the directory. I realize this is beyond the scope of what you are doing here (but an opportunity none the less). -Alan
Re: RFR: JDK-8028726 - (prefs) Check src/solaris/native/java/util/FileSystemPreferences.c for JNI pending exceptions
Hi All, Thanks for your good review. I have dropped the change in FileSystemPreferences.java , and created the new webrev which only changes FileSystemPreferences.c. Please help review it. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8028726/webrev.01/ When changing FileSystemPreferences.c, I noticed the code like JNU_GetStringPlatformChars(env, java_fname, JNI_FALSE). Because JNI_FALSE is passed into this function, I am wondering why our code still release string by calling JNU_ReleaseStringPlatformChars(env, java_fname, fname). Thanks! -Dan On 01/07/2014 01:36 AM, Alan Bateman wrote: On 06/01/2014 22:29, Dan Xu wrote: Hi All, Please review the simple fix for JNI pending exceptions in FileSystemPreferences.c. Thanks! Bug: https://bugs.openjdk.java.net/browse/JDK-8028726 Webrev: http://cr.openjdk.java.net/~dxu/8028726/webrev/ The update to FIleSystemPreferences.c looks okay but if I read it correctly then lockFile0 can never return NULL without a pending exception (meaning that the change to FileSystemPreferences.java could mask an underlying bug if it existed). One passing comment is that this native methods could be completely eliminated here by changing FileSystemPreferences to lock the file via a FileChannel (use a FileLock as the lock handle). Also the chmod usage can be eliminated by mkdirs with Files.createDirectory and specify the permission files when creating the directory. I realize this is beyond the scope of what you are doing here (but an opportunity none the less). -Alan
RFR: JDK-8028726 - (prefs) Check src/solaris/native/java/util/FileSystemPreferences.c for JNI pending exceptions
Hi All, Please review the simple fix for JNI pending exceptions in FileSystemPreferences.c. Thanks! Bug: https://bugs.openjdk.java.net/browse/JDK-8028726 Webrev: http://cr.openjdk.java.net/~dxu/8028726/webrev/ -Dan
Re: RFR: JDK-8028726 - (prefs) Check src/solaris/native/java/util/FileSystemPreferences.c for JNI pending exceptions
Dan, Looks OK, but line 914 which you did not change, notice the comments not sure if that is common in this code but seemed a bit off to me: 914 //// If at first, you don't succeed... On Jan 6, 2014, at 5:29 PM, Dan Xu wrote: Hi All, Please review the simple fix for JNI pending exceptions in FileSystemPreferences.c. Thanks! Bug: https://bugs.openjdk.java.net/browse/JDK-8028726 Webrev: http://cr.openjdk.java.net/~dxu/8028726/webrev/ -Dan Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: RFR: JDK-8028726 - (prefs) Check src/solaris/native/java/util/FileSystemPreferences.c for JNI pending exceptions
Hi Lance, Thanks for your review. My understanding towards the for loop in lockFile() of FileSystemPreferences.java is that it retries if anything fails. So I don't touch L914 to keep its old logic un-changed. Please let me know if you have some good suggestions. Thanks! -Dan On 01/06/2014 02:35 PM, Lance Andersen - Oracle wrote: Dan, Looks OK, but line 914 which you did not change, notice the comments not sure if that is common in this code but seemed a bit off to me: 914 //// If at first, you don't succeed... On Jan 6, 2014, at 5:29 PM, Dan Xu wrote: Hi All, Please review the simple fix for JNI pending exceptions in FileSystemPreferences.c. Thanks! Bug: https://bugs.openjdk.java.net/browse/JDK-8028726 Webrev: http://cr.openjdk.java.net/~dxu/8028726/webrev/ -Dan Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
