Re: RFR: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux [v4]

[Vyom Tewari]

On Tue, 31 May 2022 18:29:30 GMT, Brian Burkhalter  wrote:

>> Modify `sun.net.www.MimeTable.findByFileName(String)` to attempt to find the 
>> file extension in the entire file name if it is not found in the portion of 
>> the name preceding the optional fragment beginning with a hash (`#`).
>
> Brian Burkhalter has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8287237: Simplify code a bit

Looks ok, i tested on centos 7 and it is working as expected.

-

Marked as reviewed by vtewari (Committer).

PR: https://git.openjdk.java.net/jdk/pull/8909

Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v9]

[Vyom Tewari]

On Wed, 10 Mar 2021 17:32:30 GMT, Jayashree S Kumar  
wrote:

>> Issue
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8243376
>> 
>> Problem
>> 
>> The scenario is: 
>> - Some specified target hostname resolves to two IP addresses (always the 
>> same address pair). 
>> - The DNS resolved order of the two ip addresses changes (a usual 
>> LoadBalancer type behavior). 
>> - The CNAME of the two ip addresses differ. 
>> 
>> In SocketPermission class(void getIP() method), it internally resolves and 
>> saves only the first IP address resolved, not all the IP addresses resolved. 
>> - Depending on when the implier/implied SocketPermission hostname is 
>> resolved, the resolved addresses order differs, and the internally saved IP 
>> address mismatches, resulting on SocketPermission#implies() false. 
>> 
>> 
>> Michael McMahon kindly reviewed and suggested changes: 
>> https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html
>
> Jayashree S Kumar has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Jcheck Review: Corrected the tab error

Hi Jay, overall changes look ok to me, please do let me know if you need any 
help.

-

PR: https://git.openjdk.java.net/jdk/pull/1916

Re: RFR: 8260428: Drop support for pre JDK 1.4 DatagramSocketImpl implementations [v3]

[Vyom Tewari]

On Thu, 21 Oct 2021 16:47:44 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my changes (and CSR) to drop support for pre JDK 
>> 1.4 `DatagramSocketImpl` implementations?
>> 
>> These changes propose to drop support for `DatagramSocketImpls` that were 
>> compiled with JDK 1.3 or older, which do not have support for connected 
>> sockets, for peeking at received datagrams, and for joining and leaving a 
>> group at a specific interface. This support is legacy, and should be 
>> relatively safe to remove as such implementations do not compile with JDK 
>> 1.4 or newer.
>> 
>> Finally, with this set of proposed changes, if you have an `oldImpl`, and 
>> don’t use connect, then the methods `joinGroup` and `leaveGroup` will throw 
>> `NoSuchMethodError`.  However, the current behaviour in `DatagramSocketImpl` 
>> is to throw an `UnsupportedOperationsException` for a method not 
>> implemented. Should this set of changes update the `joinGroup` and 
>> `leaveGroup` methods in order to preserve this behaviour?
>> 
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8274633
>> 
>> Kind regards,
>> 
>> Patrick
>
> Patrick Concannon 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 four additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/master' into JDK-8260428
>  - 8260428: Corrected javadoc typos; combined declaration and initialization 
> of variables in DSI; refactored dummy impls in test
>  - Merge remote-tracking branch 'origin/master' into JDK-8260428
>  - 8260428: Drop support for pre JDK 1.4 DatagramSocketImpl implementations

Marked as reviewed by vtewari (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/5887

Re: RFR: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST [v4]

[Vyom Tewari]

On Tue, 19 Oct 2021 13:48:24 GMT, Evan Whelan  wrote:

>> Hi,
>> 
>> Please review my fix for JDK-8274779 which changes how HttpClient and 
>> HttpsClient  checks for equality when comparing request methods. 
>> 
>> When `HttpURLConnection.setRequestMethod` is passed `new String("POST")` 
>> rather than the "POST" String literal, the old behaviour resulted in broken 
>> HttpClients being reused from the `KeepAliveCache`. 
>> 
>> This is because a call to `HttpClient.available()` was never reachable due 
>> to identity equality being used instead of logical equality.
>> 
>> The test case uses an injected KeepAliveCache, to which we put a HttpClient 
>> that is unavailable. By comparing the initial HttpClient's `connectTimeout` 
>> value to the "cached" client's connectTimeout (1234 vs 4321 respectively) we 
>> can assert that these values should never be equal as a new HttpClient 
>> should be created in cases where we can no longer use the cached one.
>> 
>> All CI testing is green for this fix.
>> 
>> Kind regards,
>> Evan
>
> Evan Whelan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added setInCache to HttpClientAccess to replace reflection

Looks good to me.

-

Marked as reviewed by vtewari (Committer).

PR: https://git.openjdk.java.net/jdk/pull/5964

Re: RFR: 8260428: Drop support for pre JDK 1.4 DatagramSocketImpl implementations

[Vyom Tewari]

On Mon, 11 Oct 2021 09:42:12 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my changes (and CSR) to drop support for pre JDK 
> 1.4 `DatagramSocketImpl` implementations?
> 
> These changes propose to drop support for `DatagramSocketImpls` that were 
> compiled with JDK 1.3 or older, which do not have support for connected 
> sockets, for peeking at received datagrams, and for joining and leaving a 
> group at a specific interface. This support is legacy, and should be 
> relatively safe to remove as such implementations do not compile with JDK 1.4 
> or newer.
> 
> Finally, with this set of proposed changes, if you have an `oldImpl`, and 
> don’t use connect, then the methods `joinGroup` and `leaveGroup` will throw 
> `NoSuchMethodError`.  However, the current behaviour in `DatagramSocketImpl` 
> is to throw an `UnsupportedOperationsException` for a method not implemented. 
> Should this set of changes update the `joinGroup` and `leaveGroup` methods in 
> order to preserve this behaviour?
> 
> CSR: https://bugs.openjdk.java.net/browse/JDK-8274633
> 
> Kind regards,
> 
> Patrick

src/java.base/share/classes/java/net/NetMulticastSocket.java line 346:

> 344: // peek at the packet to see who it is from.
> 345: DatagramPacket peekPacket = new 
> DatagramPacket(new byte[1], 1);
> 346: peekPort = getImpl().peekData(peekPacket);

Is it possible for you to combine the variable declaration and assignment as 
follows ?. This will increase the code readability little bit. 

 int peekPort = getImpl().peekData(peekPacket);
 String peekAd = peekPacket.getAddress().getHostAddress();

src/java.base/share/classes/java/net/NetMulticastSocket.java line 383:

> 381: // peek at the packet to see who it is from.
> 382: DatagramPacket peekPacket = new DatagramPacket(new 
> byte[1], 1);
> 383: peekPort = getImpl().peekData(peekPacket);

same as previous comment.

-

PR: https://git.openjdk.java.net/jdk/pull/5887

Re: RFR: 8274949: Use String.contains() instead of String.indexOf() in java.base

[Vyom Tewari]

On Fri, 17 Sep 2021 08:56:47 GMT, Andrey Turbanov 
 wrote:

> String.contains was introduced in Java 5.
> Some code in java.base still uses old approach with `String.indexOf` to check 
> if String contains specified substring.
> I propose to migrate such usages. Makes code shorter and easier to read.

java.net changes look OK to me.

-

Marked as reviewed by vtewari (Committer).

PR: https://git.openjdk.java.net/jdk/pull/5559

Re: RFR: 8270903: sun.net.httpserver.HttpConnection: Improve toString

[Vyom Tewari]

On Thu, 29 Jul 2021 10:22:51 GMT, Julia Boes  wrote:

> This is a minor change that updates 
> `sun.net.httpserver.HttpConnection::toString` to never return null.
> 
> Testing: tier 1-3 all clear

Looks good.

-

Marked as reviewed by vtewari (Committer).

PR: https://git.openjdk.java.net/jdk/pull/4928

Re: RFR: 8263531: Remove unused buffer int

[Vyom Tewari]

On Mon, 12 Jul 2021 20:39:53 GMT, Christoph Langer  wrote:

> The change for JDK-8257001 left an obsolete int field. Remove it.

Marked as reviewed by vtewari (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/4759

Re: RFR: 8269993: [Test]: java/net/httpclient/DigestEchoClientSSL.java contains redundant @run tags

[Vyom Tewari]

On Wed, 7 Jul 2021 09:34:13 GMT, Thejasvi Voniadka  
wrote:

> Hi,
> 
> Please review this simple change to remove redundancy of @run tags. The test 
> "java/net/httpclient/DigestEchoClientSSL.java" contains a couple of redundant 
> @run tags:
> 
>  * @run main/othervm/timeout=300
>  * DigestEchoClientSSL SSL
>  * @run main/othervm/timeout=300
>  * DigestEchoClientSSL SSL
>  * @run main/othervm/timeout=300
>  * -Djdk.http.auth.proxying.disabledSchemes=
>  * -Djdk.http.auth.tunneling.disabledSchemes=
>  * DigestEchoClientSSL SSL PROXY
>  * @run main/othervm/timeout=300
>  * -Djdk.http.auth.proxying.disabledSchemes=
>  * -Djdk.http.auth.tunneling.disabledSchemes=
>  * DigestEchoClientSSL SSL PROXY
> 
> This change is to remove the redundancy. I have run the test and verified 
> that it passes on all platforms following the change.

Looks OK to me.

-

Marked as reviewed by vtewari (Committer).

PR: https://git.openjdk.java.net/jdk/pull/4706

Re: [jdk17] RFR: 8269772: [macos-aarch64] test compilation failed with "SocketException: No buffer space available"

[Vyom Tewari]

On Mon, 5 Jul 2021 11:21:39 GMT, Daniel Fuchs  wrote:

> Please find here a trivial fix for:
> 
> 8269772: [macos-aarch64] test compilation failed with "SocketException: No 
> buffer space available"
> 
> Running several of the websocket tests concurrently on some of the CI 
> machines is causing resource exhaustion, because resources are only freed 
> after the TIMED_WAIT delay has expired once the tests have finished.
> The proposed solution is to run these tests in exclusive dir mode.

Marked as reviewed by vtewari (Committer).

-

PR: https://git.openjdk.java.net/jdk17/pull/210

Re: RFR: 8267938: (sctp) SCTP channel factory methods should check platform support

[Vyom Tewari]

On Fri, 28 May 2021 13:11:43 GMT, Chris Hegarty  wrote:

> The SCTP channel factory methods, namely SctpChannel::open, 
> SctpServerChannel::open, and SctpMultiChannel::open, are specified to throw 
> UnsupportedOperationException, if the SCTP protocol is not supported. 
> Currently, underlying platform support is assumed once the appropriate 
> libsctp.so.1 library is present (along with its supported interface 
> functions). This may not always be the case, e.g. if the Linux sctp kernel 
> module is not present or loaded. In which case a SocketException is thrown.
> 
> It would be more appropriate to check for EPROTONOSUPPORT and 
> ESOCKTNOSUPPORT, and throw UOE rather than SE.
> 
> The existing java/net/SctpSanity.java tests already covers this case, when 
> run on platforms without support.

Looks OK to me.

-

Marked as reviewed by vtewari (Committer).

PR: https://git.openjdk.java.net/jdk/pull/4246

Re: RFR: 8264824: java/net/Inet6Address/B6206527.java doesn't close ServerSocket properly [v2]

[Vyom Tewari]

On Tue, 13 Apr 2021 10:30:18 GMT, Conor Cleary  wrote:

>> ### Description
>> `Inet6Address/B6206527.java` test creates two instances of ServerSocket, 
>> both of which are explicity bound to a Link-Local address. Neither of the 
>> ServerSocket instances are explicitly closed meaning there is no guarantee 
>> that their associated resources are freed. 
>> 
>> ### Fix
>> Each ServerSocket is instantiated in a try-with-resources block. This 
>> ensures that in both cases of success or failure within the 
>> try-with-resources block, the sockets are always closed thanks to 
>> ServerSocket implementing Closeable. The test is also now started in othervm 
>> mode as an added assurance of the test's isolation in the event that 
>> resources are not freed.
>
> Conor Cleary has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removed othervm argument

Marked as reviewed by vtewari (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/3437

Re: RFR: 8263905: Remove finalize methods for SocketInput/OutputStream

[Vyom Tewari]

On Mon, 22 Mar 2021 06:02:37 GMT, Kim Barrett  wrote:

> Please review this change to java.net.SocketInputStream and
> java.net.SocketOutputStream, removing their "finalize" methods.  These
> methods are empty.  Their purpose is to override and suppress the finalize
> methods of their superclasses (FileInputStream and FileOutputStream,
> respectively).  But those classes had their finalize methods removed by
> JDK-8192939 in JDK 12.

Marked as reviewed by vtewari (Committer).

-

PR: https://git.openjdk.java.net/jdk/pull/3111

Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v6]

[Vyom Tewari]

On Fri, 5 Mar 2021 15:08:10 GMT, Michael McMahon  wrote:

>> Jayashree S Kumar has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code Review: Incorporated Indention correction, added getIP and updated 
>> try-catch inside for loop
>
> Changes requested by michaelm (Reviewer).

As you change equal & hashCode method, if possible can you please  add some 
additional  tests in "jdk/java/net/SocketPermission/Equals.java" just to make 
sure we test every corner case.

-

PR: https://git.openjdk.java.net/jdk/pull/1916

Integrated: JDK-8260925: HttpsURLConnection does not work with other JSSE provider.

[Vyom Tewari]

On Tue, 16 Feb 2021 07:50:05 GMT, Vyom Tewari  wrote:

> HttpsURLConnection, works with SunJSSE provider but does not work with other 
> JSSE provider. In case of SunJSSE , HttpsURLConnection set the host name as 
> follows
> 
> s = (SSLSocket)serverSocket;
>if (s instanceof SSLSocketImpl) {
>   ((SSLSocketImpl)s).setHost(host);
>} 
> 
> But in case of other providers(BouncyCastleProvider )  host will not get set 
> and "java.security.cert.CertificateException: No subject alternative name 
> found matching IP address" exception will be thrown.

This pull request has now been integrated.

Changeset: 80182f92
Author:Vyom Tewari 
URL:   https://git.openjdk.java.net/jdk/commit/80182f92
Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod

8260925: HttpsURLConnection does not work  with other JSSE provider.

Reviewed-by: xuelei

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v5]

[Vyom Tewari]

On Thu, 4 Mar 2021 00:27:52 GMT, Xue-Lei Andrew Fan  wrote:

>> Vyom Tewari has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - resolve jcheck issue.
>>  - put if block which will prevent host being set twice in case of 
>> SSLSocketImpl
>
> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line 
> 569:
> 
>> 567: SSLParameters paramaters = s.getSSLParameters();
>> 568: 
>> paramaters.setEndpointIdentificationAlgorithm("HTTPS");
>> 569: if (!(s instanceof SSLSocketImpl)) {
> 
> Please have a comment that the host has been set previously for SSLSocketImpl.

done

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider.

[Vyom Tewari]

On Wed, 3 Mar 2021 15:20:29 GMT, Vyom Tewari  wrote:

>> Vyom, can you provide, or point to a test that exercises the code paths that 
>> have been changed? And also some new test that would fail before the fix and 
>> pass after? 
>> 
>> best regards,
>> 
>> -- daniel
>
>> Vyom, can you provide, or point to a test that exercises the code paths that 
>> have been changed? And also some new test that would fail before the fix and 
>> pass after?
>> 
>> best regards,
>> 
>> -- daniel
> 
> Hi Daniel,
> 
> there are multiple test in 
> "test/jdk/sun/net/www/protocol/https/HttpsURLConnection" which exercises the 
> code paths that have been changed. To be more specific if you change the JSSE 
> provider for example to "BouncyCastle" then these test will fail because host 
> will not set for other(BC) JSSE provider.
> 
> In JDK-8260925, i put the detail instruction how to reproduce this issue. 
> Please do let me know if you need any additional information.
> Vyom

updated the PR

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v6]

[Vyom Tewari]

> HttpsURLConnection, works with SunJSSE provider but does not work with other 
> JSSE provider. In case of SunJSSE , HttpsURLConnection set the host name as 
> follows
> 
> s = (SSLSocket)serverSocket;
>if (s instanceof SSLSocketImpl) {
>   ((SSLSocketImpl)s).setHost(host);
>} 
> 
> But in case of other providers(BouncyCastleProvider )  host will not get set 
> and "java.security.cert.CertificateException: No subject alternative name 
> found matching IP address" exception will be thrown.

Vyom Tewari has updated the pull request incrementally with one additional 
commit since the last revision:

  added a comment that host has bees set previously

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2583/files
  - new: https://git.openjdk.java.net/jdk/pull/2583/files/ce940edc..bcf1fab4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2583=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2583=04-05

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2583.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2583/head:pull/2583

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v4]

