Will generate a new webrev shortly, meanwhile some comments inline..
> On May 7, 2020, at 1:02 AM, Weijun Wang <weijun.w...@oracle.com> wrote: > > > >> On May 7, 2020, at 1:05 PM, Mikael Vidstedt <mikael.vidst...@oracle.com> >> wrote: >> >> >> New webrev here: >> >> webrev: >> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/security/open/webrev/ >> incremental: >> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/security.incr/open/webrev/ >> >> Items yet to be resolved: >> >> * File follow-up to drop $ISA support in >> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java > > Maybe keep it now. I remember on Ubuntu the libraries are also in different > directories for x64 and x86. I’ll leave it the way it is in webrev.01. Just to make sure we’re on the same page: the only remaining effect it will have is that the "$ISA/“ substring will be removed if it’s found. A possible removal can perhaps be handled as a follow-up instead? > >> >> * Get confirmation on the “solaris” property in >> test/jdk/sun/security/tools/keytool/KeyToolTest.java > > 1. remove all lines except for the 1st in the test's @summary > 2. remove the whole "if (System.getProperty("solaris") != null)" block in > main() > 3. remove the static final String fields SUN_P11_ARG and SUN_SRC_P11_ARG Thanks for the off-thread education on why this code is indeed Solaris specific: all other platforms need the -addprovider option to enable PKCS11, Solaris comes with enabled by default. > >> >> * Get confirmation if https://bugs.openjdk.java.net/browse/JDK-8039280 is >> enough to cover >> test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.java > > The test has been neglected for a long time, but I think in theory the > SolarisPrincipal can be substituted into UnixPrincipal. Let's keep it. I changed SolarisPrincipal to UnixPrincipal in the .java and .policy files, but did *not* verify that the test works as expected with that change. > >> >> * Get confirmation on what to do about the "(secret == null)” block in >> test/jdk/sun/security/pkcs11/tls/TestPRF.java > > I don't quite understand this test, but I think we can simply remove the > block. The comment seems to suggest that this will not fail on Solaris. Let's > see what would happen on other platforms. Consider it gone! Cheers, Mikael > >> >>> On May 3, 2020, at 10: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/ >> >
