Re: RFR 8136933: Additional tests for Solaris SO_FLOW_SLA socket option in JDK 9

[Svetlana Nikandrova]

I hate being annoying, but may be you can find another minute to review 
updated diff?


Thank you,
Svetlana

On 23.05.2016 19:36, Svetlana Nikandrova wrote:

Alan, Chris,

thank you for your comments. I've decided to do as Chris suggested and 
updated existing  test test/jdk/net/Sockets/Test.java instead of 
creating a new one.

Please see updated review:

http://cr.openjdk.java.net/~snikandrova/8136933/webrev.03/ 



Thank you,
Svetlana

On 20.05.2016 18:19, Chris Hegarty wrote:

On 20 May 2016, at 14:10, Alan Bateman  wrote:

On 20/05/2016 14:05, Svetlana Nikandrova wrote:

Alan,

another test related to this option is on the same level 
(test/jdk/net/SocketFlow)

I added this recently, when working on a different issue, when I
noticed that there were no tests for the SocketFllow API, regardless
of the underlying system support. The test directory structure
matches the class package/name hierarchy.

so I thought it's ok to maintain the same hierarchy. I can move 
test to test/jdk/net/Sockets/ExtendedSocketOptions/SO_FLOW_SLA or 
to test/jdk/net/Sockets, but in that case won't it be a little bit 
confusing?


As for overlapping coverage: existing test silently exits if option 
is not in supported list while this one is focused on platform 
support check.
I think it would be good to put all the tests in the same place, 
will make it easier for further maintenance in this area.

The proposed new test seems, somewhat, to be a replacement for
test/jdk/net/Sockets/Test.java, but it does not assert that if set
succeeds that get should return something useful? Also set
typically cannot succeed unless run with privileges.  The test also
seems to focus on setting the option through the idk.net.Sockets API
rather than through the setOption of the socket types, any reason
for this? The jdk.net.Sockets API should be deprecated in 9, as
we have a standard replacement.

When working in the area previously, I ran
test/jdk/net/Sockets/Test.java manually and examined the output
to determine that the option was being set correctly.

It this test is merely to check that the option is supported on
S 11.2 +, then maybe that could be added to the existing
test/jdk/net/Sockets/Test.java  ?

-Chris.

Re: JDK-8148424 Support IPv6-only Unix environments

[Martin Buchholz]

I'm (slowly) working on getting you more ipv6 stuff...

On Thu, May 12, 2016 at 8:32 AM, Chris Hegarty  wrote:
> Martin,
>
> Promoted by some other changes that are going on, I stumbled
> across your proposal for 8148424: "Support IPv6-only Unix
> environments” [1][2][3].
>
> I think the changes are good. I rebased them against the latest
> source in jdk9/dev.
>
>   http://cr.openjdk.java.net/~chegar/8148424/
>
> I put the changes through our build and test system, and there
> are no surprising results.  Have you done much testing of the
> change in your environment?
>
> -Chris
>
> [1] http://mail.openjdk.java.net/pipermail/nio-dev/2016-January/003509.html
> [2] http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ipv6-only/
> [3] https://bugs.openjdk.java.net/browse/JDK-8148424
>

Re: RFR 8136933: Additional tests for Solaris SO_FLOW_SLA socket option in JDK 9

[Chris Hegarty]

> On 24 May 2016, at 17:21, Svetlana Nikandrova 
>  wrote:
> 
> I hate being annoying, but may be you can find another minute to review 
> updated diff?

This mainly look fine. Some comments:

 - The test will need to '@build jdk.testlibrary.*’ to ensure that the library 