[Vyom Tewari]

On Wed, 3 Mar 2021 16:16:48 GMT, Xue-Lei Andrew Fan  wrote:

>> Vyom Tewari has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   reverted the overly deleted else block
>
> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line 
> 566:
> 
>> 564: SSLParameters paramaters = s.getSSLParameters();
>> 565: 
>> paramaters.setEndpointIdentificationAlgorithm("HTTPS");
>> 566: paramaters.setServerNames(List.of(new 
>> SNIHostName(host)));
> 
> Thank you for taking my comment.  But I may not update line 456-458.  A safer 
> update may be in line 566, for example:
> +// The host has been set for SSLSocketImp previously.
> +if (!(s instanced SSLSocketImpl)) {
>   paramaters.setServerNames(List.of(new SNIHostName(host)));
> +}

I did the changes as you suggested.

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v5]

[Vyom Tewari]

> HttpsURLConnection, works with SunJSSE provider but does not work with other 
> JSSE provider. In case of SunJSSE , HttpsURLConnection set the host name as 
> follows
> 
> s = (SSLSocket)serverSocket;
>if (s instanceof SSLSocketImpl) {
>   ((SSLSocketImpl)s).setHost(host);
>} 
> 
> But in case of other providers(BouncyCastleProvider )  host will not get set 
> and "java.security.cert.CertificateException: No subject alternative name 
> found matching IP address" exception will be thrown.

Vyom Tewari has updated the pull request incrementally with two additional 
commits since the last revision:

 - resolve jcheck issue.
 - put if block which will prevent host being set twice in case of SSLSocketImpl

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2583/files
  - new: https://git.openjdk.java.net/jdk/pull/2583/files/424e6ab5..ce940edc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2583=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2583=03-04

  Stats: 8 lines in 1 file changed: 5 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2583.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2583/head:pull/2583

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider.

[Vyom Tewari]

On Wed, 3 Mar 2021 10:09:34 GMT, Daniel Fuchs  wrote:

> Vyom, can you provide, or point to a test that exercises the code paths that 
> have been changed? And also some new test that would fail before the fix and 
> pass after?
> 
> best regards,
> 
> -- daniel

Hi Daniel,

there are multiple test in 
"test/jdk/sun/net/www/protocol/https/HttpsURLConnection" which exercises the 
code paths that have been changed. To be more specific if you change the JSSE 
provider for example to "BouncyCastle" then these test will fail because host 
will not set for other(BC) JSSE provider.

In JDK-8260925, i put the detail instruction how to reproduce this issue. 
Please do let me know if you need any additional information.
Vyom

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v4]

[Vyom Tewari]

> HttpsURLConnection, works with SunJSSE provider but does not work with other 
> JSSE provider. In case of SunJSSE , HttpsURLConnection set the host name as 
> follows
> 
> s = (SSLSocket)serverSocket;
>if (s instanceof SSLSocketImpl) {
>   ((SSLSocketImpl)s).setHost(host);
>} 
> 
> But in case of other providers(BouncyCastleProvider )  host will not get set 
> and "java.security.cert.CertificateException: No subject alternative name 
> found matching IP address" exception will be thrown.

Vyom Tewari has updated the pull request incrementally with one additional 
commit since the last revision:

  reverted the overly deleted else block

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2583/files
  - new: https://git.openjdk.java.net/jdk/pull/2583/files/63532401..424e6ab5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2583=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2583=02-03

  Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2583.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2583/head:pull/2583

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v2]

[Vyom Tewari]

On Wed, 3 Mar 2021 11:36:19 GMT, Vyom Tewari  wrote:

>> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line 
>> 454:
>> 
>>> 452: s = (SSLSocket)factory.createSocket(serverSocket,
>>> 453: host, port, 
>>> true);
>>> 454: } else {
>> 
>> This line should not have been deleted - otherwise NPE will arise later on - 
>> e.g. at line 474. Has this change really been tested?
>
> right,  i just saw failure(66)  in my local environment. I wanted to delete 
> only the below two lines.
> if (s instanceof SSLSocketImpl) {
> ((SSLSocketImpl)s).setHost(host);
> }
> I will update the PR soon, once my  local test successful.

Hi Daniel, i updated the PR with your suggested change.

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v2]

[Vyom Tewari]

On Wed, 3 Mar 2021 10:05:24 GMT, Daniel Fuchs  wrote:

>> Vyom Tewari has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   used List.of instead of Collections.singletonList
>
> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line 
> 454:
> 
>> 452: s = (SSLSocket)factory.createSocket(serverSocket,
>> 453: host, port, 
>> true);
>> 454: } else {
> 
> This line should not have been deleted - otherwise NPE will arise later on - 
> e.g. at line 474. Has this change really been tested?

right,  i just saw failure(66)  in my local environment. I wanted to delete 
only the below two lines.
if (s instanceof SSLSocketImpl) {
((SSLSocketImpl)s).setHost(host);
}
I will update the PR soon, once my  local test successful.

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v3]

[Vyom Tewari]

> HttpsURLConnection, works with SunJSSE provider but does not work with other 
> JSSE provider. In case of SunJSSE , HttpsURLConnection set the host name as 
> follows
> 
> s = (SSLSocket)serverSocket;
>if (s instanceof SSLSocketImpl) {
>   ((SSLSocketImpl)s).setHost(host);
>} 
> 
> But in case of other providers(BouncyCastleProvider )  host will not get set 
> and "java.security.cert.CertificateException: No subject alternative name 
> found matching IP address" exception will be thrown.

Vyom Tewari has updated the pull request incrementally with one additional 
commit since the last revision:

  removed the code that set hostname twice in case of SSLSocketImpl

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2583/files
  - new: https://git.openjdk.java.net/jdk/pull/2583/files/2952f596..63532401

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2583=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2583=01-02

  Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2583.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2583/head:pull/2583

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider.

[Vyom Tewari]

On Wed, 3 Mar 2021 05:01:13 GMT, Vyom Tewari  wrote:

>>> HttpsURLConnection, works with SunJSSE provider but does not work with 
>>> other JSSE provider. In case of SunJSSE , HttpsURLConnection set the host 
>>> name as follows
>>> 
>>> s = (SSLSocket)serverSocket;
>>> if (s instanceof SSLSocketImpl) {
>>> ((SSLSocketImpl)s).setHost(host);
>>> }
>> 
>> Did you copy the code/lines above from 
>> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java?  
>> This is the file you updated in the pull request.  If I read the updated 
>> file right, the line numbers are from 455 to 458.  If the socket is an 
>> instance of SSLSocketImpl, the host is set here.
>> 
>>> 
>>> But in case of other providers(BouncyCastleProvider ) host will not get set 
>>> and "java.security.cert.CertificateException: No subject alternative name 
>>> found matching IP address" exception will be thrown.
>>>
>
>> > HttpsURLConnection, works with SunJSSE provider but does not work with 
>> > other JSSE provider. In case of SunJSSE , HttpsURLConnection set the host 
>> > name as follows
>> > s = (SSLSocket)serverSocket;
>> > if (s instanceof SSLSocketImpl) {
>> > ((SSLSocketImpl)s).setHost(host);
>> > }
>> 
>> Did you copy the code/lines above from 
>> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java? 
>> This is the file you updated in the pull request. If I read the updated file 
>> right, the line numbers are from 455 to 458. If the socket is an instance of 
>> SSLSocketImpl, the host is set here.
>> 
>> > But in case of other providers(BouncyCastleProvider ) host will not get 
>> > set and "java.security.cert.CertificateException: No subject alternative 
>> > name found matching IP address" exception will be thrown.
> 
> thanks for  pointing me out.  yes you are right in case of SSLSocketImpl host 
> will set twice. I will do the changes and update the PR.

update the PR  and incorporated the review comment suggested by Xuelei.

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider.

[Vyom Tewari]

On Wed, 3 Mar 2021 04:38:47 GMT, Xue-Lei Andrew Fan  wrote:

> > HttpsURLConnection, works with SunJSSE provider but does not work with 
> > other JSSE provider. In case of SunJSSE , HttpsURLConnection set the host 
> > name as follows
> > s = (SSLSocket)serverSocket;
> > if (s instanceof SSLSocketImpl) {
> > ((SSLSocketImpl)s).setHost(host);
> > }
> 
> Did you copy the code/lines above from 
> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java? This 
> is the file you updated in the pull request. If I read the updated file 
> right, the line numbers are from 455 to 458. If the socket is an instance of 
> SSLSocketImpl, the host is set here.
> 
> > But in case of other providers(BouncyCastleProvider ) host will not get set 
> > and "java.security.cert.CertificateException: No subject alternative name 
> > found matching IP address" exception will be thrown.

thanks for  pointing me out.  yes you are right in case of SSLSocketImpl host 
will set twice. I will do the changes and update the PR.

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v2]

[Vyom Tewari]

On Tue, 2 Mar 2021 20:17:25 GMT, Xue-Lei Andrew Fan  wrote:

>> Vyom Tewari has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   used List.of instead of Collections.singletonList
>
> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line 
> 569:
> 
>> 567: SSLParameters paramaters = s.getSSLParameters();
>> 568: 
>> paramaters.setEndpointIdentificationAlgorithm("HTTPS");
>> 569: 
>> paramaters.setServerNames(Collections.singletonList(new SNIHostName(host)));
> 
> List.of() is more preferred now.
> 
> Is the code in line 465 and 467 duplicated now?

used List.of(), but i a not able to follow line 465 and 467. Are we are looking 
same code base repo?

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider. [v2]

[Vyom Tewari]

> HttpsURLConnection, works with SunJSSE provider but does not work with other 
> JSSE provider. In case of SunJSSE , HttpsURLConnection set the host name as 
> follows
> 
> s = (SSLSocket)serverSocket;
>if (s instanceof SSLSocketImpl) {
>   ((SSLSocketImpl)s).setHost(host);
>} 
> 
> But in case of other providers(BouncyCastleProvider )  host will not get set 
> and "java.security.cert.CertificateException: No subject alternative name 
> found matching IP address" exception will be thrown.

Vyom Tewari has updated the pull request incrementally with one additional 
commit since the last revision:

  used List.of instead of Collections.singletonList

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2583/files
  - new: https://git.openjdk.java.net/jdk/pull/2583/files/5d2c05af..2952f596

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2583=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2583=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2583.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2583/head:pull/2583

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider.

[Vyom Tewari]

On Tue, 2 Mar 2021 15:56:35 GMT, Daniel Fuchs  wrote:

>> i ran tests, it looks 
>> clean(https://github.com/vyommani/jdk/actions/runs/612949667)
>
> As far as I know this is only tier1 - so none of the network tests have been 
> run.

I ran tier1 & tier2 tests locally on my linux vm, it was clear. Can you please 
do let me know how to run it on OpenJDK build system.

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider.

[Vyom Tewari]

On Tue, 2 Mar 2021 15:03:47 GMT, Daniel Fuchs  wrote:

>> May be i am not sure, we may need this code change to review by security 
>> expert. I am setting "SNIHostName" only if "isDefaultHostnameVerifier" is 
>> true(If the HNV is the default from HttpsURLConnection) so there should not 
>> be problem.
>
> Did you try to run the httpclient tests? They make use of the httpserver - so 
> they can also serve to test it - somewhat.
> More generally - please run jdk_net/tier2 tests.

i ran tests, it looks 
clean(https://github.com/vyommani/jdk/actions/runs/612949667)

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider.

[Vyom Tewari]

On Tue, 2 Mar 2021 12:43:27 GMT, Daniel Fuchs  wrote:

>> HttpsURLConnection, works with SunJSSE provider but does not work with other 
>> JSSE provider. In case of SunJSSE , HttpsURLConnection set the host name as 
>> follows
>> 
>> s = (SSLSocket)serverSocket;
>>if (s instanceof SSLSocketImpl) {
>>   ((SSLSocketImpl)s).setHost(host);
>>} 
>> 
>> But in case of other providers(BouncyCastleProvider )  host will not get set 
>> and "java.security.cert.CertificateException: No subject alternative name 
>> found matching IP address" exception will be thrown.
>
> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line 
> 569:
> 
>> 567: SSLParameters paramaters = s.getSSLParameters();
>> 568: 
>> paramaters.setEndpointIdentificationAlgorithm("HTTPS");
>> 569: 
>> paramaters.setServerNames(Collections.singletonList(new SNIHostName(host)));
> 
> What if the SSL Layer has already an SNIHostName configured? Is there a risk 
> that this will introduce regressions in such cases?

May be i am not sure, we may need this code change to review by security 
expert. I am setting "SNIHostName" only if "isDefaultHostnameVerifier" is 
true(If the HNV is the default from HttpsURLConnection) so there should not be 
problem.

-

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR: 8252831: Correct "no comment" warnings in jdk.net module

[Vyom Tewari]

On Tue, 2 Mar 2021 10:15:13 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my changeset that fixes the "no comment" warnings 
> generated by `javadoc -Xdoclint` for `java.base/jdk.net`?
> 
> Kind regards,
> Patrick

LGTM

-

Marked as reviewed by vtewari (Committer).

PR: https://git.openjdk.java.net/jdk/pull/2792

RFR: JDK-8260925: HttpsURLConnection does not work with other JSSE provider.

[Vyom Tewari]

HttpsURLConnection, works with SunJSSE provider but does not work with other 
JSSE provider. In case of SunJSSE , HttpsURLConnection set the host name as 
follows

s = (SSLSocket)serverSocket;
   if (s instanceof SSLSocketImpl) {
  ((SSLSocketImpl)s).setHost(host);
   } 

But in case of other providers(BouncyCastleProvider )  host will not get set 
and "java.security.cert.CertificateException: No subject alternative name found 
matching IP address" exception will be thrown.

-

Commit messages:
 - fixed jcheck failure
 - JDK-8260925: HttpsURLConnection does not work  with other JSSE provider.

Changes: https://git.openjdk.java.net/jdk/pull/2583/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2583=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260925
  Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2583.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2583/head:pull/2583

PR: https://git.openjdk.java.net/jdk/pull/2583

Re: RFR:8046500 GetIpAddrTable function failed on Pure Ipv6 environment

[vyom tewari]

Hi Chris,

thanks for review, you are right, we should clear the pending exception 
if -2 is return. Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8046500/webrev0.2/index.html).


Thanks,

Vyom


On 15/11/18 9:57 PM, Chris Hegarty wrote:

Vyom,


On 13 Nov 2018, at 12:35, vyom tewari  wrote:

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8046500/webrev0.1/index.html) where 
i incorporated your suggestion. Now i am returning the different error code(-2) 
if IP Helper Library function GetIfTable fails. I checked the all other call 
sites and we don't need to do any additional changes.

This looks much better to me. One question …

Looking at `createNetworkInterfaceXP` I wonder if its usage of
`enumAddresses_win` should clear the pending exception if
-2 is returned?

-Chris.

Re: RFR:8046500 GetIpAddrTable function failed on Pure Ipv6 environment

[vyom tewari]

reseeding again,  looks like some problem with my email client.

Vyom

On 13/11/18 6:05 PM, vyom tewari wrote:

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8046500/webrev0.1/index.html) 
where i incorporated your suggestion. Now i am returning the different 
error code(-2) if IP Helper Library function GetIfTable fails. I 
checked the all other call sites and we don't need to do any 
additional changes.


