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? > 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? :) > 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? :) > 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/ >
