Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Ulf Zibis
Am 15.04.2011 01:44, schrieb David Holmes: Ulf, Ulf Zibis said the following on 04/15/11 04:02: Oops, SystemRoot could be null theoretically. So forget my comment about NUL termination. But anyway, should we allow to have get(SystemRoot) != getEnv(SystemRoot)? Is it correct to defaultly set

Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Alan Bateman
Ulf Zibis wrote: IMHO there should be a big hint in this file: // // This is the Windows implementation of this class! // //

Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Ulf Zibis
Am 15.04.2011 10:50, schrieb Alan Bateman: Ulf Zibis wrote: IMHO there should be a big hint in this file: // // This is the Windows implementation of this class! //

Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Michael McMahon
Michael McMahon wrote: oops. This isn't correct. Sorry. It seems the NameComparator class uses exactly the same algorithm as the CASE_INSENSITIVE_ORDER comparator provided by the String class. So, it probably makes sense to replace NameComparator with the one from String (one less class

Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Alan Bateman
Michael McMahon wrote: On Windows, we are ensuring that SystemRoot will at a minimum always be set. Don't you still have the corner case where Runtime.exec is invoked with an empty array and SystemRoot is not in the environment (launched by an older JDK for example)? In that case we can't

Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Ulf Zibis
Am 15.04.2011 11:54, schrieb Alan Bateman: Michael McMahon wrote: On Windows, we are ensuring that SystemRoot will at a minimum always be set. Don't you still have the corner case where Runtime.exec is invoked with an empty array and SystemRoot is not in the environment (launched by an older

Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Ulf Zibis
Am 15.04.2011 11:45, schrieb Michael McMahon: Ulf, On SystemRoot vs systemroot, I had thought that Windows required the sort to be case-sensitive. But, I double-checked and it doesn't. So we can use SystemRoot. If that would be true, we wouldn't need to sort by CASE_INSENSITIVE_ORDER or

Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Alan Bateman
Ulf Zibis wrote: : Again I ask if we shouldn't disallow to user-define the SystemRoot variable on Windows? Imagine someone has set the variable to C:\WINDOWS, but the correct value is D:\WINDOWS. No, I don't think we should silently change the value of this or other variables. -Alan.

Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Ulf Zibis
Am 15.04.2011 14:15, schrieb Alan Bateman: Ulf Zibis wrote: : Again I ask if we shouldn't disallow to user-define the SystemRoot variable on Windows? Imagine someone has set the variable to C:\WINDOWS, but the correct value is D:\WINDOWS. No, I don't think we should silently change the

Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

2011-04-15 Thread Alan Bateman
Ulf Zibis wrote: I don't think about silently changing, I think about to *disallow to set* it e.g. by the put() method, and instead retrieve it by default from constructor, so it could be always read by the get() method. Ah, you mean the environment returned by ProcessBuilder where the spec

Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Mandy Chung
Fix for 7032589: FileHandler leaking file descriptor of the file lock Webrev at: http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.00/ When a log file is currently being used, tryLock() will fail and it has to close the file channel of the lock file Thanks Mandy

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Daniel D. Daugherty
Thumbs up. Dan On 4/15/2011 11:01 AM, Mandy Chung wrote: Fix for 7032589: FileHandler leaking file descriptor of the file lock Webrev at: http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.00/ When a log file is currently being used, tryLock() will fail and it has to close the file

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Rémi Forax
On 04/15/2011 07:55 PM, Daniel D. Daugherty wrote: Thumbs up. Dan Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). Rémi On 4/15/2011 11:01 AM, Mandy Chung wrote: Fix for 7032589: FileHandler leaking

Review needed for 7037085 : Add hashCode() to Timestamp to address Findbugs warning

2011-04-15 Thread Lance Andersen - Oracle
Hi all, Need a reviewer for the following minor change which adds hasCode() to Timestamp to address a Findbugs warning. Regards Lance hg diff diff -r d9248245a88c src/share/classes/java/sql/Timestamp.java --- a/src/share/classes/java/sql/Timestamp.java Wed Apr 13 11:21:36 2011 -0400 +++

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Mandy Chung
Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). That's a good point. I think it should throw IOException if anything goes wrong with fc.close(). Revised the

Re: Review needed for 7037085 : Add hashCode() to Timestamp to address Findbugs warning

2011-04-15 Thread Lance Andersen - Oracle
Hi Eamonn The javadocs for Timestamp have always specifically called the following blurb out in the class description. Based on some side discussions, it was best to also copy this blurb to the added hashCode method (you will see the text at the top of Timestamp) for additional clarity.

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Daniel D. Daugherty
On 4/15/2011 1:12 PM, Mandy Chung wrote: Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). That's a good point. I think it should throw IOException if

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Mandy Chung
On 04/15/11 13:41, Daniel D. Daugherty wrote: On 4/15/2011 1:12 PM, Mandy Chung wrote: Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). That's a good point.

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Rémi Forax
On 04/15/2011 10:55 PM, Mandy Chung wrote: On 04/15/11 13:41, Daniel D. Daugherty wrote: On 4/15/2011 1:12 PM, Mandy Chung wrote: Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a

hg: jdk7/tl/jdk: 7035115: sun/security/pkcs11/Provider/ConfigShortPath.java compilation failed

2011-04-15 Thread valerie . peng
Changeset: 131ed7967996 Author:valeriep Date: 2011-04-15 15:56 -0700 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/131ed7967996 7035115: sun/security/pkcs11/Provider/ConfigShortPath.java compilation failed Summary: Updated the test to use reflection and skip when SunPKCS11

Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock

2011-04-15 Thread Mandy Chung
On 4/15/11 3:57 PM, Rémi Forax wrote: Hi Mandy, Minor nit, initializing available when declaring it is not necessary because both path (with or without exception) initialize it later. Sure. Will fix that before the push. otherwise, I'm Ok with the patch. Thanks Mandy