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