On Thu, 1 Aug 2024 04:11:24 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.
looks good - some minor issues highlighted
src/java.base/share/classes/jdk/internal/access/JavaIOFileInputStreamAccess.java
line 30:
> 28: import java.io.IOException;
> 29:
> 30: import jdk.internal.ref.PhantomCleanable;
these imports aren't required
src/java.base/share/classes/sun/security/util/KeyStoreDelegator.java line 41:
> 39: import java.util.Set;
> 40:
> 41: import jdk.internal.access.JavaIOFileInputStreamAccess;
shouldn't be required
src/java.base/share/classes/sun/security/util/KeyStoreDelegator.java line 296:
> 294: .getJavaIOFileInputStreamAccess()
> 295: .getPath((FileInputStream) stream);
> 296: if (keystorePath != null) {
if `keystorePath` is null, no "Loaded" statement is printed. Let's cover that
case also.
src/java.base/share/classes/sun/security/util/KeyStoreDelegator.java line 297:
> 295: .getPath((FileInputStream) stream);
> 296: if (keystorePath != null) {
> 297: debug.println("Loaded " + keystorePath.substring(
looking at debug output - it might be useful to surround the filename with
quotes to emphasize that name is being printed. e.g.
Loaded "keystore" keystore in PKCS12 format
test/jdk/java/security/KeyStore/LogKeyStorePathVerifier.java line 27:
> 25: * @test
> 26: * @bug 8329251
> 27: * @library ../../
don't think this line is necessary
test/jdk/java/security/KeyStore/LogKeyStorePathVerifier.java line 31:
> 29: * @summary Validates the customized keystore/ truststore paths
> 30: * in the debug logs
> 31: * @run main LogKeyStorePathVerifier
please run in ovm mode since test is reading system wide properties
-------------
PR Review: https://git.openjdk.org/jdk/pull/20414#pullrequestreview-2213714078
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1700708610
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1700711853
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1700714989
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1700714024
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1700715819
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1700716269
