code review request: 8012971 PKCS11Test hiding exception failures

Valerie (Yu-Ching) Peng valerie.peng at oracle.com
Fri Jul 19 23:21:01 UTC 2013


Oh, another thing, I noticed that for ProblemList.txt, line 261 is a 
duplicate of line 289.
Otherwise, looks fine.

BTW, I didn't get to finish reviewing your second fix today, may take a 
few more days to get that done...
Thanks,
Valerie

On 07/19/13 11:19, Anthony Scarpino wrote:
> It was necessary, but I found the unused variable that was triggering 
> the compile error without it.
>
> Tony
>
> On 07/18/2013 03:40 PM, Valerie (Yu-Ching) Peng wrote:
>>
>> Just noticed that the newly-added import statement on line 32 seems
>> redundant?
>> Webrev seems the same?
>> Thanks,
>> Valerie
>>
>> On 07/18/13 15:17, Anthony Scarpino wrote:
>>> On 07/18/2013 02:49 PM, Valerie (Yu-Ching) Peng wrote:
>>>>
>>>> Please find comments inline.
>>>>
>>>> On 07/17/13 13:51, Anthony Scarpino wrote:
>>>>> I have broken these into two webrev.  The first:
>>>>>
>>>>> JDK-8012971 PKCS11Test hiding exception failures
>>>>> http://cr.openjdk.java.net/~ascarpino/8012971/webrev.01/
>>>>>
>>>>> handles the minimum changes needed for PKCS11Test to function and the
>>>>> Problemlist updated to reflect the failures that would show up.
>>>> Looks fine in general, I only have some nit comments, and questions
>>>> since I have not touched NSS much.
>>>> PKCS11Test.java
>>>> 1) line 88, change "this finally block" to "any finally block"
>>> yep
>>>
>>>> 2) line 192, add space before and after the "+" for consistency 
>>>> with the
>>>> rest of the source, e.g. line 213.
>>> yep
>>>
>>>> 3) The comments from line 58-60 say the default value is "nss3" but 
>>>> the
>>>> default value set on line 61 is "softokn3". Seems conflicting?
>>>
>>> yep.  it nss3 use to be the default, must have forgot the change the
>>> comment
>>>
>>>> 4) Is "nss3" only used for Secmod? It seems that everywhere else 
>>>> you use
>>>> the "softokn3" value. If yes, perhaps using "setSecmod()" would be
>>>> clearer than "useNSS()".
>>>
>>> Secmod is the only one currently, but I would rather leave it as NSS
>>> because I named it for the library it's looking for.  If another test
>>> comes around that is not a Secmod test, then setSecmod() doesn't seem
>>> logical.
>>>
>>> webrev is updated in place.
>>>
>>>>
>>>> Will look at 8020424 later today.
>>>> Thanks,
>>>> Valerie
>>>>>
>>>>>
>>>>> The second, are the test changes that fix the problems uncovered by
>>>>> the above change:
>>>>>
>>>>> JDK-8020424 The NSS version should be detected before running crypto
>>>>> tests
>>>>> http://cr.openjdk.java.net/~ascarpino/8020424/webrev.00/
>>>>>
>>>>> Tony
>>>>>
>>>>
>>>
>>
>




More information about the security-dev mailing list