Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
Hi Peter,
Looks like you have a committer status, will you be pushing this?
On 12/18/2014 5:23 AM, Peter Levart wrote:
Hi,
I propose a patch for the following issue:
https://bugs.openjdk.java.net/browse/JDK-8047769
Here's the webrev:
http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.01/
Looks good. A few minor comments.
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.)
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?
Both of these current calls are contained within a
AccessContrller.doPriviledged, the checkRead() seems redundant.
In your test case, if assertions are not enabled, the tests at 46/50/51
are noops, e.g. when run by hand. Generally should directly throw
Exceptions.
The patch uses a package-private FileInputStreamPool class that caches
open FileInputStream(s) so that at most one file descriptor is open for
a particular file. Reading from shared unbuffered FileInputStream in
multiple threads should be safe, right?
I would think (hope?) so, but I am not 100% sure. I tracked it down
into libjava native code:
io_util.c
=
fd = GET_FD(this, fid);
if (fd == -1) {
JNU_ThrowIOException(env, "Stream Closed");
nread = -1;
} else {
nread = IO_Read(fd, buf, len);
which is then defined to handleRead, which calls something called
RESTARTABLE in io_util_md.c. Assuming I'm looking at the right code.
Core-libs folks?
sun/security/provider/PolicyFile/GrantAllPermToExtWhenNoPolicy.java:
Make sure that when no system policy and user policy files exist, the
built-in default policy will be used, which - amongst other things -
grants standard extensions the AllPermission.
sun/security/provider/PolicyParser/ExtDirsChange.java: standard
extensions path is hard-coded in default system policy file
sun/security/provider/PolicyParser/PrincipalExpansionError.java: parser
incorrectly ignores a principal if the principal name expands to nothing.
...they are unrelated to this patch - seems to be caused by recent
layout changes for modular runtime images.
Hopefully you saw my previous response. Repeating:
---begin---
They've been failing for a while:
https://bugs.openjdk.java.net/browse/JDK-8039280
These tests are all "/manual" so they don't show up in our regular runs
of JPRT/jtreg, which include -a.
-a | -automatic | -automagic
Any test with /manual will not be run
---end---
Thanks,
Brad
Re: RFR: 8048603: Additional tests for MAC algorithms
Updated webrev looks good. Thanks, Valerie On 12/22/2014 3:29 AM, Artem Smotrakov wrote: Hi Valerie, I have updated the webrev, please take a look. http://cr.openjdk.java.net/~asmotrak/8048603/webrev.02/ Artem On 12/16/2014 01:38 AM, Valerie Peng wrote: Artem, Here are my comments: 1) Three tests under com/sun/crypto/provider directory do not specify a provider when calling Mac.getInstance(). Since they are under the regression tests directory for SunJCE, perhaps we should specify SunJCE provider to be used. 2) 'ALGORITHMS' seems redundant for sun/security/pkcs11/Mac/MacSameTest.java Thanks, Valerie On 12/11/2014 2:17 AM, Artem Smotrakov wrote: I have addressed a couple of comments from Valerie: - updated tests in test/com/sun/crypto/provider/Mac not to test SunPKCS11 provider - added the following tests for PKCS11 providers test/sun/security/pkcs11/Mac/MacKAT.java test/sun/security/pkcs11/Mac/MacSameTest.java - removed a check that CloneNotSupportedException is thrown on Solaris Please take a look. http://cr.openjdk.java.net/~asmotrak/8048603/webrev.01/ Artem On 11/07/2014 01:07 PM, Artem Smotrakov wrote: Please review a couple of new test cases for MAC algorithms. https://bugs.openjdk.java.net/browse/JDK-8048603 http://cr.openjdk.java.net/~asmotrak/8048603/webrev.00/ Artem
Re: RFR: 8048603: Additional tests for MAC algorithms
Hi Valerie, I have updated the webrev, please take a look. http://cr.openjdk.java.net/~asmotrak/8048603/webrev.02/ Artem On 12/16/2014 01:38 AM, Valerie Peng wrote: Artem, Here are my comments: 1) Three tests under com/sun/crypto/provider directory do not specify a provider when calling Mac.getInstance(). Since they are under the regression tests directory for SunJCE, perhaps we should specify SunJCE provider to be used. 2) 'ALGORITHMS' seems redundant for sun/security/pkcs11/Mac/MacSameTest.java Thanks, Valerie On 12/11/2014 2:17 AM, Artem Smotrakov wrote: I have addressed a couple of comments from Valerie: - updated tests in test/com/sun/crypto/provider/Mac not to test SunPKCS11 provider - added the following tests for PKCS11 providers test/sun/security/pkcs11/Mac/MacKAT.java test/sun/security/pkcs11/Mac/MacSameTest.java - removed a check that CloneNotSupportedException is thrown on Solaris Please take a look. http://cr.openjdk.java.net/~asmotrak/8048603/webrev.01/ Artem On 11/07/2014 01:07 PM, Artem Smotrakov wrote: Please review a couple of new test cases for MAC algorithms. https://bugs.openjdk.java.net/browse/JDK-8048603 http://cr.openjdk.java.net/~asmotrak/8048603/webrev.00/ Artem
