RFR 8133151: Preferred provider configuration for JCE

[Valerie Peng]

Sure.
Valerie

On 10/16/2015 3:52 PM, Anthony Scarpino wrote:
> 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
>>>>>>
>>>
>