Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Tue, 8 Mar 2022 05:51:21 GMT, Xue-Lei Andrew Fan wrote: >> src/java.base/share/classes/sun/security/ssl/ECDHKeyExchange.java line 204: >> >>> 202: } catch (GeneralSecurityException | java.io.IOException e) >>> { >>> 203: throw new SSLHandshakeException( >>> 204: "Could not generate ECPublicKey", e); >> >> Nit: I think combining these lines would be < 80 chars > > The exception name is too long to have in one line. There are 83 characters > in total if combining line 203 and 204. 3 characters exceeding 80 chars per > line limit is not easy to tell with eyes. The 80 character recommendation isn't a hard limit, I favor readability in the 80-100 char range. - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 20:34:02 GMT, Bradford Wetmore wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> typo correction > > src/java.base/share/classes/sun/security/ssl/ECDHKeyExchange.java line 204: > >> 202: } catch (GeneralSecurityException | java.io.IOException e) { >> 203: throw new SSLHandshakeException( >> 204: "Could not generate ECPublicKey", e); > > Nit: I think combining these lines would be < 80 chars The exception name is too long to have in one line. There are 83 characters in total if combining line 203 and 204. 3 characters exceeding 80 chars per line limit is not easy to tell with eyes. - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 20:36:41 GMT, Bradford Wetmore wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> typo correction > > src/java.base/share/classes/sun/security/ssl/SSLSocketInputRecord.java line > 263: > >> 261: fragment = plaintext.fragment; >> 262: contentType = plaintext.contentType; >> 263: } catch (BadPaddingException bpe) { > > Does the copyright need to get updated? Oops, I missed this file. - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 20:24:08 GMT, Rajan Halade wrote: > Update following for SSLPeerUnverifiedException - > > https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/com/sun/jndi/ldap/ext/StartTlsResponseImpl.java#L439 Hm, I will check more usage out of the JSSE implementation code. Thank you! > Please consider adding simple tests to cover new APIs, similar to > https://github.com/openjdk/jdk/pull/7705/files#diff-28e1041be519b6266b3b92aea29c5d8917c279880da7e5e9823a518196e96ea5 Thank you for the reference. I will add some test cases. - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 19:42:47 GMT, Xue-Lei Andrew Fan wrote: >> Please review this small API enhancement to add the usual constructors >> taking a cause to javax.net.ssl exceptions. The use of initCause in the >> JSSE implementation code is updated to use the new constructors accordingly. >> >> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > typo correction > - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 20:39:44 GMT, Xue-Lei Andrew Fan wrote: >> src/java.base/share/classes/sun/security/ssl/SSLTrafficKeyDerivation.java >> line 158: >> >>> 156: } catch (GeneralSecurityException gse) { >>> 157: throw new SSLHandshakeException( >>> 158: "Could not generate secret", gse); >> >> I can't quite tell given the coloring, but did this get changed into a tab? > > Yes, I added 4 more whitespaces in line 158. Ah, missed that. Good to be consistent. - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 20:30:07 GMT, Bradford Wetmore wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> typo correction > > src/java.base/share/classes/sun/security/ssl/SSLTrafficKeyDerivation.java > line 158: > >> 156: } catch (GeneralSecurityException gse) { >> 157: throw new SSLHandshakeException( >> 158: "Could not generate secret", gse); > > I can't quite tell given the coloring, but did this get changed into a tab? Yes, I added 4 more whitespaces in line 158. - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 19:42:47 GMT, Xue-Lei Andrew Fan wrote: >> Please review this small API enhancement to add the usual constructors >> taking a cause to javax.net.ssl exceptions. The use of initCause in the >> JSSE implementation code is updated to use the new constructors accordingly. >> >> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > typo correction Didn't see any unit tests for the new methods. Can approve after they are included. src/java.base/share/classes/sun/security/ssl/ECDHKeyExchange.java line 204: > 202: } catch (GeneralSecurityException | java.io.IOException e) { > 203: throw new SSLHandshakeException( > 204: "Could not generate ECPublicKey", e); Nit: I think combining these lines would be < 80 chars src/java.base/share/classes/sun/security/ssl/SSLSocketInputRecord.java line 263: > 261: fragment = plaintext.fragment; > 262: contentType = plaintext.contentType; > 263: } catch (BadPaddingException bpe) { Does the copyright need to get updated? - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 19:42:47 GMT, Xue-Lei Andrew Fan wrote: >> Please review this small API enhancement to add the usual constructors >> taking a cause to javax.net.ssl exceptions. The use of initCause in the >> JSSE implementation code is updated to use the new constructors accordingly. >> >> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > typo correction src/java.base/share/classes/sun/security/ssl/SSLTrafficKeyDerivation.java line 158: > 156: } catch (GeneralSecurityException gse) { > 157: throw new SSLHandshakeException( > 158: "Could not generate secret", gse); I can't quite tell given the coloring, but did this get changed into a tab? - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 19:42:47 GMT, Xue-Lei Andrew Fan wrote: >> Please review this small API enhancement to add the usual constructors >> taking a cause to javax.net.ssl exceptions. The use of initCause in the >> JSSE implementation code is updated to use the new constructors accordingly. >> >> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > typo correction Please consider adding simple tests to cover new APIs, similar to https://github.com/openjdk/jdk/pull/7705/files#diff-28e1041be519b6266b3b92aea29c5d8917c279880da7e5e9823a518196e96ea5 - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 19:42:47 GMT, Xue-Lei Andrew Fan wrote: >> Please review this small API enhancement to add the usual constructors >> taking a cause to javax.net.ssl exceptions. The use of initCause in the >> JSSE implementation code is updated to use the new constructors accordingly. >> >> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724 > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > typo correction Update following for SSLPeerUnverifiedException - https://github.com/openjdk/jdk/blob/master/src/java.naming/share/classes/com/sun/jndi/ldap/ext/StartTlsResponseImpl.java#L439 - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
> Please review this small API enhancement to add the usual constructors taking > a cause to javax.net.ssl exceptions. The use of initCause in the JSSE > implementation code is updated to use the new constructors accordingly. > > Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724 Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: typo correction - Changes: - all: https://git.openjdk.java.net/jdk/pull/7722/files - new: https://git.openjdk.java.net/jdk/pull/7722/files/20c0c6b6..edb185ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7722=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7722=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7722.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7722/head:pull/7722 PR: https://git.openjdk.java.net/jdk/pull/7722