Thanks,

Vyom

On 08/11/18 6:19 PM, Chris Hegarty wrote:

Hi Vyom,

On 08/11/18 09:22, vyom tewari wrote:

Hi,

Please review the below code change.

Webrev: 
http://cr.openjdk.java.net/~vtewari/8046500/webrev0.0/index.html


enumInterfaces may return -1 for error conditions other than
IP Helper Library GetIfTable function failed. I wonder if this
specific failure should have its own separate return value,
maybe -2, so as to not be confused with other error conditions?

Similarly enumAddresses_win.

Of course all call sites of these functions will need to be
checked if this is done, but there are not that many.

-Chris

Re: RFR:8046500 GetIpAddrTable function failed on Pure Ipv6 environment

[vyom tewari]

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8046500/webrev0.1/index.html) 
where i incorporated your suggestion. Now i am returning the different 
error code(-2) if IP Helper Library function GetIfTable fails. I checked 
the all other call sites and we don't need to do any additional changes.


Thanks,

Vyom

On 08/11/18 6:19 PM, Chris Hegarty wrote:

Hi Vyom,

On 08/11/18 09:22, vyom tewari wrote:

Hi,

Please review the below code change.

Webrev: http://cr.openjdk.java.net/~vtewari/8046500/webrev0.0/index.html


enumInterfaces may return -1 for error conditions other than
IP Helper Library GetIfTable function failed. I wonder if this
specific failure should have its own separate return value,
maybe -2, so as to not be confused with other error conditions?

Similarly enumAddresses_win.

Of course all call sites of these functions will need to be
checked if this is done, but there are not that many.

-Chris

RFR:8046500 GetIpAddrTable function failed on Pure Ipv6 environment

[vyom tewari]

Hi,

Please review the below code change.

Webrev: http://cr.openjdk.java.net/~vtewari/8046500/webrev0.0/index.html

BugId:  https://bugs.openjdk.java.net/browse/JDK-8046500

This issue is specific to pure ipv6 environment, In pure ipv6 
environment  if "enumInterface" fails,  i am clearing the exception and 
continue processing the IPv6 interface configurations.


I tested this issue locally, by uninstalling IPv4(netsh interface ipv4 
uninstall) on my Windows 10 laptop.


Thanks,

Vyom

RFR:8212114 Reconsider the affect on closed streams resulting from 8189366

[vyom tewari]

Hi All,

Please review the below fix.

Webrev: http://cr.openjdk.java.net/~vtewari/8212114/webrev0.0/index.html

bugId: https://bugs.openjdk.java.net/browse/JDK-8212114

this change the revert the effect of JDK-8189366,  restore the 
existing(SocketInputStream:available() will throw the socketException if 
the underline socket is closed.) long-standing behavior.


Thanks,

Vyom

Re: RFR: 8189366: SocketInputStream.available() should check for eof

[vyom tewari]

sure, i will send the webrev  soon.

Vyom


On Wednesday 17 October 2018 04:31 PM, Chris Hegarty wrote:

On 12/10/18 12:58, Daniel Fuchs wrote:

...

 int available = impl.available();
 return eof ? 0 : available;

addresses the issue of available potentially returning garbage
after EOF while being much less risky...


Agreed.

The above resolves the original reported potential bug,
without changing existing long-standing behavior.

Vyom, can you please proceed with the aboves source
change, and also update the existing CloseAvailable.java
to add additional coverage for this scenario.

Thanks,
-Chris.

Re: RFR: 8189366: SocketInputStream.available() should check for eof

[vyom tewari]

On Friday 12 October 2018 04:31 PM, Chris Hegarty wrote:

Daniel,

On 12/10/18 11:26, Daniel Fuchs wrote:

Hi,

Maybe a more conservative implementation could have been:

    int available = impl.available();
    return eof ? 0 : available;


That buys us little more than we had prior to this change,
since impl.available will still call into native before
checking the EOF status.

If we want to keep this, then we need:

    public int available() throws IOException {
    if (impl.isClosedOrPending()) {
    throw new SocketException("Socket closed");
    }

    if (eof) {
    return 0;
    } else {
    return impl.available();
    }
    }

Above change is working as expected, i  modified the "CloseAvailable"  
test little bit and with modified code, SocketInputstream.available() is 
now throwing "java.net.SocketException: Socket closed" exception if the 
underline socket is closed.


I will prepare a patch along with modified test and send it for review.

I ran our internal tests and it looks like  it was not caught there.

Thanks,
Vyom


I almost suggested that yesterday, but I saw that
read() already had a logic similar to what Vyom was
proposing for available:

  146 // EOF already encountered
  147 if (eof) {
  148 return -1;
  149 }

which AFAICT  means that read returns -1 instead of throwing
if the socket is closed and EOF was previously reached.


That is correct. While not intuitive, I don't propose
that we change this. ( if this were a new implementation
then I think it should throw IOE for this scenario, but
we are where we are ).

-Chris.



best regards,

-- daniel

On 12/10/2018 09:55, Chris Hegarty wrote:


On 12/10/18 08:29, Alan Bateman wrote:

On 11/10/2018 09:03, vyom tewari wrote:

Hi Chris,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8189366/webrev0.1/index.html) 
where i included the test.
Can you explain the behavior change for the closed socket case? 
Will this change mean that available returns 0 when it previously 
throw IOException?


You are correct. This is not intentional, or desirable.

We should revert the change or add an additional check
for isClosedOrPending. Since this is already pushed, I
filed a new JIRA issue to track this.

   https://bugs.openjdk.java.net/browse/JDK-8212114

-Chris.

Re: [12] RFR 8187522: sun/net/ftp/FtpURLConnectionLeak.Java timed out

[vyom tewari]

On Thursday 11 October 2018 02:15 PM, Chris Yin wrote:
Please review below small change to fix test 
sun/net/ftp/FtpURLConnectionLeak.Java, thanks


Besides the original timeout issue, looks like the test not working as 
expected even the results is pass. Per test description, we expect 
FileNotFoundException and then verify connection closed, but look 
through recent test run log, it’s actually got 
“sun.net.ftp.FtpLoginException: Invalid username/password” and this 
exception been caught in IOException block coincidentally, so the test 
result is pass, but it never hit FileNotFoundException. The fix change 
will remove IOException catch block and close server socket in close() 
of try-with-resouce to avoid possible invalid exception catch and 
connection issue, also ftp 220 response message was modified to allow 
FtpClient working correctly (I leave the corrupted message style as it 
is since looks like it will be used to test JDK-8151586)
you are right, corrupted message style is intentional. Your code changes 
looks good to me but i am not able to understand 
"FtpCommandHandler.java" changes.


 I can see that you inserted one "-" , is it intentional ?

Thanks,
Vyom


bug: https://bugs.openjdk.java.net/browse/JDK-8187522
webrev: http://cr.openjdk.java.net/~xyin/8187522/webrev.00/

Regards,
Chris

Re: RFR: 8189366: SocketInputStream.available() should check for eof

[vyom tewari]

Hi Chris,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8189366/webrev0.1/index.html) 
where i included the test.


Thanks,

Vyom


On Wednesday 10 October 2018 09:50 PM, Chris Hegarty wrote:

Vyom,

On 10/10/18 14:16, vyom tewari wrote:

Hi All,

Please provide your review comments on below fix.

Thanks,

Vyom


On Friday 27 October 2017 11:31 AM, vyom tewari wrote:


Wow, is it really almost a year since this was first
discussed!


 ...
http://cr.openjdk.java.net/~vtewari/8189366/webrev0.0/index.html 


To satisfy myself, I put together a test, ran it through
Oracle's build and test infra, both with and without your
proposed changes, and it passes ok. This should give more
confidence that there is no inadvertant behavior change
resulting from your changes.

http://cr.openjdk.java.net/~chegar/8189366.test/

Can you please include this test as part of your push?

Thanks,
-Chris.

P.S. The test amends an existing test with additional
coverage, without interfering too much with the original
test.

Re: RFR: 8189366: SocketInputStream.available() should check for eof

[vyom tewari]

Hi All,

Please provide your review comments on below fix.

Thanks,

Vyom


On Friday 27 October 2017 11:31 AM, vyom tewari wrote:




On Thursday 26 October 2017 03:14 PM, Bernd Eckenfels wrote:
What is currently returned at the end of a stream? This looks like a 
dangerous thing to do, if a existing implementation only
Currently it returns 0 at end of stream and  same as after change. As 
David pointed out that ultimately it delegates on to "ioctl", i 
checked the doc(http://man7.org/linux/man-pages/man4/tty_ioctl.4.html) 
and did not found anything which tells about eof.


What i found out, setting eof at socketinputstream there is no effect 
on native  "ioctl" call. I set the "eof"  and 
SocketInputStream.available() return 0.


Let's wait for other people opinions.

Note: you have to shutdown the SocketInputstream to set "eof", i am 
not sure if there is any other way to set "eof" for SocketInputStream.


Thanks,
Vyom

read when something is available it might never detect that it 
reached EOF.


Gruss
Bernd
--
http://bernd.eckenfels.net
--------
*From:* net-dev  on behalf of vyom 
tewari 

*Sent:* Thursday, October 26, 2017 11:26:15 AM
*To:* OpenJDK Network Dev list
*Subject:* RFR: 8189366: SocketInputStream.available() should check 
for eof

Hi All,

Please review the simple change below.

Webrev   : 
http://cr.openjdk.java.net/~vtewari/8189366/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8189366/webrev0.0/index.html>


BugId  : https://bugs.openjdk.java.net/browse/JDK-8189366


Currently SocketInputStream.available() does not check for "eof" and
simply delegate to the impl even when "eof" reached. I put a check  to
return 0 if "eof" is already reached.

Thanks,

Vyom

Re: [openjdk-x] Bind to a multicast address fails

[vyom tewari]

Hi Alan,Chris,

I imported the Andre's patch and our all existing internal tests are clean.

Thanks,

Vyom


On Monday 10 September 2018 10:58 PM, Chris Hegarty wrote:


On 10/09/18 15:35, Chris Hegarty wrote:

...

Maybe the native implementation of these methods can be
updated to accept a scope, rather than poking around in
the InetSocketAddress from JNI code.


Rolling back on this comment. I overlooked the fact that
the additional checks are multicast-specific,
IN6_IS_ADDR_MC_NODELOCAL and IN6_IS_ADDR_MC_LINKLOCAL.
Given this, I am less concerned about the proposed patch.

-Chris.

Re: [openjdk-x] Bind to a multicast address fails

[vyom tewari]

On Monday 10 September 2018 12:28 PM, Andre Naujoks wrote:

On 9/9/18 9:16 AM, Alan Bateman wrote:

On 07/09/2018 13:22, Andre Naujoks wrote:

:
I have not tried joining IPv4 groups on an IPv6 socket through java,
since we do not use IPv4 at all in this particular environment. I have
tried setting IP_MULTICAST_ALL to 0 in the IPv6 scenario (in a C++
project), hoping it would help, but it did not. Hence the patch for the
linux kernel.

Would it actually help, if I tried the IPv4 multicast group bind on an
IPv6 socket?

The bind to an address would be a workaround for the missing
IPV6_MULTICAST_ALL handling.


The tests that we have for the scenario of two sockets bound to the same
port but joining different multicast groups seems to be mostly using
IPv4 multicast addresses so one thing out this discussion is that we may
need to expand the tests to IPv6 multicast addresses. As the existing
tests use IPv6 sockets (when not disabled on the system or in the test
run) then it means they are exercising IP_MULTICAST_ALL=0 so I think we
can conclude that disabling IP_MULTICAST_ALL works correctly for IPv6
sockets when joining IPv4 multicast groups. If the tests were expanded
to IPv6 multicast groups then I assume we will run into the need for
IPV6_MULTICAST_ALL too.

Yes, I would assume the same. That was the whole reason for the kernel
patch.


As regards the patch to NET_InetAddressToSockaddr to set the scope_id
then it looks correct but need testing (both for bind and connect). I
see JDK-8210493 has been picked up by Vyom.

Please test it! This is my very first look into the java code-base. The
patch does what it says, but I cannot rule out, that it doesn't have any
unintended side effects.

Hi Andre,

i will apply your patch run our all network tests. I will update once i 
will have result.


Thanks,
Vyom



-Alan

Re: RFR: 8172346 sun.net.ftp.FtpDirEntry.setCreated(Date) may expose internal represent

[vyom tewari]

Hi Chris,

Thanks for review, please find my comment inline.

Thanks,

Vyom


On Tuesday 24 July 2018 06:54 PM, Chris Hegarty wrote:

On 24 Jul 2018, at 10:55, vyom tewari  wrote:

Hi All,

Please review below a trivial fix.

Webrev : http://cr.openjdk.java.net/~vtewari/8172346/webrev0.0/index.html

BugID: https://bugs.openjdk.java.net/browse/JDK-8172346

The above code change will avoid storing the externally mutable object 
reference into FtpDirEntry.

Is this defensive coding really needed here, given that the
class is internal and only ever used by the FTP URL protocol
handler?
It is good to have but i agree with you as class is internal and part of 
non-exported package so it is not really needed.

If it is not really needed, then maybe we just close the bug as
‘not an issue’.
-Chris.

RFR: 8172346 sun.net.ftp.FtpDirEntry.setCreated(Date) may expose internal represent

[vyom tewari]

Hi All,

Please review below a trivial fix.

Webrev : http://cr.openjdk.java.net/~vtewari/8172346/webrev0.0/index.html

BugID: https://bugs.openjdk.java.net/browse/JDK-8172346

The above code change will avoid storing the externally mutable object 
reference into FtpDirEntry.


Thanks,

Vyom

Re: RFR : 8205959 : Do not restart close if errno is EINTR

[vyom tewari]

Hi Ivan,

code changes looks good to me, although i am not the official reviewer.

Thanks,

Vyom


On Thursday 28 June 2018 04:45 AM, Ivan Gerasimov wrote:

Hello!

When closing a socket via NET_SocketClose(int fd), a close(fd) is called.
The later is wrapped in a retry-loop, which is wrong because close() 
is not restartable.


The `man 2 close` states:
"""
... close() should not be retried after an EINTR since this may cause 
a reused descriptor from another thread to be closed.

"""

Would you please help review a trivial fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8205959
WEBREV: http://cr.openjdk.java.net/~igerasim/8205959/00/webrev/

Thanks in advance!

Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes

[vyom tewari]

On Friday 25 May 2018 11:19 AM, Ivan Gerasimov wrote:

Hi Wiijun!


On 5/24/18 10:13 PM, Weijun Wang wrote:


On May 25, 2018, at 11:58 AM, Ivan Gerasimov 
 wrote:


I also wonder whether a smart compiler might not flag code where 
the errors do infact have the same value:


if (errno == 11 || errno == 11) ...

At least gcc -O completely removes the second redundant test, so no 
observable changes is expected on supported platforms.
And it silently compiles without showing any warning, right? Good if 
yes.


--Max


Yep, all is good.
I've built/tested the patched JDK on all supported platforms with no 
issues.


And we already have places, where both EAGAIN and EWOULDBLOCK are used 
in one if clause (as I just replied to David):
java.base/unix/native/libnet/SocketInputStream.c, in 
NET_ReadWithTimeout():

    result = NET_NonBlockingRead(fd, bufP, len);
    if (result == -1 && ((errno == EAGAIN) || (errno == 
EWOULDBLOCK))) {





i  wrote this code and that time even i wanted to fix the other native 
code (check error code for both EAGAIN & EWOULDBLOCK)  as you did ,but i 
did not .

So the fix basically proposes to use this approach consistently.

we can at least fix the native part of code.

Vyom



With kind regards,
Ivan

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.6/index.html). 
I renamed the macosx files to 
"MacOSXSocketOptions.java".


Our internal tests are clean.

Thanks,
Vyom

On Tuesday 15 May 2018 03:13 PM, Alan Bateman wrote:

On 15/05/2018 08:35, Langer, Christoph wrote:


I’m asking because I’m planning to add some AIX options and will have 
to choose a name for this implementation eventually.


@Alan: What do you think?


Yes, I agree it should be renamed. Vyom has just finalized the CSR so 
I assume the final points around the implementation and naming of the 
JDK internal classes can be sorted out while the CSR is in progress.


-Alan.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

On Tuesday 15 May 2018 03:13 PM, Alan Bateman wrote:

On 15/05/2018 08:35, Langer, Christoph wrote:


I’m asking because I’m planning to add some AIX options and will have 
to choose a name for this implementation eventually.


@Alan: What do you think?


Yes, I agree it should be renamed. Vyom has just finalized the CSR so 
I assume the final points around the implementation and naming of the 
JDK internal classes can be sorted out while the CSR is in progress.


sure, i will rename the files to 
MacOSXSocketOptions.java I will send the modified 
webrev soon. Please do let me know if any buddy have better name 
suggestion than this.


Thanks,
vyom

-Alan.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi All,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.5/index.html). 
Only change with the previous wrev(04) is i removed "socket type" as 
Alan suggested and used the default  constructor (Set<SocketOption> 
options = new HashSet<>();) in ExtendedSocketOptions.java


