Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:23:25 GMT, Bradford Wetmore wrote: >> src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82: >> >>> 80: String type; >>> 81: type = GetPropertyAction.privilegedGetProperty( >>> 82: "ssl.TrustManagerFactory.algorithm"); >> >> Sorry I got it wrong here, this is a security property. > > Similar comment. Back to the original lambda expression. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 17:29:53 GMT, Bradford Wetmore wrote: >> My mistake. It's only the trim that you wanted removed, line 94. > > No, the API for Security.getProperty doesn't specify trimming, so suggest > leaving the trim() part also. Okay. Line 94 is back. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:37:35 GMT, Mark Powers wrote: >> `Security.getProperty()` does not specify the value will be `trim()`. > > My mistake. It's only the trim that you wanted removed, line 94. No, the API for Security.getProperty doesn't specify trimming, so suggest leaving the trim() part also. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:27:08 GMT, Bradford Wetmore wrote: >> Just found the same. This needs to be reverted. You can set a Security >> Property to an "empty" string which won't work here. Suggest you revert to >> previous code, possibly using a lambda if that was the original intent. > > `Security.getProperty()` does not specify the value will be `trim()`. My mistake. It's only the trim that you wanted removed, line 94. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:22:43 GMT, Bradford Wetmore wrote: >> src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92: >> >>> 90: static String getSecurityProperty(final String name) { >>> 91: return AccessController.doPrivileged((PrivilegedAction) >>> () -> { >>> 92: return Security.getProperty(name); >> >> I assume we still need to do the if-empty-then-null conversion? > > Just found the same. This needs to be reverted. You can set a Security > Property to an "empty" string which won't work here. Suggest you revert to > previous code, possibly using a lambda if that was the original intent. `Security.getProperty()` does not specify the value will be `trim()`. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:14:01 GMT, Bradford Wetmore wrote: >> src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70: >> >>> 68: String type; >>> 69: type = GetPropertyAction.privilegedGetProperty( >>> 70: "ssl.KeyManagerFactory.algorithm"); >> >> So sorry I got it wrong here, this is a security property. >> `GetPropertyAction.privilegedGetProperty` is for system properties. > > I just noticed the same. Interesting that mach5 tier1 and tier2 tests passed. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 15:45:58 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains eight additional >> commits since the last revision: >> >> - Merge >> - Max and Brad comments >> - jaikiran comments >> - Alan Bateman comments >> - second iteration >> - Merge >> - Merge >> - first iteration > > src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92: > >> 90: static String getSecurityProperty(final String name) { >> 91: return AccessController.doPrivileged((PrivilegedAction) >> () -> { >> 92: return Security.getProperty(name); > > I assume we still need to do the if-empty-then-null conversion? Just found the same. This needs to be reverted. You can set a Security Property to an "empty" string which won't work here. Suggest you revert to previous code, possibly using a lambda if that was the original intent. > src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82: > >> 80: String type; >> 81: type = GetPropertyAction.privilegedGetProperty( >> 82: "ssl.TrustManagerFactory.algorithm"); > > Sorry I got it wrong here, this is a security property. Similar comment. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 02:33:49 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge > - Max and Brad comments > - jaikiran comments > - Alan Bateman comments > - second iteration > - Merge > - Merge > - first iteration These need to be addressed before integration. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 15:47:44 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains eight additional >> commits since the last revision: >> >> - Merge >> - Max and Brad comments >> - jaikiran comments >> - Alan Bateman comments >> - second iteration >> - Merge >> - Merge >> - first iteration > > src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70: > >> 68: String type; >> 69: type = GetPropertyAction.privilegedGetProperty( >> 70: "ssl.KeyManagerFactory.algorithm"); > > So sorry I got it wrong here, this is a security property. > `GetPropertyAction.privilegedGetProperty` is for system properties. I just noticed the same. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 02:33:49 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge > - Max and Brad comments > - jaikiran comments > - Alan Bateman comments > - second iteration > - Merge > - Merge > - first iteration I made some wrong suggestions earlier. src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70: > 68: String type; > 69: type = GetPropertyAction.privilegedGetProperty( > 70: "ssl.KeyManagerFactory.algorithm"); So sorry I got it wrong here, this is a security property. `GetPropertyAction.privilegedGetProperty` is for system properties. src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92: > 90: static String getSecurityProperty(final String name) { > 91: return AccessController.doPrivileged((PrivilegedAction) > () -> { > 92: return Security.getProperty(name); I assume we still need to do the if-empty-then-null conversion? src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82: > 80: String type; > 81: type = GetPropertyAction.privilegedGetProperty( > 82: "ssl.TrustManagerFactory.algorithm"); Sorry I got it wrong here, this is a security property. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
> https://bugs.openjdk.java.net/browse/JDK-8285504 > > JDK-8273046 is the umbrella bug for this bug. The changes were too large for > a single code review, so it was decided to split into smaller chunks. This is > one such chunk: > > open/src/java.base/share/classes/java/net Mark Powers has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge - Max and Brad comments - jaikiran comments - Alan Bateman comments - second iteration - Merge - Merge - first iteration - Changes: - all: https://git.openjdk.java.net/jdk/pull/8384/files - new: https://git.openjdk.java.net/jdk/pull/8384/files/7624f4a9..6997837f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8384=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8384=02-03 Stats: 12840 lines in 345 files changed: 9325 ins; 1407 del; 2108 mod Patch: https://git.openjdk.java.net/jdk/pull/8384.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8384/head:pull/8384 PR: https://git.openjdk.java.net/jdk/pull/8384