John,
Thanks for the review! Comments/questions inline.. > On May 4, 2020, at 4:02 PM, sha.ji...@oracle.com wrote: > > Hi, > Generally, this patch doesn't take care of the solaris/sunos/ucrypto-related > words in test summaries, code comments, configs and READMEs. > E.g. > test/jdk/javax/net/ssl/templates/SSLEngineTemplate.java > test/jdk/sun/security/provider/MessageDigest/TestSHAClone.java > test/jdk/sun/security/util/Resources/Format.config > test/jdk/sun/security/pkcs11/KeyStore/BasicData/README > Would we need some follow-up issues to double-check this point? I’ve deliberately avoided changing comments I didn’t feel comfortable modifying. I’d prefer to file follow-ups as needed, but if you have specific suggestions (or patches!) I should include in the already huge patch do let me know. > test/jdk/sun/security/tools/keytool/KeyToolTest.java > 39 * Testing Solaris Cryptography Framework PKCS11 keystores: > 40 * # make sure you've already run pktool and set test12 as pin > 41 * echo | java -Dsolaris KeyToolTest > ... > 1863 if (System.getProperty("solaris") != null) { > 1864 // For Solaris Cryptography Framework > 1865 t.srcP11Arg = SUN_SRC_P11_ARG; > 1866 t.p11Arg = SUN_P11_ARG; > 1867 t.testPKCS11(); > 1868 t.testPKCS11ImportKeyStore(); > 1869 t.i18nPKCS11Test(); > 1870 } > It may be necessary to remove the above lines. I was staring at this one for quite a while. AFAICT there’s nothing Solaris specific about that block or the methods it’s calling - to me it looks like the property just happens to be called “solaris”, but it could equally well be called “pkcs11” or “standard” or “foobar”. That said, I don’t know enough about PKCS11 & friends to say for sure. Do let me know if it is indeed dead code and should be removed.. > test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.java > test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.policy > It looks this manual test and the associated policy are solaris-related only. > Could these files be removed as well? > In fact, the solaris-related com.sun.security.auth classes, including > SolarisPrincipal, are already removed. Ah, that indeed seems to be the case. Made me wonder why the test doesn’t fail to compile altogether, but then I noticed that it’s ProblemListed on all platforms.. How about a follow-up since this isn’t strictly related to the Solaris/SPARC removal itself - perhaps https://bugs.openjdk.java.net/browse/JDK-8039280 covers it? > test/jdk/sun/security/pkcs11/tls/TestPRF.java > 114 if (secret == null) { > 115 // This fails on Solaris, but since we never > call this > 116 // API for this case in JSSE, ignore the > failure. > 117 // (SunJSSE uses the > CKM_TLS_KEY_AND_MAC_DERIVE > 118 // mechanism) > 119 System.out.print("X"); > 120 continue; > 121 } > Could remove this block? Good question - the comment certainly makes it sound Solaris specific, but could be a stale comment.. Cheers, Mikael > > On 2020/5/4 13:12, Mikael Vidstedt 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/