Thanks,

Vyom


On Sunday 13 May 2018 12:37 PM, Alan Bateman wrote:

On 12/05/2018 10:21, vyom tewari wrote:

:

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html).

I've skimmed through this webrev.

The spec for the new options mostly look good but all three include 
"The exact semantics of this socket option are socket type and system 
dependent". I assume "socket type" should be dropped from this as 
these are TCP options. Maybe you can borrow text from the TCP_NODELAY 
option where it has the statement "The socket option is specific to 
stream-oriented sockets using the TCP/IP protocol".


The changes to the NetworkChannel implementations look okay. The 
changes to the NIO tests looks okay too but would be good if you could 
keep the formatting consistent with the existing code if you can.


Ivan had good comments so I didn't spent as much time on that code and 
it seems very reviewed.


-Alan.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

On Saturday 12 May 2018 04:14 PM, Ivan Gerasimov wrote:

Thanks Vyom!

I like it much better now.

The last minorish comment:

src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Set<SocketOption> options = new HashSet<>(5);

Please note that the constructor of HashSet wants the initial capacity 
as the argument, not the expected number of elements.
So in this case it would be more accurate to have (7), so that 7 * 
0.75 = 5.25 > 5.
Practically, it wouldn't make any difference, as both 5 and 7 would be 
rounded up to 8 anyways.


thanks for detail explanation :) , i did not saw the code but i was 
under impression that it will use size as the nearest prime number for 
uniform distribution of elements in general.


However, I would recommend using the default constructor just to avoid 
confusion.
Or, alternatively, collect the options to an ArrayList of desired 
capacity and then make unmodifiable set with Set.copyOf(list).


No further comments from my side :)
if no further comment from others as well, i will change it to use 
default constructor at time of pushing.


Thanks,
Vyom



With kind regards,
Ivan


On 5/12/18 2:21 AM, vyom tewari wrote:

Hi Ivan,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html). 
Please find my answers in-line.


Thanks,

vyom


On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:

Hi Vyom!

1)
src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java

Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then 
Collectors.toUnmodifiableSet could be used for convenience:


return options.stream()
    .filter((option) -> !option.name().startsWith("TCP_"))
    .collect(Collectors.toUnmodifiableSet());

Also, I think Alan suggested to filter out UDP_* options for 
SOCK_STREAM here.

fixed


2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java

If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other 
files, then the line 80 wouldn't become too long.


The same applies to 
src/java.base/unix/classes/java/net/PlainSocketImpl.java

fixed


3)
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java

Was it intentional that when flowSupported == true, 
quickAckSupported == false, keepAliveOptSupported == true, the 
TCP_KEEP* options are *not* added to the resulting set?


If not, then can this set population be organized in more linear 
way:  Just create an ArrayList, conditionally fill it in and return 
unmodifiable set with Set.of(list.toArray()).
of course not, i  don't know but i always prefer complex nested 
"if-else" then linear ifs :)  fixed as well.


Nit:  please place the equal sign at line 172 consistently with the 
other two inits above.

fixed.


With kind regards,
Ivan


On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:
On 11 May 2018, at 01:04, Alan Bateman <alan.bate...@oracle.com> 
wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html) 


...
It would be better if the channel implementation didn't static 
import ExtendedSocketOptions.getInstance as that is a very 
generic method method name. As I mentioned previously, you could 
simplify all these usages if you add the following to 
sun.net.ext.ExtendedSocketOption
    static Set<SocketOption> options(int type) { return 
getInstance().options(type)); }

+1

A minor comment on tests is that they can use List.of instead of 
Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that 
we should deprecate-for-removal jdk/net/Sockets.java. It’s 
functionality is already supported by a standard API.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi Ivan,

Thanks for review, please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html). 
Please find my answers in-line.


Thanks,

vyom


On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:

Hi Vyom!

1)
src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java

Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then 
Collectors.toUnmodifiableSet could be used for convenience:


return options.stream()
    .filter((option) -> !option.name().startsWith("TCP_"))
    .collect(Collectors.toUnmodifiableSet());

Also, I think Alan suggested to filter out UDP_* options for 
SOCK_STREAM here.

fixed


2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java

If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other 
files, then the line 80 wouldn't become too long.


The same applies to 
src/java.base/unix/classes/java/net/PlainSocketImpl.java

fixed


3)
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java

Was it intentional that when flowSupported == true, quickAckSupported 
== false, keepAliveOptSupported == true, the TCP_KEEP* options are 
*not* added to the resulting set?


If not, then can this set population be organized in more linear way:  
Just create an ArrayList, conditionally fill it in and return 
unmodifiable set with Set.of(list.toArray()).
of course not, i  don't know but i always prefer complex nested 
"if-else" then linear ifs :)  fixed as well.


Nit:  please place the equal sign at line 172 consistently with the 
other two inits above.

fixed.


With kind regards,
Ivan


On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:

On 11 May 2018, at 01:04, Alan Bateman <alan.bate...@oracle.com> wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html) 


...
It would be better if the channel implementation didn't static 
import ExtendedSocketOptions.getInstance as that is a very generic 
method method name. As I mentioned previously, you could simplify 
all these usages if you add the following to 
sun.net.ext.ExtendedSocketOption
    static Set<SocketOption> options(int type) { return 
getInstance().options(type)); }

+1

A minor comment on tests is that they can use List.of instead of 
Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that 
we should deprecate-for-removal jdk/net/Sockets.java. It’s 
functionality is already supported by a standard API.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

thanks all for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html). 
I address most of the  review comments.


Vyom


On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:

On 11 May 2018, at 01:04, Alan Bateman <alan.bate...@oracle.com> wrote:

On 10/05/2018 16:21, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html)
...

It would be better if the channel implementation didn't static import 
ExtendedSocketOptions.getInstance as that is a very generic method method name. 
As I mentioned previously, you could simplify all these usages if you add the 
following to sun.net.ext.ExtendedSocketOption
static Set<SocketOption> options(int type) { return 
getInstance().options(type)); }

+1


A minor comment on tests is that they can use List.of instead of Arrays.asList.

+1

Otherwise, this looks very good.

-Chris.

P.S. A separate issue, but when reviewing this it reminded me that we should 
deprecate-for-removal jdk/net/Sockets.java. It’s functionality is already 
supported by a standard API.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html). 
I incorporated most of the review comments. Chris as you suggested in 
below mail i did not added the note for upper-bound because values are 
also OS specific.


Thanks,

Vyom


On Monday 23 April 2018 07:26 PM, Chris Hegarty wrote:


On 23/04/18 13:34, Alan Bateman wrote:

On 23/04/2018 13:05, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
I incorporated  most of the review comments.
This looks much better but I think the second paragraph of the spec 
of each option needs to be inverted so that it states clearly what 
the options does before it gets into the background of SO_KEEPALIVE. 
For example, TCP_KEEPALIVE should say that it sets the idle period 
for keep alive before it explains SO_KEEPALIVE. 


Mea culpa, I ordered the paragraphs as they are to be consistent
with TCP_QUICKACK. I don't have any objection if they are reversed.

The currently wording also begs questions as to whether the socket 
option means anything when SO_KEEPALIVE is not enabled.


It's implicit. The option can still be set and its value retrieved.


Also it probably should say something about whether it can be changed 
at any time or not.


You can set it any time. Of course, it may not be effective
immediately, depending on the exact state of the socket.

We may also want to add a note that the accepted values may
have an upper-bound. For example, the following is the largest
set of values that I can set on my Ubuntu Linux, without an
exception being thrown [*].

 TCP_KEEPIDLE = 32767
 TCP_KEEPINTERVAL = 32767
 TCP_KEEPCOUNT = 127

-Chris.

[*] "java.net.SocketException: Invalid argument" when a given
value is "too" large.

Re: RFR 8202558 : Access to freed memory in java.base/unix/native/libnet/net_util_md.c

[vyom tewari]

Hi Ivan,

the url(http://cr.openjdk.java.net/~igerasim/8202558/00/webrev/ ) is not 
accessible to me.


Thanks,

Vyom


On Thursday 03 May 2018 01:51 AM, Ivan Gerasimov wrote:
I just realized that we've got a similar issue in the same file in the 
function initLocalIfs().


The webrev was updated in place to fix this issue as well.

With kind regards,

Ivan


On 5/2/18 12:54 PM, Ivan Gerasimov wrote:

Hello!

The function needsLoopbackRoute() calls initLoopbackRoutes() to 
initialize two global variables: loRoutes and nRoutes.


If realloc() fails at line 582 then loRoutes is freed, but nRoutes is 
left positive.


Then, in needsLoopbackRoute() the already freed memory will be accessed.

Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202558
WEBREV: http://cr.openjdk.java.net/~igerasim/8202558/00/webrev/

Thanks!

Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in Unix Inet*AddressImpl_getLocalHostName implementations

[vyom tewari]

On Friday 27 April 2018 01:05 PM, Langer, Christoph wrote:

Hi all,

thanks for looking into this. Here are a few comments

First of all, there are no real life issues I have seen with this. It is just 
something that occurred to me when working with the code. But, why not fix it, 
even it is a corner case that might never happen.

@Thomas: As for the zero termination of the hostname result after the call to 
gethostname: Yes, we should unconditionally terminate the result, which we do. 
Unfortunately this part of code cannot be moved outside the solaris #ifdef 
because the part in the #ifdef contains variable declarations. And you know - 
the C compatibility issue...

I looked again into the macro definitions for for HOST_NAME_MAX and NI_MAXHOST. 
HOST_NAME_MAX is mentioned in the gethostname docs ([1] and [2]). Glibc docs 
indicate it is 64 Byte or 255 Byte. So it looks like it is a quite small 
buffer, compared to NI_MAXHOST from netdb.h, which is the value that 
getnameinfo likes to work with, as per [3]. Posix genameinfo doc ([4]) does not 
mention NI_MAXHOST but Linux doc says it is 1025 which is what we'd define if 
it is not set.

Taking this input I have updated my webrev to round things up a little bit: 
http://cr.openjdk.java.net/~clanger/webrevs/8202181.1/

it looks good to me, although i am not official reviewer.

I moved the definition of NI_MAXHOST into net_util_md.h and updated the comment 
a little bit to make clearer why it is there. In Inet4AddressImpl.c and 
Inet6AddressImpl.c I also fixed the other places where getnameinfo is called to 
use sizeof(buffer) instead of NI_MAXHOST.

good cleanup, comment will definitely help.

Thanks,
Vyom


Best regards
Christoph

[1] http://man7.org/linux/man-pages/man2/gethostname.2.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/gethostname.html
[3] http://man7.org/linux/man-pages/man3/getnameinfo.3.html
[4] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getnameinfo.html



-Original Message-
From: vyom tewari [mailto:vyom.tew...@oracle.com]
Sent: Freitag, 27. April 2018 08:38
To: Thomas Stüfe <thomas.stu...@gmail.com>
Cc: Langer, Christoph <christoph.lan...@sap.com>; net-
d...@openjdk.java.net
Subject: Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in
Unix Inet*AddressImpl_getLocalHostName implementations



On Friday 27 April 2018 10:58 AM, Thomas Stüfe wrote:

On Fri, Apr 27, 2018 at 5:57 AM, vyom tewari <vyom.tew...@oracle.com>

wrote:

Hi Christoph,


On Tuesday 24 April 2018 04:45 PM, Langer, Christoph wrote:

Hi Vyom,

I think, it is intentional to handle case where return "hostname" is to
large to
fit in  array.  if you see the man page(http://man7.org/linux/man-
pages/man2/gethostname.2.html) it says that it is unspecified whether
returned buffer includes a terminating null byte.

current code will put null in case of large "hostname", What do you think ?

yes, I had read the man page and saw this point of the spec. But exactly

for

this purpose there's this code:

// make sure string is null-terminated
hostname[NI_MAXHOST] = '\0';

If we only hand 'NI_MAXHOST' as size value into gethostname, then the
function might only write NI_MAXHOST - 1 characters of the hostname

into the

buffer.

doc says it will copy len bytes into buffer and will not copy null character
into the buffer.



C library/kernel differences
 The GNU C library does not employ the gethostname() system call;
 instead, it implements gethostname() as a library function that calls
 uname(2) and copies up to len bytes from the returned nodename

field

 into name.  Having performed the copy, the function then checks if
 the length of the nodename was greater than or equal to len, and if
 it is, then the function returns -1 with errno set to ENAMETOOLONG;
 in this case, a terminating null byte is not included in the returned
 name.


##
##

This is shared code, so we should refer to Posix, not linux specific man

pages.



http://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname
.html



DESCRIPTION

The gethostname() function shall return the standard host name for the
current machine. The namelen argument shall specify the size of the
array pointed to by the name argument. The returned name shall be
null-terminated, except that if namelen is an insufficient length to
hold the host name, then the returned name shall be truncated and it
is unspecified whether the returned name is null-terminated.

Host names are limited to {HOST_NAME_MAX} bytes.

RETURN VALUE

Upon successful completion, 0 shall be returned; otherwise, -1 shall
be returned.



Note that there is no indication what happens if the buffer is too
small. It may zero-terminate, it may not. It may return an error, it
may not. Decision is left to the platform imp

Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in Unix Inet*AddressImpl_getLocalHostName implementations

[vyom tewari]

On Friday 27 April 2018 10:58 AM, Thomas Stüfe wrote:

On Fri, Apr 27, 2018 at 5:57 AM, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Christoph,


On Tuesday 24 April 2018 04:45 PM, Langer, Christoph wrote:

Hi Vyom,

I think, it is intentional to handle case where return "hostname" is to
large to
fit in  array.  if you see the man page(http://man7.org/linux/man-
pages/man2/gethostname.2.html) it says that it is unspecified whether
returned buffer includes a terminating null byte.

current code will put null in case of large "hostname", What do you think ?

yes, I had read the man page and saw this point of the spec. But exactly for
this purpose there's this code:

// make sure string is null-terminated
hostname[NI_MAXHOST] = '\0';

If we only hand 'NI_MAXHOST' as size value into gethostname, then the
function might only write NI_MAXHOST - 1 characters of the hostname into the
buffer.

doc says it will copy len bytes into buffer and will not copy null character
into the buffer.



C library/kernel differences
The GNU C library does not employ the gethostname() system call;
instead, it implements gethostname() as a library function that calls
uname(2) and copies up to len bytes from the returned nodename field
into name.  Having performed the copy, the function then checks if
the length of the nodename was greater than or equal to len, and if
it is, then the function returns -1 with errno set to ENAMETOOLONG;
in this case, a terminating null byte is not included in the returned
name.



This is shared code, so we should refer to Posix, not linux specific man pages.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/gethostname.html



DESCRIPTION

The gethostname() function shall return the standard host name for the
current machine. The namelen argument shall specify the size of the
array pointed to by the name argument. The returned name shall be
null-terminated, except that if namelen is an insufficient length to
hold the host name, then the returned name shall be truncated and it
is unspecified whether the returned name is null-terminated.

Host names are limited to {HOST_NAME_MAX} bytes.

RETURN VALUE

Upon successful completion, 0 shall be returned; otherwise, -1 shall
be returned.



Note that there is no indication what happens if the buffer is too
small. It may zero-terminate, it may not. It may return an error, it
may not. Decision is left to the platform implementors.

So from that, I would pass in a large-enough buffer and always
zero-terminate on success. According to Posix, a large-enough buffer
means HOST_NAME_MAX bytes.

I do not understand why we use NI_MAXHOST instead (and we we define it
to an arbitrary 1025 byte if undefined). Were there platforms where
HOST_NAME_MAX was too short? If yes, one should at least check that
NI_MAXHOST >= HOST_NAME_MAX.

Even i noticed this, why we use our own NI_MAXHOST instead HOST_NAME_MAX ?

Just for curiosity, are we facing any issues with the current code ?. Your
code change looks logical but if nothing is broken then why to change code.


If it can be proven by looking at the API description that it is
broken for some corner case, why keep it broken?
 :) Agreed, as i said Christoph change is logically correct  but i 
