On Wed, 24 Sep 2025 13:23:44 GMT, Weijun Wang <[email protected]> wrote:
>> Mark Powers has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix behavior with keytool
>
> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1487:
>
>> 1485: int writeIterationCount = macIterationCount;
>> 1486:
>> 1487: if (newKeystore) {
>
> I cannot see how `newKeystore` is useful. Back when it's set to true,
> `macAlgorithm` was also set to `defaultMacAlgorithm()`. Therefore there is no
> need to try `defaultMacAlgorithm()` again below. As for
> `writeIterationCount`, for a new keystore it is -1, so we can use this to
> decide whether it should be assigned `defaultMacIterationCount()`.
`newKeystore` and `macAlgorithm` are not always set together. When creating a
keystore, they are both set, but when reading a keystore only `macAlgorithm` is
set. So if I read first and then write (no create), `newKeystore` will not be
set. Therefore, I can't remove `newKeystore` and only use `macAlgorithm` if
that's what you're suggesting.
I agree that `defaultMacAlgorithm()` can be replaced by `macAlgorithm` on lines
1489 and 1490.
`writeIterationCount` is initialized to `defaultMacIterationCount()` on. line
1253 so it is never -1 when `calculateMac` is entered. `writeIterationCount` is
also set when a keystore is read (lines 2209 and 2220). I probably shouldn't be
doing that.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2376265799
