Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
I'm looking at this version of the webrev. http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.03/ I just assigned 8047769 to you. My username is wetmore, Alan is alanb. On 12/24/2014 3:37 AM, Peter Levart wrote: Looks like you have a committer status, will you be pushing this? I can, yes. As soon as we clear-out the remaining questions, right? Yes. The comments below are minor and shouldn't need another review cycle. I have started a JPRT job for you, I'll run it through "core" target which will give us: jdk_lang, jdk_math, jdk_util, jdk_io, jdk_net, jdk_nio, jdk_security*, jdk_rmi, jdk_text, jdk_time, jdk_other, core_tools. Anything else? I'm off tomorrow, I should have full results Wed. Here are the preliminary results for the jobs that have finished. jdk.testlibrary.Asserts is causing compilation errors, additional comments below: /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:33: error: package jdk.testlibrary does not exist import static jdk.testlibrary.Asserts.*; ^ /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:52: error: cannot find symbol assertEquals(bytes.length, nread, "short read"); ^ symbol: method assertEquals(int,int,String) location: class FileInputStreamPoolTest /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:53: error: cannot find symbol assertTrue(Arrays.equals(readBytes, bytes), ^ symbol: method assertTrue(boolean,String) location: class FileInputStreamPoolTest 3 errors TEST RESULT: Failed. Compilation failed: Compilation failed I'm also getting failures in the following test. I unfortunately have to leave now, so don't have time to look at this. But it did mention "seed" so I'm mentioning it here. TEST: java/lang/invoke/LFCaching/LFGarbageCollectedTest.java ACTION: main -- Failed. Execution failed: `main' threw exception: java.lang.Error: 36 of 39 test cases FAILED! Rerun the test with the same "-Dseed=" option as in the log file! REASON: User specified action: run main/othervm LFGarbageCollectedTest TIME: 213.406 seconds messages: command: main LFGarbageCollectedTest reason: User specified action: run main/othervm LFGarbageCollectedTest elapsed time (seconds): 213.406 STDOUT: -Dseed=6102311124531075592 -DtestLimit=2000 Number of iterations according to -DtestLimit is 153 (1989 cases) Code cache size is 251658240 bytes Non-profiled code cache size is 123058253 bytes Number of iterations limited by code cache size is 84 (1092 cases) Number of iterations limited by non-profiled code cache size is 41 (533 cases) Number of iterations is set to 41 (533 cases) Not enough time to continue execution. Interrupted. STDERR: Iteration 0: Tested LF caching feature with MethodHandles.foldArguments method. java.lang.AssertionError: Error: Lambda form is not garbage collected at LFGarbageCollectedTest.doTest(LFGarbageCollectedTest.java:81) at LambdaFormTestCase$TestRun.doIteration(LambdaFormTestCase.java:139) at LambdaFormTestCase$$Lambda$2/5042013.call(Unknown Source) at jdk.testlibrary.TimeLimitedRunner.call(TimeLimitedRunner.java:71) at LambdaFormTestCase.runTests(LambdaFormTestCase.java:201) at LFGarbageCollectedTest.main(LFGarbageCollectedTest.java:105) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) In a couple places, there are a few lines > 80 chars. (It's a pet peeve of mine, this makes side-by-side reviews difficult without a wide monitor. I realize not everyone shares this view, but they're probably not working on a laptop screen regularly.) I have wrapped the lines to contain them inside the 80 column margin. I and my scrubber/slider thank you. :) Two minor nits? SeedGenerator.java: Lines 507/518 Do you need to close the InputStream when last holder is GC'd, or do we just let the FileInputStream's finalizer take care of that? WeakReference is enqueued when it is cleared, so at that time we have no access to the referent (UncloseableInputStream) any more. We could, in addition, "strongly" reference the underlying FileInputStream in the WeakReference subclass itself, but that would just prolong the life of FileInputStream (possibly forever if no further calls to FileInputStreamPool are made that expunge the references from the queue). So yes, we rely on FileInputStream's finalizer, but I don't see any other way that would be better than that. Makes sense, thanks. NativePRNG and URLSeedGenerator don't have a means of closing underlying resources either, so this is not making things any worse. Yes. Both of these current calls are contained within a AccessContrller.doPriviledged, the checkRead() seems redundant. That's right, but from encapsulation, u
Re: [9] RFR 8039921: SHA1WithDSA with key > 1024 bits not working
Looks fine. Thanks Max > On Dec 30, 2014, at 08:40, Valerie Peng wrote: > > > Can someone please help review this change? The fix is straightforward - > remove the key length check inside the code for DSA related signatures. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8039921 > Webrev: http://cr.openjdk.java.net/~valeriep/8039921/webrev.00/ > > Thanks, > Valerie
[9] RFR 8039921: SHA1WithDSA with key > 1024 bits not working
Can someone please help review this change? The fix is straightforward - remove the key length check inside the code for DSA related signatures. Bug: https://bugs.openjdk.java.net/browse/JDK-8039921 Webrev: http://cr.openjdk.java.net/~valeriep/8039921/webrev.00/ Thanks, Valerie
Re: Please review CR 8049021 Add smartcardio tests with APDU buffer
Ok, will put it back tomorrow. Regards, Valerie On 12/29/2014 2:59 PM, Amanda Jiang wrote: Hi Valerie, Thanks for reviewing this changeset, could you please sponsor this? Regards, Amanda On 12/29/14 1:51 PM, Valerie Peng wrote: Hi, Amanda, Your changes look fine. Thanks, Valerie On 9/9/2014 5:52 PM, Amanda Jiang wrote: Hello, Please help to review the changeset below, new tests are added for javax.smartcardio.CommandAPDU, javax.smartcardio.ResponseAPDU and javax.smartcardio.TerminalFactorySpi. Bug - https://bugs.openjdk.java.net/browse/JDK-8049021 Webrev - http://cr.openjdk.java.net/~tyan/amandaj/8049021/webrev00/ Thanks, Amanda
Re: Please review CR 8049021 Add smartcardio tests with APDU buffer
Hi Valerie, Thanks for reviewing this changeset, could you please sponsor this? Regards, Amanda On 12/29/14 1:51 PM, Valerie Peng wrote: Hi, Amanda, Your changes look fine. Thanks, Valerie On 9/9/2014 5:52 PM, Amanda Jiang wrote: Hello, Please help to review the changeset below, new tests are added for javax.smartcardio.CommandAPDU, javax.smartcardio.ResponseAPDU and javax.smartcardio.TerminalFactorySpi. Bug - https://bugs.openjdk.java.net/browse/JDK-8049021 Webrev - http://cr.openjdk.java.net/~tyan/amandaj/8049021/webrev00/ Thanks, Amanda
Re: Please review CR 8049021 Add smartcardio tests with APDU buffer
Hi, Amanda, Your changes look fine. Thanks, Valerie On 9/9/2014 5:52 PM, Amanda Jiang wrote: Hello, Please help to review the changeset below, new tests are added for javax.smartcardio.CommandAPDU, javax.smartcardio.ResponseAPDU and javax.smartcardio.TerminalFactorySpi. Bug - https://bugs.openjdk.java.net/browse/JDK-8049021 Webrev - http://cr.openjdk.java.net/~tyan/amandaj/8049021/webrev00/ Thanks, Amanda
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
On 29/12/2014 09:45, Peter Levart wrote: Thanks for looking at this, Alan. You're right about File.getCanonicalFile(). It already checks read permission for a file. The additional explicit check is superfluous. I have removed it. With explicit check I wanted the API to behave uniformly regardless of whether the returned stream is opened by getInputStream() call or an already opened stream is just returned. getCannonicalFile() already takes care of it. Here's the updated webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.03/ Updated patch looks good to me. -Alan.
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
On 12/29/2014 10:08 AM, Alan Bateman wrote: On 24/12/2014 11:37, Peter Levart wrote: Hi Brad, Thanks for looking into this. Here's updated webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.02/ This mostly looks good to me too, except the permission checking. As I read it, getInputStream uses getCanonicalFile and thus the permission check will be happen early and so it makes me wonder if checkRead is needed. Additionally, both of the uses are in privileged blocks so it looks like checkRead will always be called with a stack that has AllPermission anyway. -Alan. Thanks for looking at this, Alan. You're right about File.getCanonicalFile(). It already checks read permission for a file. The additional explicit check is superfluous. I have removed it. With explicit check I wanted the API to behave uniformly regardless of whether the returned stream is opened by getInputStream() call or an already opened stream is just returned. getCannonicalFile() already takes care of it. Here's the updated webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.03/ Regards, Peter
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
On 24/12/2014 11:37, Peter Levart wrote: Hi Brad, Thanks for looking into this. Here's updated webrev: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.02/ This mostly looks good to me too, except the permission checking. As I read it, getInputStream uses getCanonicalFile and thus the permission check will be happen early and so it makes me wonder if checkRead is needed. Additionally, both of the uses are in privileged blocks so it looks like checkRead will always be called with a stack that has AllPermission anyway. -Alan.
