On Mon, 25 Mar 2024 02:17:18 GMT, John Jiang <jji...@openjdk.org> wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updated with John's comments
>
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 168:
>
>> 166: boolean mapEntryUpdated =
>> processCredentials(builder,
>> 167: i, alias);
>> 168: if (!mapEntryUpdated){
>
> Nit: need a space between `)` and `{`.
Space added.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 191:
>
>> 189: @Override
>> 190: public X509Certificate[] getCertificateChain(String alias) {
>> 191: if (ksP12) {
>
> It looks the new codes in method `getCertificateChain` and `getPrivateKey`
> are quite similar.
> They both mainly retrieve the `X509Credentials` from the cached map, though
> finally the former returns certificate chain and the latter returns the
> private key.
> So, it may be better to abstract a method for retrieving the
> `X509Credentials`, and this new method can be shared by the
> `getCertificateChain` and `getPrivateKey`.
New method created.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 302:
>
>> 300: //
>> 301:
>> 302: private String removeAliasIndex (String alias){
>
> Nit: the space between `x` and `(` can be removed; a space is needed between
> `)` and `{`.
Fixed.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 368:
>
>> 366: credentialsMap.put(builderAlias, cred);
>> 367:
>> 368: if (SSLLogger.isOn && SSLLogger.isOn("keymanager")) {
>
> The method `processCredentials` itself logs for the success, however the
> callers log for the failure (see line 169 and 388).
> Especially, the logs on successful and failed cases display different
> contents.
> The log for the failed case doesn't contain the builder index and entry alias.
>
> Could it also log the failed cases in the body of this method?
> That should log the messages before returning false.
> Then, the callers may not need to log for the method.
Done as suggested.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 369:
>
>> 367:
>> 368: if (SSLLogger.isOn && SSLLogger.isOn("keymanager")) {
>> 369: SSLLogger.fine("found key for : " +
>
> May not need the space between `for` and `:`.
Space removed.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 371:
>
>> 369: SSLLogger.fine("found key for : " +
>> 370: "keystore builder index = " + builderIndex +
>> 371: " alias = " + alias, (Object[]) certs);
>
> Suggestion: add a `,` after the first `"`. That can separate the parameters
> `index` and `alias` clearly.
"," added.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631064
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631147
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631089
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631262
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631176
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631196
