On Wed, 17 May 2023 21:54:20 GMT, Kevin Driver <kdri...@openjdk.org> wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > rework based upon code review comments Similar comments for update in CertificateRequest.java Similar comments for update in CertificateRequest.java src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java line 126: > 124: } > 125: > 126: X500Principal[] getAuthorities() throws IllegalArgumentException > { IAE is unchecked exception, and should not be throwing explicitly in method signature/statement. I'm not sure if this throwing is really helpful for caller to check the exception. src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java line 147: > 145: builder.append(principal.toString()); > 146: } catch (IllegalArgumentException iae) { > 147: builder.append("unparseable X500Principal"); I may use the iae message as well for better debugging. src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java line 289: > 287: shc.peerSupportedAuthorities = spec.getAuthorities(); > 288: } catch (IllegalArgumentException iae) { > 289: shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal > could not be parsed"); To easy debugging, I may use the exception with fatal(Alert alert, String diagnostic, Throwable cause). Considering this point, I may prefer to throw SSLException in getAuthorities() method. X500Principal[] getAuthorities() throws SSLException { ... try { shc.peerSupportedAuthorities = spec.getAuthorities(); } catch (SSLException ssle) { shc.conContext.fatal(Alert.DECODE_ERROR, "Cannot parse peer supported authorities", ssle); } ------------- Changes requested by xuelei (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1431961001 PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1431970382 PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197326576 PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197327083 PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197334523
