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/
