On Thu, 5 May 2022 19:38:06 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> This change refactors the PBES2Core and PKCS12PBECipherCore classes in >> SunJCE provider as requested in the bug record. Functionality should remain >> the same with a clearer and simplified code/control flow with less lines of >> code. This should improve readability and maintenance. I enhanced one >> existing regression test to test more scenarios. This test would pass before >> the proposed change and continues to pass with the proposed changes. > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > update copyright year for PBES2Core.java Is it possible to rewrite `PKCS12PBECipherCore.java` so that the implementation inside is `CipherCore` instead of `CipherSpi`? I'd rather create a `CipherSpi` child inside this package as the base for all implementations that does nothing more but simply is able to expose all those `engineXYZ` methods to other classes in the same package. Sigh. src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 229: > 227: if (key instanceof javax.crypto.interfaces.PBEKey > pbeKey) { > 228: salt = check(pbeKey.getSalt()); // may be null > 229: iCount = check(pbeKey.getIterationCount()); // may > be 0 It seems the return value is never 0. src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java line 183: > 181: "for PBEWithSHA1And" + algo); > 182: } > 183: } How about using switch/case or at least put the `if` in same indentation level? src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java line 214: > 212: > 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params, > 214: SecureRandom random, CipherSpi cipher) Why rename `cipherImpl` to `cipher`? I think `cipher` is usually a `Cipher` object and `cipherImpl` is a good name for a `CipherSpi` object. src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java line 215: > 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params, > 214: SecureRandom random, CipherSpi cipher) > 215: throws InvalidKeyException, InvalidAlgorithmParameterException { Indent the line above. src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java line 314: > 312: } else if (cipher instanceof DESedeCipher tripleDes) > { > 313: tripleDes.engineInit(opmode, cipherKey, ivSpec, > random); > 314: } else { Can we combine the 2 if above? What else type can it be? ------------- PR: https://git.openjdk.java.net/jdk/pull/8521