> On May 7, 2020, at 4:08 PM, Valerie Peng <valerie.p...@oracle.com> wrote:
>
> 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.
Thank you for verifying!
I’ll revert the removal of the test/jdk/sun/security/pkcs11/Config/ files and
send out a new webrev in a minute!
Cheers,
Mikael
>
> 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/