don't know the history behind current code, so just wanted to be sure 
that  we are not missing any corner case.


Thanks,
Vyom



Thanks, Thomas


Thanks,
Vyom

Best regards
Christoph

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

On Thursday 26 April 2018 03:48 PM, Langer, Christoph wrote:

Hi Vyom,

what about my suggestions for renaming?

src/jdk.net/macosx/classes/jdk/net/UnixSocketOptions.java -> 
src/jdk.net/macosx/classes/jdk/net/MacOSXSocketOptions.java
src/jdk.net/macosx/native/libextnet/UnixSocketOptions.c -> 
src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c
till now we don't have file name like MacOSX* so i choose to leave 
as it is but if people think "MacOSXSocketOption" is more appropriate, i 
will change the filename name in my next webrev.


Thanks,
Vyom


This would be more consistent as we have:
src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
src/jdk.net/linux/classes/jdk/net/LinuxSocketOptions.java
...
src/jdk.net/solaris/native/libextnet/SolarisSocketOptions.c
src/jdk.net/solaris/classes/jdk/net/SolarisSocketOptions.java

Thanks
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
vyom tewari
Sent: Montag, 23. April 2018 14:06
To: Chris Hegarty <chris.hega...@oracle.com>; OpenJDK Network Dev list
<net-dev@openjdk.java.net>
Subject: Re: RFR:8194298 Add support for per Socket configuration of TCP
keepalive

Hi,

Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.htm
l).
I incorporated  most of the review comments.

Thanks,

Vyom


On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:

Vyom,

On 13/04/18 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev :
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html

Here is some proposed wording for the JDK-specific extended socket
options specification.

1) Uses a consistent style across all three new options,
    and is in line with existing extended options.
2) Renamed the options slightly, to improve readability
    ( they don't need to conform to the native option names )
3) Reordered them so the build up is more logical
4) Removed inheritance from server sockets
5) Added standard verbiage about being "platform and
    system dependent
6) Added typical values. I think this is useful, as it
    gives an idea to the developer, but maybe it could be
    misleading. Can be dropped if there are concerns.

Can you please confirm that this is accurate, and also
will leave enough "wriggle" room when additional platform
support is added.

---

     /**
  * Keep-Alive idle time.
  *
  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
has been
  * idle for some amount of time. The default value for this idle
period is
  * system dependent, but is typically 2 hours. The {@code
TCP_KEEPIDLE}
  * option can be used to affect this value for a given socket.
  *
  *  The value of this socket option is an {@code Integer} that
is the
  * number of seconds of idle time before keep-alive initiates a
probe. The
  * socket option is specific to stream-oriented sockets using the
TCP/IP
  * protocol. The exact semantics of this socket option are socket
type and
  * system dependent.
  *
  *  @since 11
  */
     public static final SocketOption TCP_KEEPIDLE
     = new ExtSocketOption("TCP_KEEPIDLE",
Integer.class);

     /**
  * Keep-Alive retransmission interval time.
  *
  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
has been
  * idle for some amount of time. If the remote system does not
respond to a
  * keep-alive probe, TCP retransmits the probe after some amount
of time.
  * The default value for this retransmition interval is system
dependent,
  * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL}
option can be
  * used to affect this value for a given socket.
  *
  *  The value of this socket option is an {@code Integer} that
is the
  * number of seconds to wait before retransmitting a keep-alive
probe. The
  * socket option is specific to stream-oriented sockets using the
TCP/IP
  * protocol. The exact semantics of this socket option are socket
type and
  * system dependent.
  *
  * @since 11
  */
     public static final SocketOption TCP_KEEPINTERVAL
     = new ExtSocketOption("TCP_KEEPINTERVAL",
Integer.class);

     /**
  * Keep-Alive retransmission maximum limit.
  *
  *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
  * SO_KEEPALIVE} option is enabled, TCP probes a connection that
has been
  * idle for some amount of time. If the remote system does not
respond to a
  * keep-alive probe, TCP retransmits the probe a certain number of
times
  * before a connection is considered to be broken. The default
value for
  * this keep-alive probe r

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi Alan,

Thanks for review comments, please find my answers inline.

Vyom


On Monday 23 April 2018 06:04 PM, Alan Bateman wrote:

On 23/04/2018 13:05, vyom tewari wrote:

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
I incorporated  most of the review comments.
This looks much better but I think the second paragraph of the spec of 
each option needs to be inverted so that it states clearly what the 
options does before it gets into the background of SO_KEEPALIVE. For 
example, TCP_KEEPALIVE should say that it sets the idle period for 
keep alive before it explains SO_KEEPALIVE. The currently wording also 
begs questions as to whether the socket option means anything when 
SO_KEEPALIVE is not enabled. Also it probably should say something 
about whether it can be changed at any time or not.


The implementation looks much better but the filtering by socket 
option name is still a bit fragile. It might be better for 
ExtendedSocketOptions to mainain 3 sets of options, one for 
SOCK_UNSPEC, one for SOCK_STREAM, the other for SOCK_DGRAM. A map 
would work too. The register method can specify socket type and it's 
very obvious which sockets the option is intended to be used for.
I choose to overload the "options(short type)" over maintain multiple 
set of options because it was straight forward and  minimal changes were 
required. If i split the extended options to multiple set then it will 
increases the complexity i have to change the register method and i have 
to change "ifOptionSupported" as well. i feel it is not worth because we 
are not going to add socket options frequently.


All usages are now ExtendedSocketOptions.getInstance().options(...). 
Have you considered a static options(...) method to reduce the typing 
at each of the usage sites?

no, let me check if we can consider static options(..) method.


-Alan

Re: RFR(XS): 8202181: Correctly specify size of hostname buffer in Unix Inet*AddressImpl_getLocalHostName implementations

[vyom tewari]

On Tuesday 24 April 2018 03:08 PM, Langer, Christoph wrote:


Hi,

please help reviewing a small change that I stumbled over when looking 
into the getLocalHostName implementation. I found that the length of 
the hostname buffer is not correctly passed to sub functions. The 
buffer size is specified as “NI_MAXHOST + 1”, so this size should be 
handed down to gethostname() and getnameinfo() calls, not just 
NI_MAXHOST. I also moved the solaris #ifdefs into the else clause to 
spare a few lines of code.


I think, it is intentional to handle case where return "hostname" is to 
large to fit in  array.  if you see the man 
page(http://man7.org/linux/man-pages/man2/gethostname.2.html) it says 
that it is unspecified whether returned buffer includes a terminating 
null byte.


current code will put null in case of large "hostname", What do you think ?

Thanks,
Vyom


Bug: https://bugs.openjdk.java.net/browse/JDK-8202181 



Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8202181.0/ 



Thanks

Christoph

Re: RFR 8202154 : Remove unused code in java.base/windows/native/libnet

[vyom tewari]

Hi Ivan,

code looks good to me, thanks for doing this cleanup. One minor comment, 
in PortConfig.java you can make defaultUpper& defaultLower as final.I 
see that Microsoft changed dynamic port range recently do we need to put 
some comment in PortConfig.java ?


Thanks, Vyom


On Tuesday 24 April 2018 09:47 AM, Ivan Gerasimov wrote:

Hello again!

A few other files contain obsolete code, so they can be combined 
together in one fix.


The webrev was updated in place:
http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/

Here's the summary of additional changes:
- sun.net.PortConfig.getLower()/getUpper() always return the same 
range, so it can be defined with constants,

- NET_GetDefaultTOS() always returns zero, so it can be removed.

Would you please help review this?

With kind regards,
Ivan

On 4/23/18 2:29 PM, Ivan Gerasimov wrote:

Hello!

Some code in TwoStacksPlainDatagramSocketImpl.c is only relevant for 
earlier versions of Windows, which are no longer supported as of JDK 11.

This code can be safely removed.

Also removing an unused argument at 
DualStackPlainDatagramSocketImpl.socketCreate().


Would you please help review this cleanup?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202154
WEBREV: http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/

Thanks in advance!

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html). 
I incorporated  most of the review comments.


Thanks,

Vyom


On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:

Vyom,

On 13/04/18 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : 
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html


Here is some proposed wording for the JDK-specific extended socket
options specification.

1) Uses a consistent style across all three new options,
   and is in line with existing extended options.
2) Renamed the options slightly, to improve readability
   ( they don't need to conform to the native option names )
3) Reordered them so the build up is more logical
4) Removed inheritance from server sockets
5) Added standard verbiage about being "platform and
   system dependent
6) Added typical values. I think this is useful, as it
   gives an idea to the developer, but maybe it could be
   misleading. Can be dropped if there are concerns.

Can you please confirm that this is accurate, and also
will leave enough "wriggle" room when additional platform
support is added.

---

    /**
 * Keep-Alive idle time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. The default value for this idle 
period is
 * system dependent, but is typically 2 hours. The {@code 
TCP_KEEPIDLE}

 * option can be used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds of idle time before keep-alive initiates a 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 *  @since 11
 */
    public static final SocketOption TCP_KEEPIDLE
    = new ExtSocketOption("TCP_KEEPIDLE", 
Integer.class);


    /**
 * Keep-Alive retransmission interval time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe after some amount 
of time.
 * The default value for this retransmition interval is system 
dependent,
 * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL} 
option can be

 * used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds to wait before retransmitting a keep-alive 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPINTERVAL
    = new ExtSocketOption("TCP_KEEPINTERVAL", 
Integer.class);


    /**
 * Keep-Alive retransmission maximum limit.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe a certain number of 
times
 * before a connection is considered to be broken. The default 
value for

 * this keep-alive probe retransmit limit is system dependent, but is
 * typically 8. The {@code TCP_KEEPCOUNT} option can be used to 
affect

 * this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * maximum number of keep-alive probes to be sent. The socket 
option is
 * specific to stream-oriented sockets using the TCP/IP protocol. 
The exact
 * semantics of this socket option are socket type and system 
dependent.

 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPCOUNT
    = new ExtSocketOption("TCP_KEEPCOUNT", 
Integer.class);


-Chris.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi Chris,


On Wednesday 18 April 2018 07:44 PM, Chris Hegarty wrote:

Vyom,

On 13/04/18 10:50, vyom tewari wrote:

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : 
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html


Here is some proposed wording for the JDK-specific extended socket
options specification.

1) Uses a consistent style across all three new options,
   and is in line with existing extended options.
2) Renamed the options slightly, to improve readability
   ( they don't need to conform to the native option names )
3) Reordered them so the build up is more logical
4) Removed inheritance from server sockets
5) Added standard verbiage about being "platform and
   system dependent
6) Added typical values. I think this is useful, as it
   gives an idea to the developer, but maybe it could be
   misleading. Can be dropped if there are concerns.

Can you please confirm that this is accurate, and also
will leave enough "wriggle" room when additional platform
support is added.

yes, below is perfect i will do the changes and send the updated webrev 
soon. Thanks for help, writing Java doc is always harder(4x) than 
writing code.

Vyom

---

    /**
 * Keep-Alive idle time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. The default value for this idle 
period is
 * system dependent, but is typically 2 hours. The {@code 
TCP_KEEPIDLE}

 * option can be used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds of idle time before keep-alive initiates a 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 *  @since 11
 */
    public static final SocketOption TCP_KEEPIDLE
    = new ExtSocketOption("TCP_KEEPIDLE", 
Integer.class);


    /**
 * Keep-Alive retransmission interval time.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe after some amount 
of time.
 * The default value for this retransmition interval is system 
dependent,
 * but is typically 75 seconds. The {@code TCP_KEEPINTERVAL} 
option can be

 * used to affect this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * number of seconds to wait before retransmitting a keep-alive 
probe. The
 * socket option is specific to stream-oriented sockets using the 
TCP/IP
 * protocol. The exact semantics of this socket option are socket 
type and

 * system dependent.
 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPINTERVAL
    = new ExtSocketOption("TCP_KEEPINTERVAL", 
Integer.class);


    /**
 * Keep-Alive retransmission maximum limit.
 *
 *  When the {@link java.net.StandardSocketOptions#SO_KEEPALIVE
 * SO_KEEPALIVE} option is enabled, TCP probes a connection that 
has been
 * idle for some amount of time. If the remote system does not 
respond to a
 * keep-alive probe, TCP retransmits the probe a certain number of 
times
 * before a connection is considered to be broken. The default 
value for

 * this keep-alive probe retransmit limit is system dependent, but is
 * typically 8. The {@code TCP_KEEPCOUNT} option can be used to 
affect

 * this value for a given socket.
 *
 *  The value of this socket option is an {@code Integer} that 
is the
 * maximum number of keep-alive probes to be sent. The socket 
option is
 * specific to stream-oriented sockets using the TCP/IP protocol. 
The exact
 * semantics of this socket option are socket type and system 
dependent.

 *
 * @since 11
 */
    public static final SocketOption TCP_KEEPCOUNT
    = new ExtSocketOption("TCP_KEEPCOUNT", 
Integer.class);


-Chris.

