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. - 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... - line 705, there is no check on "t" being non-null, why not use type.compareToIgnoreCase(t) instead? - 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.
- line 722, why is "type" not included in the toString()?

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

Reply via email to