One more thing:
test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java:
Not worth keeping the tests in this directory.
We do need the tests under test/jdk/sun/security/pkcs11/Config/, please
add them back if you have already removed them.
As stated earlier, the config parsing function can be tested against the
dummy SunPKCS11 provider.
Thanks,
Valerie
On 5/7/2020 2:48 PM, Valerie Peng wrote:
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java:
Maybe we should not support $ISA at all?
I suppose it should be ok not supporting $ISA. Existing PKCS11
provider documentation does not mention it anyway.
test/jdk/sun/security/pkcs11/Provider/ConfigShortPath.java:
Will this test ever run? There is no out-of-box SunPKCS11
provider. Even if one is configured manually, the name is likely to
be SunPKCS11-XYZ,
This test will be run as there is an out-of-box PKCS11 provider named
"SunPKCS11" (which is sort of a dummy for actual crypto operations but
it can still be used to test the generic configuration parsing
functionality in this test).
As for removing the tests, I am more on the conservative side. We
should only remove tests which are no longer necessary due to the
removal of Solaris support. There are already a lot of changes
involved here.
Valerie
On 5/4/2020 6:58 AM, Weijun Wang wrote:
I've gone through all files. Many of them are PKCS11-related, it will
be nice if Valerie can confirm my findings.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java:
Maybe we should not support $ISA at all?
test/jdk/com/sun/security/auth/login/ConfigFile/InconsistentError.java:
Please also remove the whole else block from line 61.
test/jdk/java/security/KeyPairGenerator/SolarisShortDSA.java:
It's not worth keeping this test. Error has never occurred on
other platforms.
test/jdk/java/security/KeyStore/TestKeyStoreBasic.java:
128-135: There is no need to use a try-catch block.
test/jdk/java/security/KeyStore/TestKeyStoreEntry.java:
54-60: No need
81-97: No need. Just a single run() is OK.
101: Remove the ", p" argument
test/jdk/java/security/MessageDigest/TestDigestIOStream.java:
test/jdk/java/security/MessageDigest/TestSameLength.java:
test/jdk/java/security/MessageDigest/TestSameValue.java:
No need for isSHA3Supported(). The SUN provider should always be
there.
Inside the test where NoSuchAlgorithmException is caught, the
catch block can be removed and no need to try
test/jdk/java/security/MessageDigest/UnsupportedProvider.java:
Not worth keeping this test.
test/jdk/sun/security/jca/PreferredProviderTest.java:
53: remove "for other platform"
test/jdk/sun/security/pkcs/pkcs8/PKCS8Test.java
Remove the comments on lines 37-40
test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java:
Not worth keeping the tests in this directory.
test/jdk/sun/security/pkcs11/Mac/MacSameTest.java:
No need for try
test/jdk/sun/security/pkcs11/Provider/ConfigShortPath.java:
Will this test ever run? There is no out-of-box SunPKCS11
provider. Even if one is configured manually, the name is likely to
be SunPKCS11-XYZ,
test/jdk/sun/security/pkcs11/tls/TestKeyMaterial.java:
No need for this catch block
test/jdk/sun/security/tools/keytool/fakegen/PSS.java:
Please also remove the comment on lines 35-37.
test/jdk/sun/security/tools/keytool/fakegen/DefaultSignatureAlgorithm.java:
Please also remove the comment on lines 36-38.
Thanks,
Max
On May 4, 2020, at 1:12 PM, Mikael Vidstedt
<[email protected]> 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/