Re: RFR 8194260, Point-to-point interface should be excluded from java/net/ipv6tests/*

[vyom tewari]

Hi Felix,

latest code looks good to me, personally i prefer to throw exception 
instead returning null, but i can see that old code was also returning 
null. Hopping the code which invokes getFirstLocalIPv4Address, 
getFirstLocalIPv6Addres already taken care of null.


Thanks,

Vyom


On Wednesday 18 April 2018 11:05 AM, Felix Yang wrote:

Hi Chris and Wyom,

    fixed as commented.  Updated webrev:

    http://cr.openjdk.java.net/~xiaofeya/8194260/webrev.01/

Thanks,
Felix
On 2018/4/17 16:25, Chris Hegarty wrote:

On 17 Apr 2018, at 04:34, Felix Yang  wrote:
...
 http://cr.openjdk.java.net/~xiaofeya/8194260/webrev.00/

Thanks for doing this Felix. Mainly looks good. Just a few comments.

The old test runs on systems without IPv4 or IPv6 configured. So
I think the Optional `get` should be replaced with `orElse`. Either that
or update usage to check for the presence of a value in the Optional.

The old test filters out the loopback address, in order to get “real”
addresses. I think we should preserve this behaviour. Other filtering
is done in the old tests too, but I don’t think it is really needed.

---
diff --git a/test/jdk/java/net/ipv6tests/Tests.java 
b/test/jdk/java/net/ipv6tests/Tests.java

--- a/test/jdk/java/net/ipv6tests/Tests.java
+++ b/test/jdk/java/net/ipv6tests/Tests.java
@@ -178,26 +178,28 @@
  }
    public static Inet4Address getFirstLocalIPv4Address () {
-    return getNetworkConfig().ip4Addresses()
- .findFirst()
- .get();
+    return networkConfig.ip4Addresses()
+ .filter(a -> !a.isLoopbackAddress())
+ .findFirst()
+ .orElse(null);
  }
    public static Inet6Address getFirstLocalIPv6Address () {
-    return getNetworkConfig().ip6Addresses()
- .findFirst()
- .get();
+    return networkConfig.ip6Addresses()
+ .filter(a -> !a.isLoopbackAddress())
+ .findFirst()
+ .orElse(null);
  }
  +    private static NetworkConfiguration networkConfig = 
getNetworkConfig();

+
  private static NetworkConfiguration getNetworkConfig() {
-    NetworkConfiguration cfg = null;
  try {
-    cfg = NetworkConfiguration.probe();
+    return  NetworkConfiguration.probe();
  } catch (IOException e) {
  System.out.println("Failed to probe 
NetworkConfiguration");

  throw new RuntimeException(e);
  }
-    return cfg;
  }
  -Chris.

Re: RFR 8194260, Point-to-point interface should be excluded from java/net/ipv6tests/*

[vyom tewari]

Hi Felix,

Looks good to me, minor bit, please fix the tag order in all the tests, 
@summary should come immediately after @bug. In Tests.java there are 
unused imports(import java.net.*, java.io.*) can you please change it to 
use explicit classes.


Thanks,
Vyom

On Tuesday 17 April 2018 09:04 AM, Felix Yang wrote:

Hi all,

   please review the following patch to apply NetworkConfiguration 
library to exclude point-to-point interfaces.


Bug:

    https://bugs.openjdk.java.net/browse/JDK-8194260

Webrev:

    http://cr.openjdk.java.net/~xiaofeya/8194260/webrev.00/

Thanks,

Felix

Re: RFR(XS): 8201369: Inet4AddressImpl_getLocalHostName reverse lookup on Solaris only

[vyom tewari]

looks good to me, As Christoph already mentioned just update the 
copyright date.


Vyom


On Monday 16 April 2018 02:14 PM, Srividya Shamaiah wrote:


Hi Chris,
Please review the attached patch in
http://cr.openjdk.java.net/~mhorie/8201369/webrev/ 



Can you also backport this to JDK 8, we have customers waiting for 
this fix at JDK 8 level.


Thanks,
Srividya S

Inactive hide details for Srividya Shamaiah---11/04/2018 03:35:38 
PM---Thanks Chris , As you suggested, I will provide the patcSrividya 
Shamaiah---11/04/2018 03:35:38 PM---Thanks Chris , As you suggested, I 
will provide the patch based on jdk 11. Thanks,


From: Srividya Shamaiah/India/IBM
To: "Langer, Christoph" 
Cc: Chris Hegarty , OpenJDK Network Dev list 


Date: 11/04/2018 03:35 PM
Subject: RE: 8169865 : Changes not ported to IPv4




Thanks Chris , As you suggested, I will provide the patch based on jdk 11.

Thanks,
Srividya S


Inactive hide details for "Langer, Christoph" ---11/04/2018 02:51:51 
PM---Hi Srividya, I would also welcome this fix."Langer, Christoph" 
---11/04/2018 02:51:51 PM---Hi Srividya, I would also welcome this fix.


From: "Langer, Christoph" 
To: Srividya Shamaiah , Chris Hegarty 


Cc: OpenJDK Network Dev list 
Date: 11/04/2018 02:51 PM
Subject: RE: 8169865 : Changes not ported to IPv4




Hi Srividya,

I would also welcome this fix.

Will you do the fix based on the jdk (11) depot? I think 
Java_java_net_Inet4AddressImpl_getLocalHostName should then be exactly 
the same as Java_java_net_Inet6AddressImpl_getLocalHostName. I can 
assist you with sponsoring/backporting to JDK8, if you like.


Best regards
Christoph



*From:*net-dev [mailto:net-dev-boun...@openjdk.java.net] *On Behalf Of 
*Srividya Shamaiah*

Sent:*Mittwoch, 11. April 2018 09:19*
To:*Chris Hegarty *
Cc:*OpenJDK Network Dev list *
Subject:*Re: 8169865 : Changes not ported to IPv4

Thank you Chris for opening the JIRA bug, I will work on the fix and 
contribute it .


Thanks,
Srividya S

Inactive hide details for Chris Hegarty ---10/04/2018 08:51:05 PM---> 
On 10 Apr 2018, at 12:34, Srividya Shamaiah  On 10 Apr 2018, at 12:34, Srividya 
Shamaiah <_ssham...@in.ibm.com_ > wrote: >


From: Chris Hegarty <_chris.hegarty@oracle.com_ 
>

To: Srividya Shamaiah <_ssham...@in.ibm.com_ >
Cc: OpenJDK Network Dev list <_net-...@openjdk.java.net_ 
>

Date: 10/04/2018 08:51 PM
Subject: Re: 8169865 : Changes not ported to IPv4







> On 10 Apr 2018, at 12:34, Srividya Shamaiah <_ssham...@in.ibm.com_ 
> wrote:

>
> Hi Chris,
>
> One of our customer reported a similar issue and the issue can be 
resolved through the bug fix 8169865 which was include at 8u152 level. 
We were looking this issue from AIX perspective as it did not do the 
reverse lookup with bug fix 8169865 (as reverse lookup is limited to 
solaris after the bug fix).

>
> While implementing the fix, we want to make sure the fix works for 
all scenario. As there is an inconsistency between IPv6 and IPv4 after 
8169865 (as reverse lookup still exists for IPv4 on AIX and Linux), we 
are afraid whether customer can hit the same issue if they use IPv4.

>
> Please confirm whether it makes sense to remove the reverse lookup 
of IPv4 for AIX and linux platforms so that IPv4 and IPv6 processing 
is consistent for those platforms.


Yes, I believe it does.

I filed the follow JIRA issue to track this:
_https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8201369=DwIFAg=jf_iaSHvJObTbx-siA1ZOg=cY5OjfQF2gZ_G00XrJYGrxPgLDHmXjFqs49sDD9oJN0=-LhngTQSiYD1d12WSDvX2Jldxusyok9A7LqJ4ZEIzos=8fDzPwCaD2hwIOSWkfchiRBeDz3uSyzk81kDXZFarXo=_

-Chris.

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

On Sunday 15 April 2018 01:33 PM, Alan Bateman wrote:

On 14/04/2018 08:09, Alan Bateman wrote:

:

Can you update SocketChannel/SocketOptionTests.java to ensure that 
SocketChannel is test? We also need to ensure that the new options 
don't show up in the supportedOptions returned by the channels that 
don't support these new options.
Just on this point, I think this needs work in ExtendedSocketOptions 
so that the extended options are organized by socket type (STREAM or 
DGRAM). This will become a lot more obvious once you add tests for 
SocketChannel as its implementation will need a change to pick up the 
extended options for STREAM sockets. It will also avoid the filtering 
in PlainDatagramSocketImpl that you've added to work around the issue 
there.
Even i thought the same, when i added TCP_QUICKACK but somehow i choose 
not to do at that time. I will modify the tests  and will  do the other 
changes suggested by you.


Vyom


-Alan

Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

On Saturday 14 April 2018 01:40 AM, Bernd Eckenfels wrote:

Hello,

Glad to see progress on this, much needed.

thanks


I wonder if there is a better way for safe and dynamic string 
concatenation in the JDK, this errmsg[56] looks scary.

sure, i will fix it.


Did you tried to support it on Windows, even if it does not support 
all 3 parameters it might be important to be available (I think Oracle 
Database supports it for DCD on Windows, too)
I already mentioned in mail, support for other platform can be added in 
future if needed. Only "Windows 10 1709" support all three socket 
options(TCP_KEEPIDLE, TCP_KEEPCNT, TCP_KEEPINTVL).  These are socket 
options, we want set/get both, I tried to implement  keepalive times 
using WSAloctl 
(https://msdn.microsoft.com/en-us/library/windows/desktop/dd877220(v=vs.85).aspx) 
but  it is not possible to get the current *SIO_KEEPALIVE_VALS* for a 
socket, MSDN says that WSAIoctl output buffer is not used.


Thanks,
Vyom


Gruss
Bernd

Gruss
Bernd
--
http://bernd.eckenfels.net

*From:* net-dev <net-dev-boun...@openjdk.java.net> on behalf of vyom 
tewari <vyom.tew...@oracle.com>

*Sent:* Friday, April 13, 2018 11:50:39 AM
*To:* OpenJDK Network Dev list
*Subject:* RFR:8194298 Add support for per Socket configuration of TCP 
keepalive

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : 
http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8194298/webrev0.0/index.html>


Currently Java supports SO_KEEPALIVE, whose default value is 7200
seconds which is too long for most of the applications. This code change
will allow us to set the keepalive
parameters(TCP_KEEPIDLE,TCP_KEEPCNT,TCP_KEEPINTVL)  which will configure
the idle time on per socket basis.

I did code changes for Linux & Mac only, support for other platforms can
be added in future if needed.

Thanks,

Vyom

RFR:8194298 Add support for per Socket configuration of TCP keepalive

[vyom tewari]

Hi All,

Please review the below code.

BugId    : https://bugs.openjdk.java.net/browse/JDK-8194298

webrev : http://cr.openjdk.java.net/~vtewari/8194298/webrev0.0/index.html

Currently Java supports SO_KEEPALIVE, whose default value is 7200 
seconds which is too long for most of the applications. This code change 
will allow us to set the keepalive 
parameters(TCP_KEEPIDLE,TCP_KEEPCNT,TCP_KEEPINTVL)  which will configure 
the idle time on per socket basis.


I did code changes for Linux & Mac only, support for other platforms can 
be added in future if needed.


Thanks,

Vyom

RFR: 8194676 NullPointerException is thrown if ipaddress is not set

[vyom tewari]

Hi,

Please review below code change for simple fix.

Webrev: http://cr.openjdk.java.net/~vtewari/8194676/webrev0.0/index.html

BudID: https://bugs.openjdk.java.net/browse/JDK-8194676

The code change will fix the unintentional NullPointerException. If ip 
address is not set, while de-serialization NPE will be thrown.


After the change the "readObject" will throw "InvalidObjectException" 
which is already thrown when code will check for "ipaddress.length" 
invariant.


Thanks,

Vyom

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

[vyom tewari]

Hi Chris,

Thanks for review, while my testing i discovered issue in the way we 
handle extended  socket options and standard socket options. I fixed it 
and updated one test as well.


I removed one redundant "if check" which i think not required. JPRT is 
clean with the changed code.


Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.2/index.html).


Thanks,

Vyom


On Thursday 23 November 2017 06:04 PM, Chris Hegarty wrote:

On 23 Nov 2017, at 09:20, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

Thanks for the review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html) where 
i modified the code to take care of SO_FLOW for Solaris.

I think this is fine Vyom, thanks.

-Chris.

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

[vyom tewari]

Hi Doychin,

Thanks for review, i will fix it before pushing.

Thanks,

Vyom


On Thursday 23 November 2017 03:01 PM, Doychin Bondzhev wrote:

Hi Vyom,

There is a typo in the comment :

whis is not applicable

Should be "which"


On 23.11.2017 г. 11:20, vyom tewari wrote:

Hi Chris,

Thanks for the review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html) 
where i modified the code to take care of SO_FLOW for Solaris.


I updated the test code as well.

Thanks,

Vyom


On Monday 20 November 2017 08:50 PM, Chris Hegarty wrote:

Vyom,

On 16/11/17 09:03, vyom tewari wrote:

Hi All,

Please review the small code change for the below issue.

Webrev : 
http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html


BugId    : https://bugs.openjdk.java.net/browse/JDK-8190843

In this code change, i removed the check(getSocket() == null) which 
will always be true for ServerSocket as in case of server socket we 
set "serverSocket" not "socket".


I think that this change is good. What will happen if SO_FLOW is set
on a ServerSocket?

-Chris.

Re: RFR:8190843 can not set/get extendedOptions to ServerSocket

[vyom tewari]

Hi Chris,

Thanks for the review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8190843/webrev0.1/index.html) 
where i modified the code to take care of SO_FLOW for Solaris.


I updated the test code as well.

Thanks,

Vyom


On Monday 20 November 2017 08:50 PM, Chris Hegarty wrote:

Vyom,

On 16/11/17 09:03, vyom tewari wrote:

Hi All,

Please review the small code change for the below issue.

Webrev : 
http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html


BugId    : https://bugs.openjdk.java.net/browse/JDK-8190843

In this code change, i removed the check(getSocket() == null) which 
will always be true for ServerSocket as in case of server socket we 
set "serverSocket" not "socket".


I think that this change is good. What will happen if SO_FLOW is set
on a ServerSocket?

-Chris.

RFR:8190843 can not set/get extendedOptions to ServerSocket

[vyom tewari]

Hi All,

Please review the small code change for the below issue.

Webrev : 
http://cr.openjdk.java.net/~vtewari/8190843/webrev0.0/index.html


BugId    : https://bugs.openjdk.java.net/browse/JDK-8190843

In this code change, i removed the check(getSocket() == null) which will 
always be true for ServerSocket as in case of server socket we set 
"serverSocket" not "socket".


Thanks,

Vyom

RFR: 8189366: SocketInputStream.available() should check for eof

[vyom tewari]

Hi All,

Please review the simple change below.

Webrev   : http://cr.openjdk.java.net/~vtewari/8189366/webrev0.0/index.html

BugId  : https://bugs.openjdk.java.net/browse/JDK-8189366


Currently SocketInputStream.available() does not check for "eof" and 
simply delegate to the impl even when "eof" reached. I put a check  to 
return 0 if "eof" is already reached.


Thanks,

Vyom

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Roger,

Thanks for the review , please find the updated 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.6/index.html).


Thanks,

Vyom


On Tuesday 17 October 2017 06:35 AM, Roger Riggs wrote:

Hi Vyom,

A few suggestions:

PlainDatagramSocketImpl.java:
 - line 95/96:  I think you can use just forEach, the order version is 
not necessary.
    The code will be a bit more readable if the .filter and .forEach 
are on a new line and don't wrap.

    You can also remove the extra "(" and ")

 - line 87-94: these are confusing and imply some implicit resetting 
of the option.

 - use @since 10
- 209/268: the native setQuickAck method should use boolean as its 
argument to enable/disable

  Since enable is a boolean; it does not need "== true'

LinuxSocketOptions.java/c:
  - 52: setQuickAck0 should use boolean for the 2nd argument; (The 
native code already does)


Thanks, Roger


On 10/15/17 11:58 PM, vyom tewari wrote:

Hi Chris,

Thanks for review. Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.html).


Thanks,

Vyom

Re: RFR(S): 8155590: Dubious collection management in sun.net.www.http.KeepAliveCache

[vyom tewari]

Hi Christoph,

Thanks for doing this, i think you don't need to synchronize the 
"remove(HttpClient h)".  This remove is get called from synchronize 
"remove (HttpClient h, Object obj)" and the underline data structure is 
which is java.util.Vector(ClientVector extends java.util.Stack) is also 
thread safe.


What do you think ?

Thanks,

Vyom


On Monday 16 October 2017 12:52 PM, Langer, Christoph wrote:


Hi,

Here is a proposal for a fix for bug 8155590. I already made this fix 
a while ago in our JDK clone and I’d like to contribute this.


Bug: https://bugs.openjdk.java.net/browse/JDK-8155590 



Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8155590.0/ 



Please review.

Thanks

Christoph

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Chris,

Thanks for review. Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.5/index.html).


Thanks,

Vyom


On Saturday 14 October 2017 02:25 AM, Chris Hegarty wrote:

Vyom,


On 12 Oct 2017, at 10:01, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Roger,

Thanks for the review, i incorporated all review comments from you except("you 
can use ExtendedSocketOptions.TCP_QUICKACK to check for the option to omit without
  embedding the name."). ExtendedSocketOptions.java is part of "jdk.net"  so we can not 
directly use it in java.base, hence i used the option name to filter "TCP_QUICKACK".

Please find the update 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).

This looks much better.

Some suggested wordings of the JDK-specific option:

/**
  * Disable Delayed Acknowledgements.
  *
  *  This socket option can be used to reduce or disable delayed
  * acknowledgments (ACKs).
  *
  *  The value of this socket option is a {@code Boolean} that represents
  * whether the option is enabled or disabled. The socket option is specific
  * to stream-oriented sockets using the TCP/IP protocol.
  *
  *  The exact semantics of this socket option are socket type and system
  * dependent.
  *
  *  When TCP_QUICKACK is enabled, ACKs are sent immediately, rather than
  * delayed if needed in accordance to normal TCP operation. This option is
  * not permanent, it only enables a switch to or from TCP_QUICKACK mode.
  *
  *  Subsequent operations of the TCP protocol will once again enter/leave
  * TCP_QUICKACK mode depending on internal protocol processing and factors
  * such as delayed ack timeouts occurring and data transfer.
  *
  * @since 18.3
  */

