On Thu, 7 Nov 2024 04:44:25 GMT, Prasadrao Koppula <pkopp...@openjdk.org> wrote:
>> Using SharedSecrets, I attempted to expose FileInputStream::path >> information. After implementing the fix, I validated the startup performance >> tests. Observed no consistent pattern of performance drops or gains, can >> disregard the occasional performance drop observed in 1 or 2 runs. > > Prasadrao Koppula has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains 11 commits: > > - Merge master > - initialized storeName with empty string > - Replaced Paths.get with Path.of > - Removed unnecessary code > - Removed unnecessary code > - Handled nested wrappers around FileInputStream > - Handled BIS case as well > - JDK-8329251 > - JDK-8329251 > - JDK-8329251 > - ... and 1 more: https://git.openjdk.org/jdk/compare/f2316f68...c90b4f30 Some initial comments, still reviewing. src/java.base/share/classes/java/security/KeyStore.java line 216: > 214: private static final String KEYSTORE_TYPE = "keystore.type"; > 215: > 216: //The keystore full path Nit, add space after `//` src/java.base/share/classes/java/security/KeyStore.java line 217: > 215: > 216: //The keystore full path > 217: private static String keystorePath = null; No need to initialize to null. src/java.base/share/classes/java/security/KeyStore.java line 811: > 809: } > 810: > 811: // Set up JavaIOInputStreamAccess in SharedSecrets s/JavaIOInputStreamAccess/JavaSecurityKeyStoreAccess/ src/java.base/share/classes/java/security/KeyStore.java line 1515: > 1513: throws IOException, NoSuchAlgorithmException, > CertificateException > 1514: { > 1515: if (kdebug != null) { See my other comment in `TrustStoreManagerFactory`. src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 1: > 1: /* Update copyright date. src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 283: > 281: if (ks != null && SSLLogger.isOn && > SSLLogger.isOn("trustmanager")) { > 282: String keystorePath = SharedSecrets > 283: .getJavaSecurityKeyStoreAccess() This code only works if `java.security.debug=keystore` is also enabled, otherwise it will always return `null`. I think that dependency is not intuitive, and will be hard for users to remember. I think you should change `KeyStore.java` to always cache the filename, whether debug is on or not. src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 286: > 284: .getPath(ks); > 285: if (keystorePath != null) { > 286: SSLLogger.fine(provider.getName() + ": using \"" + > Path.of( Suggest rewording as "Initializing with keystore: keystore.p12 in PKCS12 format from SUN provider" Do you really need to log the provider name? src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1962: > 1960: macAlgorithm = null; > 1961: macIterationCount = -1; > 1962: String storeName = ""; I think it is cleaner to not initialize this (leave it as null) and check for null inside debug statements. Same comment in `PKCS12KeyStore.java`. src/java.base/share/classes/sun/security/provider/JavaKeyStore.java line 1: > 1: /* Update copyright date. src/java.base/share/classes/sun/security/ssl/TrustStoreManager.java line 1: > 1: /* Update copyright date. src/java.base/share/classes/sun/security/util/KeyStoreDelegator.java line 1: > 1: /* Update copyright date. ------------- PR Review: https://git.openjdk.org/jdk/pull/20414#pullrequestreview-2421303647 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833180471 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833180807 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833192049 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833192647 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833125524 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833191501 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833129736 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833138466 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833135436 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1832866804 PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1832873896