classes
are compiled. Please verify that the test can run successfully, by itself, 
in a 
clean work directory.

 - You can initialize ‘expectSupport' and make it final:
 private static final boolean expectSupport = checkExpectedOptionSupport();
 
 - If you statically import SO_FLOW_SLA, then things will fit easier, e.g.

   import static jdk.net.ExtendedSocketOptions.SO_FLOW_SLA;
   …
  if (s.supportedOptions().contains(SO_FLOW_SLA) != expectSupport) {

  … and maybe drop the unnecessary braces. 

 - Can you please revert the indentation on SocketFlow.create(),
   i.e. line up the dots

-Chris.

> Thank you,
> Svetlana
> 
> On 23.05.2016 19:36, Svetlana Nikandrova wrote:
>> Alan, Chris,
>> 
>> thank you for your comments. I've decided to do as Chris suggested and 
>> updated existing  test test/jdk/net/Sockets/Test.java instead of creating a 
>> new one.
>> Please see updated review:
>> 
>> http://cr.openjdk.java.net/~snikandrova/8136933/webrev.03/ 
>> 
>> 
>> Thank you,
>> Svetlana
>> 
>> On 20.05.2016 18:19, Chris Hegarty wrote:
 On 20 May 2016, at 14:10, Alan Bateman  wrote:
 
 On 20/05/2016 14:05, Svetlana Nikandrova wrote:
> Alan,
> 
> another test related to this option is on the same level 
> (test/jdk/net/SocketFlow)
>>> I added this recently, when working on a different issue, when I
>>> noticed that there were no tests for the SocketFllow API, regardless
>>> of the underlying system support. The test directory structure
>>> matches the class package/name hierarchy.
>>> 
> so I thought it's ok to maintain the same hierarchy. I can move test to 
> test/jdk/net/Sockets/ExtendedSocketOptions/SO_FLOW_SLA or to 
> test/jdk/net/Sockets, but in that case won't it be a little bit confusing?
> 
> As for overlapping coverage: existing test silently exits if option is 
> not in supported list while this one is focused on platform support check.
 I think it would be good to put all the tests in the same place, will make 
 it easier for further maintenance in this area.
>>> The proposed new test seems, somewhat, to be a replacement for
>>> test/jdk/net/Sockets/Test.java, but it does not assert that if set
>>> succeeds that get should return something useful? Also set
>>> typically cannot succeed unless run with privileges.  The test also
>>> seems to focus on setting the option through the idk.net.Sockets API
>>> rather than through the setOption of the socket types, any reason
>>> for this? The jdk.net.Sockets API should be deprecated in 9, as
>>> we have a standard replacement.
>>> 
>>> When working in the area previously, I ran
>>> test/jdk/net/Sockets/Test.java manually and examined the output
>>> to determine that the option was being set correctly.
>>> 
>>> It this test is merely to check that the option is supported on
>>> S 11.2 +, then maybe that could be added to the existing
>>> test/jdk/net/Sockets/Test.java  ?
>>> 
>>> -Chris.
>> 
> 

RE: JDK-8148424 Support IPv6-only Unix environments

[Langer, Christoph]

Hi,

to me the changes look good.

I'll take the patch into my queue and do some testing on AIX.

Best regards
Christoph

> -Original Message-
> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Chris
> Hegarty
> Sent: Donnerstag, 12. Mai 2016 17:32
> To: Martin Buchholz ; net-dev@openjdk.java.net >>
> OpenJDK Network Dev list 
> Subject: JDK-8148424 Support IPv6-only Unix environments
>
> Martin,
>
> Promoted by some other changes that are going on, I stumbled
> across your proposal for 8148424: "Support IPv6-only Unix
> environments" [1][2][3].
>
> I think the changes are good. I rebased them against the latest
> source in jdk9/dev.
>
>   http://cr.openjdk.java.net/~chegar/8148424/
>
> I put the changes through our build and test system, and there
> are no surprising results.  Have you done much testing of the
> change in your environment?
>
> -Chris
>
> [1] http://mail.openjdk.java.net/pipermail/nio-dev/2016-January/003509.html
> [2] http://cr.openjdk.java.net/~martin/webrevs/openjdk9/ipv6-only/
> [3] https://bugs.openjdk.java.net/browse/JDK-8148424