> On May 5, 2020, at 7:29 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
>
>
>
>> On May 6, 2020, at 6:51 AM, Mikael Vidstedt <mikael.vidst...@oracle.com>
>> wrote:
>>
>>
>> Max,
>>
>> Thank you so much for the thorough review! I’m working on an incremental
>> webrev but could use some help - comments/questions inline..
>>
>>> On May 4, 2020, at 6:58 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
>>>
>>> I've gone through all files. Many of them are PKCS11-related, it will be
>>> nice if Valerie can confirm my findings.
>>>
>>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java:
>>>
>>> Maybe we should not support $ISA at all?
>>
>> It does seem like it could go, but I’m not comfortable making that change
>> myself. How about a follow-up issue?
>
> Sure.
>
>>
>>> test/jdk/com/sun/security/auth/login/ConfigFile/InconsistentError.java:
>>>
>>> Please also remove the whole else block from line 61.
>>
>> Fixed.
>>
>>> test/jdk/java/security/KeyPairGenerator/SolarisShortDSA.java:
>>>
>>> It's not worth keeping this test. Error has never occurred on other
>>> platforms.
>>
>> Fixed.
>>
>>> test/jdk/java/security/KeyStore/TestKeyStoreBasic.java:
>>>
>>> 128-135: There is no need to use a try-catch block.
>>
>> Fixed.
>>
>>> test/jdk/java/security/KeyStore/TestKeyStoreEntry.java:
>>>
>>> 54-60: No need
>>
>> Fixed.
>>
>>> 81-97: No need. Just a single run() is OK.
>>> 101: Remove the ", p” argument
>>
>> I’m not sure I understand what to do here. Help? :)
>
> Since there is only one provider having "jceks" you can
>
> 1 remove run() method
> 2 rename runTest(p) to run()
> 3 call 'KeyStore.getInstance("jceks")' without the "p" argument.
Thank you! I did exactly that.
>
>>
>>> test/jdk/java/security/MessageDigest/TestDigestIOStream.java:
>>> test/jdk/java/security/MessageDigest/TestSameLength.java:
>>> test/jdk/java/security/MessageDigest/TestSameValue.java:
>>>
>>> No need for isSHA3Supported(). The SUN provider should always be there.
>>
>> Fixed.
>>
>>> Inside the test where NoSuchAlgorithmException is caught, the catch block
>>> can be removed and no need to try
>>
>> Fixed.
>>
>>> test/jdk/java/security/MessageDigest/UnsupportedProvider.java:
>>>
>>> Not worth keeping this test.
>>
>> Fixed.
>>
>>> test/jdk/sun/security/jca/PreferredProviderTest.java:
>>>
>>> 53: remove "for other platform”
>>
>> Fixed.
>>
>>> test/jdk/sun/security/pkcs/pkcs8/PKCS8Test.java
>>>
>>> Remove the comments on lines 37-40
>>
>> Fixed.
>>
>>> test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java:
>>>
>>> Not worth keeping the tests in this directory.
>>
>> Fixed.
>>
>>> test/jdk/sun/security/pkcs11/Mac/MacSameTest.java:
>>>
>>> No need for try
>>
>> Fixed.
>>
>>> test/jdk/sun/security/pkcs11/Provider/ConfigShortPath.java:
>>>
>>> Will this test ever run? There is no out-of-box SunPKCS11 provider. Even if
>>> one is configured manually, the name is likely to be SunPKCS11-XYZ,
>>
>> You tell me? :)
>
> This is why I included Valerie, I'll ping her.
Sounds good, I’ll wait for Valerie’s feedback.
Cheers,
Mikael
>
> Thanks,
> Max
>
>>
>>> test/jdk/sun/security/pkcs11/tls/TestKeyMaterial.java:
>>>
>>> No need for this catch block
>>
>> Fixed (removed the InvalidAlgorithmParameterException catch).
>>
>>> test/jdk/sun/security/tools/keytool/fakegen/PSS.java:
>>>
>>> Please also remove the comment on lines 35-37.
>>
>> Fixed.
>>
>>> test/jdk/sun/security/tools/keytool/fakegen/DefaultSignatureAlgorithm.java:
>>>
>>> Please also remove the comment on lines 36-38.
>>
>> Fixed.
>>
>> Cheers,
>> Mikael
>>
>>>
>>>> On May 4, 2020, at 1:12 PM, Mikael Vidstedt <mikael.vidst...@oracle.com>
>>>> wrote:
>>>>
>>>>
>>>> Please review this change which implements part of JEP 381:
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>>>> webrev:
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/security/open/webrev/
>>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>>>>
>>>>
>>>> Note: When reviewing this, please be aware that this exercise was
>>>> *extremely* mind-numbing, so I appreciate your help reviewing all the
>>>> individual changes carefully. You may want to get that coffee cup filled
>>>> up (or whatever keeps you awake)!
>>>>
>>>>
>>>> Background:
>>>>
>>>> Because of the size of the total patch and wide range of areas touched,
>>>> this patch is one out of in total six partial patches which together make
>>>> up the necessary changes to remove the Solaris and SPARC ports. The other
>>>> patches are being sent out for review to mailing lists appropriate for the
>>>> respective areas the touch. An email will be sent to jdk-dev summarizing
>>>> all the patches/reviews. To be clear: this patch is *not* in itself
>>>> complete and stand-alone - all of the (six) patches are needed to form a
>>>> complete patch. Some changes in this patch may look wrong or incomplete
>>>> unless also looking at the corresponding changes in other areas.
>>>>
>>>> For convenience, I’m including a link below[1] to the full webrev, but in
>>>> case you have comments on changes in other areas, outside of the files
>>>> included in this thread, please provide those comments directly in the
>>>> thread on the appropriate mailing list for that area if possible.
>>>>
>>>> In case it helps, the changes were effectively produced by searching for
>>>> and updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”,
>>>> etc. More information about the areas impacted can be found in the JEP
>>>> itself.
>>>>
>>>>
>>>> Testing:
>>>>
>>>> A slightly earlier version of this change successfully passed tier1-8, as
>>>> well as client tier1-2. Additional testing will be done after the first
>>>> round of reviews has been completed.
>>>>
>>>> Cheers,
>>>> Mikael
>>>>
>>>> [1]
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
>>>
>>
>
