Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Fri, 18 Mar 2022 21:08:56 GMT, Bradford Wetmore wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> more test case udpate
>
> test/jdk/javax/net/ssl/SSLException/CheckSSLHandshakeException.java line 32:
>
>> 30: import java.util.Objects;
>> 31:
>> 32: public class CheckSSLHandshakeException {
>
> My personal preference would have been to combine all of these into a single
> testfile to minimize clutter in the logs and directories, but no strong
> objection.
I understand. I would like to have them in separated test cases so that it is
easier to monitor the regressions in the future.
-
PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v6]
> 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: update copyright years - Changes: - all: https://git.openjdk.java.net/jdk/pull/7722/files - new: https://git.openjdk.java.net/jdk/pull/7722/files/61cc7934..5d0fcf2e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=04-05 Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 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
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Fri, 18 Mar 2022 23:05:36 GMT, Iris Clark wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> more test case udpate > > Marked as reviewed by iris (Reviewer). Thanks, @irisclark! - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Tue, 8 Mar 2022 08:02:16 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: > > more test case udpate Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Fri, 18 Mar 2022 21:00:40 GMT, Bradford Wetmore wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> more test case udpate > > test/jdk/javax/net/ssl/SSLException/CheckSSLHandshakeException.java line 2: > >> 1: /* >> 2: * Copyright (C) 2022 THL A29 Limited, a Tencent company. All rights >> reserved. > > I am unsure about the third-party copyrights on these files. They are > probably ok, but you should get approval by someone more familiar. > > Secondly, in the Oracle environment, we generally use a trailing , comma > after the date. e.g. > > `Copyright (C) 2022, ` The location of the copyright is fine. No need to enforce Oracle date formatting. - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Tue, 8 Mar 2022 08:02:16 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:
>
> more test case udpate
Please see latest comments and let's verify the copyright is correct, but
otherwise looks good.
test/jdk/javax/net/ssl/SSLException/CheckSSLHandshakeException.java line 32:
> 30: import java.util.Objects;
> 31:
> 32: public class CheckSSLHandshakeException {
My personal preference would have been to combine all of these into a single
testfile to minimize clutter in the logs and directories, but no strong
objection.
test/jdk/javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java line 349:
> 347: }
> 348: }
> 349: } finally {
Copyright update
test/jdk/javax/net/ssl/templates/SSLSocketTemplate.java line 544:
> 542: /*
> 543: * Check various exception conditions.
> 544: */
Copyright update
-
Marked as reviewed by wetmore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Tue, 8 Mar 2022 08:02:16 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:
>
> more test case udpate
test/jdk/javax/net/ssl/ALPN/SSLServerSocketAlpnTest.java line 487:
> 485: if ((local != null) && (remote != null)) {
> 486: // If both failed, return the curthread's exception.
> 487: local.addSuppressed(remote);
Copyright 2016->2022.
test/jdk/javax/net/ssl/ALPN/SSLSocketAlpnTest.java line 483:
> 481: if ((local != null) && (remote != null)) {
> 482: // If both failed, return the curthread's exception.
> 483: local.addSuppressed(remote);
Copyright 2016->2022.
test/jdk/javax/net/ssl/SSLException/CheckSSLHandshakeException.java line 2:
> 1: /*
> 2: * Copyright (C) 2022 THL A29 Limited, a Tencent company. All rights
> reserved.
I am unsure about the copyrights on these files. They are probably ok, but you
should get approval by someone more familiar.
-
PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Tue, 8 Mar 2022 08:02:16 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: > > more test case udpate ping ... - PR: https://git.openjdk.java.net/jdk/pull/7722
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 [v5]
> 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: more test case udpate - Changes: - all: https://git.openjdk.java.net/jdk/pull/7722/files - new: https://git.openjdk.java.net/jdk/pull/7722/files/37953310..61cc7934 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=03-04 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 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
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v4]
> 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: update usage in test template - Changes: - all: https://git.openjdk.java.net/jdk/pull/7722/files - new: https://git.openjdk.java.net/jdk/pull/7722/files/628605e6..37953310 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=02-03 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 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
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 [v3]
> 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: update more usage and add test cases - Changes: - all: https://git.openjdk.java.net/jdk/pull/7722/files - new: https://git.openjdk.java.net/jdk/pull/7722/files/edb185ec..628605e6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=01-02 Stats: 216 lines in 10 files changed: 200 ins; 5 del; 11 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
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
On Mon, 7 Mar 2022 07:52:29 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
src/java.base/share/classes/javax/net/ssl/SSLException.java line 52:
> 50: */
> 51: public SSLException(String reason)
> 52: {
Thanks for changing the style. I've always hated the extra vertical space. :)
-
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&pr=7722&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=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
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions
On Mon, 7 Mar 2022 07:52:29 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 LGTM. Someone from security-dev should probably review this too. - PR: https://git.openjdk.java.net/jdk/pull/7722
RFR: 8282723: Add constructors taking a cause to JSSE exceptions
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 - Commit messages: - 8282723: Add constructors taking a cause to JSSE exceptions Changes: https://git.openjdk.java.net/jdk/pull/7722/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7722&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282723 Stats: 174 lines in 19 files changed: 64 ins; 48 del; 62 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
