It should be ok to remove the if (secret == null) block of
test/jdk/sun/security/pkcs11/tls/TestPRF.java.
I verified that the test runs fine on Linux without that block, so the
comment is legit.
Thanks,
Valerie
On 5/5/2020 4:17 PM, Mikael Vidstedt wrote:
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/
