Update webrev looks fine, just one nit:
ProviderList.java, line 671: The current output seems to suggest that
there is a match with the "==" between the two entries. Perhaps
something likedebug.println("Config match " + toString() + ": " + t + ",
" + a); Since the toString() is already reported on line 671, line 683
doesn't need to repeat this info?
Thanks,
Valerie
On 10/16/2015 10:53 AM, Anthony Scarpino wrote:
On 10/15/2015 05:20 PM, Valerie Peng wrote:
Hi Tony,
Here are the comments for the remaining file.
<ProviderList.java>
- line 621, is there a particular reason to return immediately when
s.ids is null? If there is, it should be commented here. The
get(ServiceList s) method will fall back to s.type and s.algorithm when
s.ids is null. Would be nice to state the reason for the difference in
implementation.
I don't think there is a reason. I suspect it was an odd assumption
that testing didn't reveal as a problem.. I'll change it to check type
and algorithm if ids is null
Going through the code I noticed the get(ServiceList) and get(type,
algorithm) methods are not used, so I am removing them. They were
there from older code that now uses getAll(...).
- line 698, the method name "compare" seems a little misleading, it only
compares type and algorithm and it returns boolean instead of integer as
the compare in other classes. Maybe names like "lookup" or "match" would
be better...
Sure..
- line 705, there is no check on "t" being non-null, why not use
type.compareToIgnoreCase(t) instead?
Yep
- line 710, exact match on algorithm? I thought u will support partial
match, e.g. AES entry matching AES/CBC/PKCS5Padding request. Why don't u
use "toString()" in the debug output as it provides more info.
The comment is old, I'll remove "full". The code does check for
partial matches because classes like Cipher break up the
transformation into pieces (Cipher.getTransforms()) creating many
ServiceId's with AES, AES/CBC, AES/CBC/PKCS5Padding, etc.
toString() fine. I added it to the previous debug.println too.
- line 722, why is "type" not included in the toString()?
You've found a lot of code cleanup that I missed. It was that from
from older version of the code before I started checking type.
The webrev is updated
http://cr.openjdk.java.net/~ascarpino/8133151/webrev.02/
Thanks,
Valerie
On 10/15/2015 11:42 AM, Valerie Peng wrote:
<MakeJavaSecurity.java>
- line 58-59, the "[openjdk target cpu architecture]" one should be
moved up. The optional restricted packages file names are at the end.
<General>
- for the javadoc changes, the approved CCC has @implNote instead of
@implSpec.
Instead of just {@code getProviders}, it seems {@code
Security.getProviders} is clearer.
<XMLSignatureFactory.java>
- line 262 - 267, given that there is an argument specifying provider
name, I don't think your changes applies to this method. If correct,
the javadoc change should be removed.
<java.security>
- looks fine.
I will continue to look at ProviderList.java and send u comments in a
separate email.
Thanks,
Valerie
On 10/9/2015 10:06 AM, Anthony Scarpino wrote:
Ping for a security review..
Tony
On 10/02/2015 10:08 AM, Anthony Scarpino wrote:
Hi all,
I'm need a review of the last developement piece to JEP 246, the
configuration changes.
I've copied the build-dev in case there were any comments on the
minor
changes in the make directory related to the java.security file.
http://cr.openjdk.java.net/~ascarpino/8133151/webrev/
thanks
Tony
