On Tue, 19 Mar 2024 06:20:53 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 82:
>
>> 80: private final Map<String,Reference<PrivateKeyEntry>> entryCacheMap;
>> 81:
>> 82: private boolean ksP12;
>
> Could `ksP12` also be `final`?
Made it final.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 106:
>
>> 104: this.builders = builders;
>> 105: uidCounter = new AtomicLong();
>> 106: KeyStore keyStore = null;
>
> It may be better to define `keyStore` in the below for-loop.
Done.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 109:
>
>> 107: boolean foundPKCS12 = false;
>> 108:
>> 109: for (int i = 0, n = builders.size(); i < n; i++) {
>
> The index `i` just be used for retrieving the elements from the list, then it
> can apply enhanced for-loop.
Done.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 335:
>
>> 333:
>> 334: // Convert KeyStore certificates to X509Certificates
>> 335: X509Certificate[] newCerts = Arrays.copyOf(keyStoreCerts,
>
> Does it have to convert `Certificate[]` to `X509Certificate[]`?
> The doc for `Arrays::equals` states
>> the two arrays are equal if they contain the same elements in the same order.
>
> It looks this method doesn't care the types of the arrays themselves.
Removed conversion.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 339:
>
>> 337: return Arrays.equals(cachedCerts, newCerts);
>> 338: } catch (Exception e) {
>> 339: return false;
>
> Should this exception be logged?
Done.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 356:
>
>> 354: }
>> 355:
>> 356: PrivateKeyEntry privateKeyEntry = (PrivateKeyEntry) newEntry;
>
> Would you like to apply pattern matching for `instanceof`?
>
> if (!(newEntry instanceof PrivateKeyEntry privateKeyEntry)) {
> return false;
> }
>
> PrivateKey privateKey = privateKeyEntry.getPrivateKey();
Done.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 358:
>
>> 356: PrivateKeyEntry privateKeyEntry = (PrivateKeyEntry) newEntry;
>> 357: PrivateKey privateKey = privateKeyEntry.getPrivateKey();
>> 358: Certificate[] certs = privateKeyEntry.getCertificateChain();
>
> The method `getCertificateChain(String)` contains the below codes (line
> 211~213),
>
> PrivateKeyEntry entry = getEntry(alias);
> return entry == null ? null :
> (X509Certificate[])entry.getCertificateChain();
>
> This method assumes `PrivateKeyEntry::getCertificateChain` should always
> return `X509Certificate[]`.
>
> Then, could line 358 also cast the certificate chain to `X509Certificate[]`
> directly?
> If so, the following codes can be simplified.
> It may not need to check the certification type, and could not to convert the
> `Certificate[]` to `X509Certificate[]`.
Changed to cast the certificate chain.
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 410:
>
>> 408: return mapEntryUpdated;
>> 409: } catch (Exception e) {
>> 410: return false;
>
> Should this exception be logged?
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125307
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125365
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125397
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125509
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125456
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125429
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125548
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1535125483
