RFR 8133151: Preferred provider configuration for JCE

[Anthony Scarpino]

I understand what you're saying about it repeating, but like specifying 
the entry as it gives reassurance which one matched.  Since this is only 
a debug message, I'd prefer to keep it.

thanks for the review

Tony


On 10/16/2015 03:07 PM, Valerie Peng wrote:
>
> 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
>>>>>
>>