RFR 8133151: Preferred provider configuration for JCE

[Valerie Peng]

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
>>>>
>