On Mon, 29 Sep 2025 03:45:38 GMT, Mark Powers <[email protected]> wrote:
>> [JDK-8343232](https://bugs.openjdk.org/browse/JDK-8343232) > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > another day another iteration src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 26: > 24: */ > 25: > 26: package com.sun.crypto.provider; This class can be moved to the `sun/security/pkcs12` package and made package-private as it is only referenced by `PKCS12KeyStore`. We can always move it again later if necessary. src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 58: > 56: * -- PBKDF2 > 57: * > 58: * {@link PBKDF2Parameters} I don't think this link works since `PBKDF2Parameters` is in another package. Note: javadoc is not generated by default for internal classes. While it is still helpful to write class/method comments in the javadoc format, for something like the above, it might be more useful to say something like "See sun.security.util.PBKDF2Parameters for ..." so it is more readable as a comment. src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 84: > 82: private int keyLength = -1; > 83: > 84: protected void Init(AlgorithmParameterSpec paramSpec) I don't think you need a separate method. Put this code in the constructor. Same for the other `Init` method. Then you can also make the fields final. src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 86: > 84: protected void Init(AlgorithmParameterSpec paramSpec) > 85: throws InvalidParameterSpecException { > 86: if (!(paramSpec instanceof PBEParameterSpec)) { Use the instanceof pattern (see JEP 394) to avoid the need to cast on lines 90 and 91. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 51: > 49: * @author Sharon Liu > 50: */ > 51: It would be useful to add the ASN.1 definition here for reference. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 174: > 172: } > 173: > 174: void processMacData(AlgorithmParameterSpec params, Can be static? src/java.base/share/classes/sun/security/pkcs12/MacData.java line 178: > 176: throws Exception { > 177: final String kdfHmac; > 178: final String Hmac; Use lower-case as first letter of variable names, s/Hmac/hmac src/java.base/share/classes/sun/security/pkcs12/MacData.java line 186: > 184: kdfHmac = macAlgorithm; > 185: Hmac = macAlgorithm; > 186: } Did you consider adding another kdfHmac parameter to the method and passing in the correct values? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392601034 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392616902 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392624684 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392641323 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392648607 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392679324 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392669962 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2392695506