-Chris.

P.S. D’oh, sorry, of course you need the paragraph tags.



Thanks,

Vyom


On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:

Hi Vyom,

Comments:

  - update copyright
  - use @since 18.3 instead of @since 10

- ExtendedSocketOptions.java: 265,269  include the "TCP_QUICKACK" in the 
exception messages.

 Line 144: if you are going to keep the assert, add a explanation about how 
it could happen.
 I'd remove the assert.

  - The first sentence should be a complete sentence: "TCP_QUICKACK socket option 
enables sending TCP/IP acks immediately" or similar.

  - A reference to the appropriate TCP/IP spec for quickack would be a good 
addition. At least the RFC # and section.
  - 85: space after "."  The phrasing is a bit odd in places in the javadoc.
  - line 81/82 the value is true to enable and false to disable.
  - This phrase is confusing: "it only enables a switch to or from TCP_QUICKACK 
mode";
Since it is set on a socket, it should remain set on that socket until it 
is changed.

  - 203: be consistent in using enable/disable in parameters, etc even for 
private methods.
 "on" -> "enable"

PlainDatagramSocketImpl: 89:
   Why create a new set with zero or one items just to throw it away?
   Use the iterator to add only the non-TCP_QUICKACK options to the supported 
options.
  Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for the option 
to omit without
  embedding the name.


Sockets.java:
   - The initialization of isQuickAckAvailable might be cleaner as an nested 
static class
 that initializes the value in its static initializer. That would delay the 
init til needed
 and avoid extra flags.

LinuxSocketOptions.java:
- the native methods should be static; since the instance is unused.

  - line 41: the return type should be Boolean

  - line 46: the return type of getQuickAck0 should be Boolean like the 
argument to set.

  - line 34:  using JNU_ThrowByNameWithLastError directly is sufficient; if the errno 
does not have a string unix supplies "unknown error nnn".


Regards, Roger

On 10/10/2017 2:58 PM, Chris Hegarty wrote:

Vyom,

What about suggestion 1) below, the name of the socket option?

-Chris.


On 27 Sep 2017, at 09:56, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). I 
incorporated review comments from you and re-base the patch to our consolidated 
repo(jdk10/master).

Thanks,

Vyom


On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:

Vyom,


On 11 Sep 2017, at 16:38, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi All,

As jdk.net API already moved out of java.base, Please review the below code 
change for jdk10.

Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html


This looks quite good. Some comments:

1) I wonder if we should just call the option TCP_QUICKACK, rather than 
SO_QUICKACK? Similar to TCP_NODELAY.

2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.

3) Java

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Alan,

thanks for pointing out, i am forwarding it to net-dev list.

Vyom


On Thursday 12 October 2017 03:54 PM, Alan Bateman wrote:
Best to reply on net-dev as that is where the main review should be 
going on (seems there are at two review threads going, maybe they 
could unite on net-dev).



On 12/10/2017 10:01, vyom tewari wrote:

Hi Roger,

Thanks for the review, i incorporated all review comments from you 
except("you can use ExtendedSocketOptions.TCP_QUICKACK to check for 
the option to omit without
 embedding the name."). ExtendedSocketOptions.java is part of 
"jdk.net"  so we can not directly use it in java.base, hence i used 
the option name to filter "TCP_QUICKACK".


Please find the update 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.4/index.html).


Thanks,

Vyom


On Wednesday 11 October 2017 08:46 PM, Roger Riggs wrote:

Hi Vyom,

Comments:

 - update copyright
 - use @since 18.3 instead of @since 10

- ExtendedSocketOptions.java: 265,269  include the "TCP_QUICKACK" in 
the exception messages.


    Line 144: if you are going to keep the assert, add a explanation 
about how it could happen.

    I'd remove the assert.

 - The first sentence should be a complete sentence: "TCP_QUICKACK 
socket option enables sending TCP/IP acks immediately" or similar.


 - A reference to the appropriate TCP/IP spec for quickack would be 
a good addition. At least the RFC # and section.
 - 85: space after "."  The phrasing is a bit odd in places in the 
javadoc.

 - line 81/82 the value is true to enable and false to disable.
 - This phrase is confusing: "it only enables a switch to or from 
TCP_QUICKACK mode";
   Since it is set on a socket, it should remain set on that socket 
until it is changed.


 - 203: be consistent in using enable/disable in parameters, etc 
even for private methods.

    "on" -> "enable"

PlainDatagramSocketImpl: 89:
  Why create a new set with zero or one items just to throw it away?
  Use the iterator to add only the non-TCP_QUICKACK options to the 
supported options.
 Also, you can use ExtendedSocketOptions.TCP_QUICKACK to check for 
the option to omit without

 embedding the name.


Sockets.java:
  - The initialization of isQuickAckAvailable might be cleaner as an 
nested static class
    that initializes the value in its static initializer. That would 
delay the init til needed

    and avoid extra flags.

LinuxSocketOptions.java:
   - the native methods should be static; since the instance is unused.

 - line 41: the return type should be Boolean

 - line 46: the return type of getQuickAck0 should be Boolean like 
the argument to set.


 - line 34:  using JNU_ThrowByNameWithLastError directly is 
sufficient; if the errno does not have a string unix supplies 
"unknown error nnn".



Regards, Roger

On 10/10/2017 2:58 PM, Chris Hegarty wrote:

Vyom,

What about suggestion 1) below, the name of the socket option?

-Chris.


On 27 Sep 2017, at 09:56, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). 
I incorporated review comments from you and re-base the patch to 
our consolidated repo(jdk10/master).


Thanks,

Vyom


On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:

Vyom,

On 11 Sep 2017, at 16:38, vyom tewari <vyom.tew...@oracle.com> 
wrote:


Hi All,

As jdk.net API already moved out of java.base, Please review the 
below code change for jdk10.


Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev: 
http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html



This looks quite good. Some comments:

1) I wonder if we should just call the option TCP_QUICKACK, 
rather than SO_QUICKACK? Similar to TCP_NODELAY.


2) I think LinuxSocketOptions.h is largely unnecessary, if we do 
1) above.


3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, 
rather than jobject, thus avoiding the need for createBoolean. 
The conversation can happen in the Java layer.  Can you please 
reduce the lint length in this file, by wrapping similar to the 
style of the Solaris version.


4) ExtendedSocketOptions.java
   - drop the , they are unnecessary.
   - I think, similar to TCP_NODELAY, the spec should say that 
the options is TCP specific. From TCP_NODELAY: "The socket option 
is specific to stream-oriented sockets using the TCP/IP protocol.”

   - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”

-Chris.

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Chris,


On Wednesday 11 October 2017 12:28 AM, Chris Hegarty wrote:

Vyom,

What about suggestion 1) below, the name of the socket option?
to be consistent with SO_FLOW_SLA in ExtendedSocketOptions.java, i 
choose the "SO" prefix. But I don't know the history behind the "SO" 
prefix  so i changed the socket option name to "TCP_QUICKACK" as suggested.


Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.3/index.html).


Thanks,
Vyom



-Chris.


On 27 Sep 2017, at 09:56, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). I 
incorporated review comments from you and re-base the patch to our consolidated 
repo(jdk10/master).

Thanks,

Vyom


On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:

Vyom,


On 11 Sep 2017, at 16:38, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi All,

As jdk.net API already moved out of java.base, Please review the below code 
change for jdk10.

Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html


This looks quite good. Some comments:

1) I wonder if we should just call the option TCP_QUICKACK, rather than 
SO_QUICKACK? Similar to TCP_NODELAY.

2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.

3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than 
jobject, thus avoiding the need for createBoolean. The conversation can happen 
in the Java layer.  Can you please reduce the lint length in this file, by 
wrapping similar to the style of the Solaris version.

4) ExtendedSocketOptions.java
   - drop the , they are unnecessary.
   - I think, similar to TCP_NODELAY, the spec should say that the options is TCP 
specific. From TCP_NODELAY: "The socket option is specific to stream-oriented 
sockets using the TCP/IP protocol.”
   - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”

-Chris.

Re: RFR(XS): 8188855: Fix VS10 build after "8187658: Bigger buffer for GetAdaptersAddresses"

[vyom tewari]

Hi Goetz,

Change looks OK to me, although i am not the official reviewer.

Thanks,

Vyom


On Friday 06 October 2017 12:18 PM, Lindenmaier, Goetz wrote:

Hi,

could I please get reviews for this tiny change?
http://cr.openjdk.java.net/~goetz/wr17/8188855-winBuild/webrev/

Best regards,
   Goetz.

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi Chris,

Thanks for review, please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8145635/webrev0.2/index.html). 
I incorporated review comments from you and re-base the patch to our 
consolidated repo(jdk10/master).


Thanks,

Vyom


On Monday 25 September 2017 01:57 AM, Chris Hegarty wrote:

Vyom,


On 11 Sep 2017, at 16:38, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi All,

As jdk.net API already moved out of java.base, Please review the below code 
change for jdk10.

Bug: https://bugs.openjdk.java.net/browse/JDK-8145635
Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html


This looks quite good. Some comments:

1) I wonder if we should just call the option TCP_QUICKACK, rather than 
SO_QUICKACK? Similar to TCP_NODELAY.

2) I think LinuxSocketOptions.h is largely unnecessary, if we do 1) above.

3) Java_jdk_net_LinuxSocketOptions_getQuickAck could return jint, rather than 
jobject, thus avoiding the need for createBoolean. The conversation can happen 
in the Java layer.  Can you please reduce the lint length in this file, by 
wrapping similar to the style of the Solaris version.

4) ExtendedSocketOptions.java
   - drop the , they are unnecessary.
   - I think, similar to TCP_NODELAY, the spec should say that the options is TCP 
specific. From TCP_NODELAY: "The socket option is specific to stream-oriented 
sockets using the TCP/IP protocol.”
   - "In TCP_QUICKACK mode”, maybe “When the option is enabled…”

-Chris.

Re: RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses

[vyom tewari]

On Tuesday 26 September 2017 02:28 AM, Ivan Gerasimov wrote:

Thank you Roger for review!


On 9/25/17 11:47 AM, Roger Riggs wrote:

Hi Ivan,

Looks ok to me.

I don't see a reason to skimp on the additional size since it is 
allocated and then freed immediately.

The increment might as well be as big as the initial size.

Right.  Let's use the same value of 15K, which reduces the number of 
variables to operate with.


I don't see a reason to retry if the buffer gets close to ULONG_MAX; 
I'd just break the for loop
and let it fail. (but the new code is just doing what the old code 
did and retry even if the buffer did not increase).


If len is close to ULONG_MAX, it is still larger value of len returned 
by the previous call to GetAdaptersAddresses (otherwise we wouldn't 
have gotten ERROR_BUFFER_OVERFLOW.)
So, if we're in the loop, there's no way the buffer will not increase 
(unless there's a failure due to OOM, of course.)


Also, please introduce a constant for the number of retries; it will 
make it clearer

and not need to hard code it in two places.

Done!

Would you please take a look at the updated webrev:

http://cr.openjdk.java.net/~igerasim/8187658/01/webrev/

looks good to me , although i am not the reviewer.
Vyom


With kind regards,
Ivan

(I would probably have coded the retry count counting down to zero 
but its the same effect).


Regards, Roger


On 9/25/2017 1:03 PM, Ivan Gerasimov wrote:

Ping.

Please review the proposed change at your convenience.

The fix will greatly reduce the possibility of a need to reallocate 
the buffer to retrieve the results (something that the documentation 
strongly suggests to avoid), and, if such reallocation still occurs 
to be necessary, will increase number of retries.


With kind regards,
Ivan


On 9/19/17 10:50 AM, Ivan Gerasimov wrote:

Hello!

When retrieving information about network interfaces on Windows we 
make up to 2 attempts to call GetAdaptersAddresses().


It was reported that in very rare cases it may not be sufficient, 
and even the second attempt can fail with ERROR_BUFFER_OVERFLOW.


I suggest that we follow the recommendation given in MSDN [1]: 
increase the initial buffer size to 15K and do up to 3 attempts to 
call GetAdaptersAddresses().


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658
WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/

Would you please help review the fix?

[1] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx

Re: RFR[10]: 8185072 network006 times out in many configs in JDK10-hs nightly

[vyom tewari]

On Monday 25 September 2017 08:08 PM, Roger Riggs wrote:

Hi Vyom,

Looks fine to me too.

My only concern was adding an intensive test (used to be a stress 
test) to the normal
test cycles.  I'd be more comfortable with it being in a 
separate/lower tier or as @manual.
thanks for review, i will make it "manual" before pushing as we already 
have similar test at lower tier.

Vyom


Roger



On 9/24/2017 1:15 PM, Chris Hegarty wrote:

On 15 Sep 2017, at 10:29, vyom tewari <vyom.tew...@oracle.com> wrote:

Hi,

Please review, small code change below.

webrev: 
http://cr.openjdk.java.net/~vtewari/8185072/webrev0.0/index.html
The changes look good to me Vyom. ( I only took a cursory look at the 
test changes )


-Chris.

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

ping!!

Vyom


On Monday 11 September 2017 09:08 PM, vyom tewari wrote:


Hi All,

As jdk.net API already moved out of java.base, Please review the below 
code change for jdk10.


Bug: https://bugs.openjdk.java.net/browse/JDK-8145635

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html

Thanks,

Vyom


On Wednesday 24 February 2016 03:16 PM, Alan Bateman wrote:


On 24/02/2016 09:16, vyom wrote:

Hi All,

Please review my code changes for the below issue.

Bug: JDK-8145635 : Add TCP_QUICKACK socket option

Webrev: 
http://cr.openjdk.java.net/~vtewari/8145635/webrev0.0/index.html


Currently TCP_QUICKACK is only supported on Linux( since Linux 
2.4.4) so i implemented is as "ExtendedSocketOption".


Is there any urgency on this one? I'm just wondering if we should try 
to the jdk.net API moved out of java.base before we add more socket 
options. This is currently tracked as JDK-8044773 and important to 
get into JDK 9.


-Alan

Re: RFR[10]: 8185072 network006 times out in many configs in JDK10-hs nightly

[vyom tewari]

ping!!

Vyom


On Friday 15 September 2017 02:59 PM, vyom tewari wrote:

Hi,

Please review, small code change below.

webrev: http://cr.openjdk.java.net/~vtewari/8185072/webrev0.0/index.html

BugId:  https://bugs.openjdk.java.net/browse/JDK-8185072

this is regression because of "JDK-8179905".

Thanks,

Vyom

RFR[10]: 8185072 network006 times out in many configs in JDK10-hs nightly

[vyom tewari]

Hi,

Please review, small code change below.

webrev: http://cr.openjdk.java.net/~vtewari/8185072/webrev0.0/index.html

BugId:  https://bugs.openjdk.java.net/browse/JDK-8185072

this is regression because of "JDK-8179905".

Thanks,

Vyom

Re: RFR[10]: JDK-8023649 - java.net.NetworkInterface.getInterfaceAddresses() call flow clean up

[vyom tewari]

Hi Mark,

Thanks for doing this, i see that "createNetworkInterface" method is 
getting called from multiple places in NetworkInterface.c there is 
pending JNI Exception at line 695 in NetworkInterface.c. I am not sure 
if it is safe to call "(*env)->ReleaseStringUTFChars" even if there is 
pending JNI Exception.


Thanks,

Vyom


On Tuesday 12 September 2017 10:50 PM, Mark Sheppard wrote:

Hi,
please oblige and review the follows changes:
http://cr.openjdk.java.net/~msheppar/8023649/webrev/

for the issue:
https://bugs.openjdk.java.net/browse/JDK-8023649

This is performed under the auspices of reliability, robustness and 
stability.

* As such, a number of error checks are amended to free malloc-ed memory,
* to add consistency to the code, additional ExceptionCheck have been 
added as a post  SetObjectArrayElement invocation check,
* A long standing issue reporting that 
NetworkInterface::getInterfacesAddresses can throw an NPE has been 
addressed.
The context for such a failure is that there is an IPv4 only 
configuration and that configuration is incorrect in its setting.
This may lead to the bindings array being allocated, but expected 
InterfaceAddress objects not allocated
and set in the array  so the bindings array implicitly contains 
null references.


In NetworkInterface.c  the function
createNetworkInterface
performs a check on an address mask which may lead to skipping the 
code block handling the InterfaceAddress allocation.


regards
Mark

Re: RFR 8145635 : Add TCP_QUICKACK socket option

[vyom tewari]

Hi All,

As jdk.net API already moved out of java.base, Please review the below 
code change for jdk10.


Bug: https://bugs.openjdk.java.net/browse/JDK-8145635

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.1/index.html

Thanks,

Vyom


On Wednesday 24 February 2016 03:16 PM, Alan Bateman wrote:


On 24/02/2016 09:16, vyom wrote:

Hi All,

Please review my code changes for the below issue.

Bug: JDK-8145635 : Add TCP_QUICKACK socket option

Webrev: http://cr.openjdk.java.net/~vtewari/8145635/webrev0.0/index.html

Currently TCP_QUICKACK is only supported on Linux( since Linux 2.4.4) 
so i implemented is as "ExtendedSocketOption".


Is there any urgency on this one? I'm just wondering if we should try 
to the jdk.net API moved out of java.base before we add more socket 
options. This is currently tracked as JDK-8044773 and important to get 
into JDK 9.


-Alan

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Vyom Tewari]

