On Thu, 18 May 2023 03:51:55 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> rework based upon code review comments
>
> 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.
Yes, I agree with that comment. I suggest adding a comment above the method to
remind callers they may need to catch IAE, something like:
// This method will throw IllegalArgumentException if the X500Principal cannot
be parsed.
> 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.
Yes, suggest:
`builder.append("unparseable X500Principal: " + iae.getMessage());`
> 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);
> }
@XueleiFan Seems like an unnecessary wrapping of IAE when it is going to be
rethrown anyway.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197768251
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197771043
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197773420
