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

Reply via email to