Hi Chris,

test looks good, one minor comment can be ignored, in failing case it 
print stack and "Exception in thread "main" java.lang.RuntimeException: 
Failed" both. I can see main is declared to throw Exception. Is is 
possible to throw exception instead of  printing trace or pass this 
exception to last line ?


Thanks,

Vyom


On Friday 23 June 2017 06:58 PM, Chris Hegarty wrote:



On 23/06/17 10:56, Seán Coffey wrote:

Thanks to Christoph, Vyom and Mark for the reviews.

I've improved the testcase as per feedback. Hope this meets all 
requests :


http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10.v2/webrev/


The change looks good.

Sean and I did a live coding session and arrived at the following
version of the test.

-Chris.

/*
 * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but 
WITHOUT

 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file 
that

 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License 
version

 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 
USA

 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/*
 * @test
 * @bug 8182672
 * @summary Java 8u121 on Linux intermittently returns null for MAC 
address

 */

import java.net.NetworkInterface;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.Phaser;
import java.util.function.Predicate;
import java.util.stream.Collectors;

public class GetMacAddress implements Callable {
static final int NUM_THREADS = 5;
static final int NUM_ITERS = 100;
static volatile boolean failed; // false

final String threadName;
final NetworkInterface ni;
final Phaser startingGate;

public GetMacAddress(NetworkInterface ni, String name, Phaser 
phaser) {

this.ni = ni;
this.threadName = name;
this.startingGate = phaser;
}

@Override
public Exception call() {
int count = 0;
startingGate.arriveAndAwaitAdvance();
try {
for (int i = 0; i < NUM_ITERS; i++) {
ni.getMTU();
byte[] addr = ni.getHardwareAddress();
if (addr == null) {
System.out.println(threadName + ". mac id is null");
failed = true;
}
count = count + 1;
if (count % 100 == 0) {
System.out.println(threadName + ". count is " + 
count);

}
}
} catch (Exception ex) {
System.out.println(threadName + ". Not expecting 
exception:" + ex.getMessage());

failed = true;
return ex;
}
return null;
}

static final Predicate hasHardwareAddress = ni -> {
try {
if (ni.getHardwareAddress() == null) {
System.out.println("Not testing null addr: " + 
ni.getName());

return false;
}
} catch (Exception ex) {
System.out.println("Not testing: " + ni.getName() +
" " + ex.getMessage());
}
return true;
};

public static void main(String[] args) throws Exception {
List toTest = 
NetworkInterface.networkInterfaces()

.filter(hasHardwareAddress)
.collect(Collectors.toList());

ExecutorService executor = 
Executors.newFixedThreadPool(NUM_THREADS);


for (NetworkInterface ni : toTest) {
Phaser startingGate = new Phaser(NUM_THREADS);
System.out.println("Testing: " + ni.getName());
List list = new ArrayList<>();
for (int i = 0; i < NUM_THREADS; i++)
list.add(new GetMacAddress(ni, ni.getName() + 
"-Thread-" + i, startingGate));

List futures = executor.invokeAll(list);
for (Future f : futures) {
if (f.get() != null)
f.get().printStackTrace(System.out);
}
if (failed)
break;
}
executor.shutdownNow();
if (!failed) {
System.out.println("PASSED 

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Vyom Tewari]

Hi Sean,

latest test code looks good, one minor bit, in MT program 
"ex.printStackTrace();" will not help as output will be screwed. I will 
suggested to pass the original exception to line 106.


throw new RuntimeException(originalException);

Thanks,

Vyom


On Friday 23 June 2017 03:26 PM, Seán Coffey wrote:

Thanks to Christoph, Vyom and Mark for the reviews.

I've improved the testcase as per feedback. Hope this meets all 
requests :


http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10.v2/webrev/

Regards,
Sean.

On 22/06/17 22:36, Langer, Christoph wrote:

Hi Sean,

I played with it a bit more and now really understand Vyoms 
observation. So, what he sees is not the original concurrency issue 
but he encounters a SocketException on some interface, where this is 
supposed to occur upon calling getHardwareAddress().


So, to enable the testcase to run robustly on any platform, any 
hardware and any network configuration, I suggest to modify the test 
like this:


1. In main, enumerate all interfaces once
2. Iterate over all interfaces, exlude loopback and see if a single 
call to getHardwareAddress() won't yield null or an Exception.
3. For each interface where a valid mac address could be obtained 
once, start the executor threads to stress getHardwareAddress() in 
parallel, e.g. like 5 threads doing it 100 times in parallel. I would 
also suggest to use per thread counters instead of one global 'count' 
as we have right now.


Furthermore, the test output could be improved a bit, e.g. when it 
comes to an exception, the thread name could be mentioned, too.


Best regards
Christoph


-Original Message-
From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
Seán Coffey
Sent: Donnerstag, 22. Juni 2017 20:17
To: Vyom Tewari <vyom.tew...@oracle.com>; net-dev 
Subject: Re: RFR : 8182672: Java 8u121 on Linux intermittently 
returns null for

MAC address

Hi Vyom,

thanks for testing. Null is a valid value for some interfaces. I've
excluded the loopback interface from being testing. Perhaps there's
other issues at play here.  We know that getHardwareAddress() can throw
SocketException if I/O fails. For this particular scenario we're not
interested in that and perhaps that can be ignored.

I'll take another look.

regards,
Sean.

On 22/06/2017 18:50, Vyom Tewari wrote:

Hi Sean,

with your patch as well your test case is failing on my
laptop(Ubuntu16.04), when i tried to run  on jdk8 i am getting below
error.

java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
 at java.net.NetworkInterface.getMacAddr0(Native Method)
 at


java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457
)

 at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
 at

java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.jav 


a:1142)

 at

java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.ja 


va:617)

 at java.lang.Thread.run(Thread.java:745)
java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
 at java.net.NetworkInterface.getMacAddr0(Native Method)
 at


java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457
)

 at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
 at

java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.jav 


a:1142)

 at

java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.ja 


va:617)

 at java.lang.Thread.run(Thread.java:745)
Exception in thread "main" java.lang.RuntimeException: Failed
 at com.java.test.GetMacAddress.main(GetMacAddress.java:96)
mac id is null for interface cscotun0- Thread0
Testing: cscotun0
mac id is null for interface cscotun0- Thread3
Testing: cscotun0

Thanks,

Vyom


On Thursday 22 June 2017 09:59 PM, Seán Coffey wrote:

JDK 10 fix required to correct a race issue in NetworkInterface. I
don't believe the ifreq struct needs to be static in any case. New
auto unit testcase also. I propose to skip this fix for JDK 9 and fix
in an update release for that family. I also plan to port this to
jdk8u-dev.

https://bugs.openjdk.java.net/browse/JDK-8182672
webrev :
http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10/webrev/

regards,
Sean.

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Vyom Tewari]

Hi Sean,

i  debug the failure and it look like you need to modify your test and 
make it more robust. As Christoph suggested first call 
"getHardwareAddress" and check if is not null then only iterate 100 
time. This way your test will run on any platform and one minor comment 
make your global variables(count ) volatile.


I did the changes at my local env and it did worked for me  and your 
modified test did not failed after fix.


Thanks,

Vyom


On Thursday 22 June 2017 11:46 PM, Seán Coffey wrote:

Hi Vyom,

thanks for testing. Null is a valid value for some interfaces. I've 
excluded the loopback interface from being testing. Perhaps there's 
other issues at play here.  We know that getHardwareAddress() can 
throw SocketException if I/O fails. For this particular scenario we're 
not interested in that and perhaps that can be ignored.


I'll take another look.

regards,
Sean.

On 22/06/2017 18:50, Vyom Tewari wrote:

Hi Sean,

with your patch as well your test case is failing on my 
laptop(Ubuntu16.04), when i tried to run  on jdk8 i am getting below 
error.


java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
at java.net.NetworkInterface.getMacAddr0(Native Method)
at 
java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457)

at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)

at java.lang.Thread.run(Thread.java:745)
java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
at java.net.NetworkInterface.getMacAddr0(Native Method)
at 
java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457)

at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)

at java.lang.Thread.run(Thread.java:745)
Exception in thread "main" java.lang.RuntimeException: Failed
at com.java.test.GetMacAddress.main(GetMacAddress.java:96)
mac id is null for interface cscotun0- Thread0
Testing: cscotun0
mac id is null for interface cscotun0- Thread3
Testing: cscotun0

Thanks,

Vyom


On Thursday 22 June 2017 09:59 PM, Seán Coffey wrote:
JDK 10 fix required to correct a race issue in NetworkInterface. I 
don't believe the ifreq struct needs to be static in any case. New 
auto unit testcase also. I propose to skip this fix for JDK 9 and 
fix in an update release for that family. I also plan to port this 
to jdk8u-dev.


https://bugs.openjdk.java.net/browse/JDK-8182672
webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10/webrev/


regards,
Sean.

Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null for MAC address

[Vyom Tewari]

Hi Sean,

with your patch as well your test case is failing on my 
laptop(Ubuntu16.04), when i tried to run  on jdk8 i am getting below error.


java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
at java.net.NetworkInterface.getMacAddr0(Native Method)
at 
java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457)

at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)

at java.lang.Thread.run(Thread.java:745)
java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR) failed)
at java.net.NetworkInterface.getMacAddr0(Native Method)
at 
java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457)

at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)

at java.lang.Thread.run(Thread.java:745)
Exception in thread "main" java.lang.RuntimeException: Failed
at com.java.test.GetMacAddress.main(GetMacAddress.java:96)
mac id is null for interface cscotun0- Thread0
Testing: cscotun0
mac id is null for interface cscotun0- Thread3
Testing: cscotun0

Thanks,

Vyom


On Thursday 22 June 2017 09:59 PM, Seán Coffey wrote:
JDK 10 fix required to correct a race issue in NetworkInterface. I 
don't believe the ifreq struct needs to be static in any case. New 
auto unit testcase also. I propose to skip this fix for JDK 9 and fix 
in an update release for that family. I also plan to port this to 
jdk8u-dev.


https://bugs.openjdk.java.net/browse/JDK-8182672
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10/webrev/

regards,
Sean.

Re: RFR (xxs): 8180424: Another build issue on AIX after 8034174

[Vyom Tewari]

Hi Thomas,

fix look good to me, but i am not jdk10 reviewer.

Thanks,

Vyom


On Tuesday 16 May 2017 06:20 PM, Thomas Stüfe wrote:

Hi all,

may I have a review for this tiny fix:

Issue: https://bugs.openjdk.java.net/browse/JDK-8180424
webrev: 
http://cr.openjdk.java.net/~stuefe/webrevs/8180424-another-build-issue-on-aix-after-8034174/webrev.00/webrev/ 



The prototypes for NET_RecvFrom and NET_Accept do not match their 
implementations for AIX since 8034174. This did not lead to an error 
in jdk9 because there, the header (net_util_md.h) was not included by 
aix_close.c. In JDK10, it is included and therefore does not build.


I believe this did not lead to a runtime error on jdk9, at least not 
for the typical values involved; the mismatch is between int* and 
unsigned int* (native socklen_t).


Kind Regards, Thomas

Re: RFR 8179905: Remove the use of gettimeofday in Networking code

[Vyom Tewari]

Hi Claes,

thanks for review, timeout is "int" so even if you set max(2147483647) 
value that int data type can hold, there will not be any overflow. If 
you try to set very large number like "0x7fff" to int data 
type you will get compile time error(integer number too large: 
7fff).


Thanks,
Vyom

On Tuesday 09 May 2017 11:20 PM, Claes Redestad wrote:

Hi,

doesn't this need to consider numerical overflows, e.g., what happens 
if someone sets a timeout value near 0x7fffL before and 
after this change?


/Claes

On 2017-05-09 11:55, Vyom Tewari wrote:

Hi ,

Please review the code change for below issue.

Webrev  : 
http://cr.openjdk.java.net/~vtewari/8179905/webrev0.0/index.html


BugId : https://bugs.openjdk.java.net/browse/JDK-8179905

This issue is duplicate of "JDK-8165437", where pushed code change 
failed on 32 bit platforms so we revert back code changes as part of 
"JDK-8179602".


Please find below is the webrev that was pushed as part of 
"JDK-8165437".


Webrev  : 
http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html


Thanks,

Vyom

RFR 8179905: Remove the use of gettimeofday in Networking code

[Vyom Tewari]

Hi ,

Please review the code change for below issue.

Webrev  : http://cr.openjdk.java.net/~vtewari/8179905/webrev0.0/index.html

BugId : https://bugs.openjdk.java.net/browse/JDK-8179905

This issue is duplicate of "JDK-8165437", where pushed code change 
failed on 32 bit platforms so we revert back code changes as part of 
"JDK-8179602".


Please find below is the webrev that was pushed as part of "JDK-8165437".

Webrev  : http://cr.openjdk.java.net/~vtewari/8165437/webrev0.7/index.html

Thanks,

Vyom

Re: RFR 8179602: Fix for JDK-8165437 is broken on 32-bit Linux

[Vyom Tewari]

Hi All,

please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8179602/webrev0.1/index.html) 
which fixed the Mac OS issued as well.


Michael as we we discussed off line, i incorporated the changes 
suggested by you,


Thanks,

Vyom


On Thursday 04 May 2017 03:11 PM, Michael McMahon wrote:

Hi Vyom,

I notice this doesn't seem to fix the Mac OS problem.
We may have to file a separate issue for that.

- Michael

On 04/05/2017, 09:50, Vyom Tewari wrote:

Hi All,

Please  review the below change.

Webrev: http://cr.openjdk.java.net/~vtewari/8179602/webrev0.0/index.html

Bugid: https://bugs.openjdk.java.net/browse/JDK-8179602

This issue is because of side effect of "JDK-8165437" where we are 
using "JVM_NanoTime" which returns a 64 bit jlong and return value 
was getting assigned to long type. On 32 bit OS long is 4 byte, which 
leads to integer overflow.


Our internal test JPRT is  still running.

Thanks,

Vyom