Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Alan Bateman]

On 27/04/2016 10:04, Chris Hegarty wrote:

On 26 Apr 2016, at 18:21, Alan Bateman  wrote:


I took a second pass over it. One thing that I'm wondering about is whether 
BaseExtendedSocketOptions + Support should be collapsed into one abstract class 
ExtendedSocketOptions (or better name) with 3 instance methods and 2 static methods. It's 
an internal interface so not a big deal but I think it would be a bit cleaner and allowed 
the oddly named "Support" to go away.

This works out quite nice.  Webrev updated in-place:
   http://cr.openjdk.java.net/~chegar/8044773/jdk/


I think this looks good.

The NoExtendedSocketOptions constructor should be able to just use 
Collections.emptySet() as that is unmodifiable.


"no extended options" - it might be better to include option.name() in 
the message.


Otherwise looks okay to me.

-Alan.

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Mandy Chung]

> On Apr 27, 2016, at 11:16 AM, Chris Hegarty  wrote:
> 
> 
> On 27 Apr 2016, at 17:27, Mandy Chung  wrote:
> 
>> 
>>> On Apr 27, 2016, at 2:04 AM, Chris Hegarty  wrote:
>>> 
>>> This works out quite nice.  Webrev updated in-place:
>>> http://cr.openjdk.java.net/~chegar/8044773/jdk/
>> 
>> One comment on the qualified exports of 
>> sun.security.action.GetPropertyAction to jdk.net module.  This is a simple 
>> utility method used only in a couple places in jdk.net.  I would suggest to 
>> change to usage to call System::getProperty with doPrivileged and no need to 
>> add the qualified export from java.base.  
> 
> Agreed. Removed. Webrev updated in-place:
>  http://cr.openjdk.java.net/~chegar/8044773/jdk/

+1

Mandy

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Chris Hegarty]

On 27 Apr 2016, at 17:27, Mandy Chung  wrote:

> 
>> On Apr 27, 2016, at 2:04 AM, Chris Hegarty  wrote:
>> 
>> This works out quite nice.  Webrev updated in-place:
>> http://cr.openjdk.java.net/~chegar/8044773/jdk/
> 
> One comment on the qualified exports of sun.security.action.GetPropertyAction 
> to jdk.net module.  This is a simple utility method used only in a couple 
> places in jdk.net.  I would suggest to change to usage to call 
> System::getProperty with doPrivileged and no need to add the qualified export 
> from java.base.  

Agreed. Removed. Webrev updated in-place:
  http://cr.openjdk.java.net/~chegar/8044773/jdk/

> As a side note, Sean and I and a couple others are discussing that the 
> security team may want to have recommended best practices around doPrivileged 
> and utility methods like this.

That would be good.

-Chris.

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Mandy Chung]

> On Apr 27, 2016, at 2:04 AM, Chris Hegarty  wrote:
> 
> This works out quite nice.  Webrev updated in-place:
>  http://cr.openjdk.java.net/~chegar/8044773/jdk/

One comment on the qualified exports of sun.security.action.GetPropertyAction 
to jdk.net module.  This is a simple utility method used only in a couple 
places in jdk.net.  I would suggest to change to usage to call 
System::getProperty with doPrivileged and no need to add the qualified export 
from java.base.  

As a side note, Sean and I and a couple others are discussing that the security 
team may want to have recommended best practices around doPrivileged and 
utility methods like this.

Mandy

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Chris Hegarty]

On 26 Apr 2016, at 18:21, Alan Bateman  wrote:

> I took a second pass over it. One thing that I'm wondering about is whether 
> BaseExtendedSocketOptions + Support should be collapsed into one abstract 
> class ExtendedSocketOptions (or better name) with 3 instance methods and 2 
> static methods. It's an internal interface so not a big deal but I think it 
> would be a bit cleaner and allowed the oddly named "Support" to go away.

This works out quite nice.  Webrev updated in-place:
  http://cr.openjdk.java.net/~chegar/8044773/jdk/

With this refactoring, and the reduced number of types over the various
iterations, I think we can remove the ‘Base' prefix and use the simpler
name sun.net.ext.ExtendedSocketOptions ( conflict with
jdk.net.ExtendedSocketOptions is less of an issue ).

-Chris.

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Alan Bateman]

On 26/04/2016 10:16, Chris Hegarty wrote:

On 26 Apr 2016, at 09:20, Alan Bateman  wrote:


On 25/04/2016 22:01, Chris Hegarty wrote:

One of the remaining open issues in JEP 200 [1] is that the base module
exports the jdk.net package, thereby violating Principle 4 of JEP 200:
a Java SE module must not export any non-SE API packages without
qualification.

http://cr.openjdk.java.net/~chegar/8044773/
https://bugs.openjdk.java.net/browse/JDK-8044773



I took a first pass over this and it looks very good.

Thanks Alan.

  Webrev updated in-place:
   http://cr.openjdk.java.net/~chegar/8044773/jdk/
I took a second pass over it. One thing that I'm wondering about is 
whether BaseExtendedSocketOptions + Support should be collapsed into one 
abstract class ExtendedSocketOptions (or better name) with 3 instance 
methods and 2 static methods. It's an internal interface so not a big 
deal but I think it would be a bit cleaner and allowed the oddly named 
"Support" to go away.


-Alan

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Erik Joelsson]

Thanks, looks good!

/Erik

On 2016-04-26 12:02, Chris Hegarty wrote:

On 26 Apr 2016, at 10:57, Erik Joelsson  wrote:



On 2016-04-26 11:51, Chris Hegarty wrote:

On 26 Apr 2016, at 10:35, Erik Joelsson  wrote:


Hello Chris,

In general it looks good.

Thanks for the review Erik.


Just a couple style [1] nits that I would like to get sorted. In 
Lib-jdk.net.gmk, the arguments to SetupNativeCompilation should be indented 4 
spaces relative to the call (continuation). Also line 32 and 45 needs a space 
after comma.

Got it. I updated the webrev in-place:
  http://cr.openjdk.java.net/~chegar/8044773/jdk/make/lib/Lib-jdk.net.gmk.html

Now if you just move the )) back again on line 43, I'm happy.

Oh sorry, I missed this. Done.
   http://cr.openjdk.java.net/~chegar/8044773/jdk/make/lib/Lib-jdk.net.gmk.html

-Chris.

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Chris Hegarty]

On 26 Apr 2016, at 10:57, Erik Joelsson  wrote:

> 
> 
> On 2016-04-26 11:51, Chris Hegarty wrote:
>> On 26 Apr 2016, at 10:35, Erik Joelsson  wrote:
>> 
>>> Hello Chris,
>>> 
>>> In general it looks good.
>> Thanks for the review Erik.
>> 
>>> Just a couple style [1] nits that I would like to get sorted. In 
>>> Lib-jdk.net.gmk, the arguments to SetupNativeCompilation should be indented 
>>> 4 spaces relative to the call (continuation). Also line 32 and 45 needs a 
>>> space after comma.
>> Got it. I updated the webrev in-place:
>>  http://cr.openjdk.java.net/~chegar/8044773/jdk/make/lib/Lib-jdk.net.gmk.html
> Now if you just move the )) back again on line 43, I'm happy.

Oh sorry, I missed this. Done.
  http://cr.openjdk.java.net/~chegar/8044773/jdk/make/lib/Lib-jdk.net.gmk.html

-Chris.

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Erik Joelsson]

On 2016-04-26 11:57, Erik Joelsson wrote:



On 2016-04-26 11:51, Chris Hegarty wrote:
On 26 Apr 2016, at 10:35, Erik Joelsson  
wrote:



Hello Chris,

In general it looks good.

Thanks for the review Erik.

Just a couple style [1] nits that I would like to get sorted. In 
Lib-jdk.net.gmk, the arguments to SetupNativeCompilation should be 
indented 4 spaces relative to the call (continuation). Also line 32 
and 45 needs a space after comma.

Got it. I updated the webrev in-place:
http://cr.openjdk.java.net/~chegar/8044773/jdk/make/lib/Lib-jdk.net.gmk.html

Now if you just move the )) back again on line 43, I'm happy.

Back as in the same level as the call.

/Erik


/Erik

-Chris.


/Erik

[1] http://openjdk.java.net/groups/build/doc/code-conventions.html

On 2016-04-25 23:01, Chris Hegarty wrote:
One of the remaining open issues in JEP 200 [1] is that the base 
module

exports the jdk.net package, thereby violating Principle 4 of JEP 200:
a Java SE module must not export any non-SE API packages without
qualification.

http://cr.openjdk.java.net/~chegar/8044773/
https://bugs.openjdk.java.net/browse/JDK-8044773

Summary:

  - The jdk.net package has been moved to the jdk.net module. The 
native

  code and implementation that provides support for extended socket
  options, currently just SO_FLOW_SLA, has been moved too. A private
  interface between the base module and the jdk.net module now 
supports

  registration and operation of extended options.

  - The native code has been simplified and cleaned up.

  - Test coverage has been updated to exercise socket options, both 
with

  the jdk.net module present, and absent. The use of the
  jdk.launcher.limitmods system property is temporary until jtreg with
  support for -limitmods is more widely available.

  - Thanks to Erik for helping with build support that now cooperates
  with the intention of the GenModuleInfoSource build tool, to support
  unqualified export additions from non-open code.

-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8051618

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Erik Joelsson]

On 2016-04-26 11:51, Chris Hegarty wrote:

On 26 Apr 2016, at 10:35, Erik Joelsson  wrote:


Hello Chris,

In general it looks good.

Thanks for the review Erik.


Just a couple style [1] nits that I would like to get sorted. In 
Lib-jdk.net.gmk, the arguments to SetupNativeCompilation should be indented 4 
spaces relative to the call (continuation). Also line 32 and 45 needs a space 
after comma.

Got it. I updated the webrev in-place:
  http://cr.openjdk.java.net/~chegar/8044773/jdk/make/lib/Lib-jdk.net.gmk.html

Now if you just move the )) back again on line 43, I'm happy.

/Erik

-Chris.


/Erik

[1] http://openjdk.java.net/groups/build/doc/code-conventions.html

On 2016-04-25 23:01, Chris Hegarty wrote:

One of the remaining open issues in JEP 200 [1] is that the base module
exports the jdk.net package, thereby violating Principle 4 of JEP 200:
a Java SE module must not export any non-SE API packages without
qualification.

http://cr.openjdk.java.net/~chegar/8044773/
https://bugs.openjdk.java.net/browse/JDK-8044773

Summary:

  - The jdk.net package has been moved to the jdk.net module. The native
  code and implementation that provides support for extended socket
  options, currently just SO_FLOW_SLA, has been moved too. A private
  interface between the base module and the jdk.net module now supports
  registration and operation of extended options.

  - The native code has been simplified and cleaned up.

  - Test coverage has been updated to exercise socket options, both with
  the jdk.net module present, and absent. The use of the
  jdk.launcher.limitmods system property is temporary until jtreg with
  support for -limitmods is more widely available.

  - Thanks to Erik for helping with build support that now cooperates
  with the intention of the GenModuleInfoSource build tool, to support
  unqualified export additions from non-open code.

-Chris.

[1] https://bugs.openjdk.java.net/browse/JDK-8051618

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Chris Hegarty]

On 26 Apr 2016, at 10:35, Erik Joelsson  wrote:

> Hello Chris,
> 
> In general it looks good.

Thanks for the review Erik.

> Just a couple style [1] nits that I would like to get sorted. In 
> Lib-jdk.net.gmk, the arguments to SetupNativeCompilation should be indented 4 
> spaces relative to the call (continuation). Also line 32 and 45 needs a space 
> after comma.

Got it. I updated the webrev in-place:
 http://cr.openjdk.java.net/~chegar/8044773/jdk/make/lib/Lib-jdk.net.gmk.html

-Chris.

> /Erik
> 
> [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
> 
> On 2016-04-25 23:01, Chris Hegarty wrote:
>> One of the remaining open issues in JEP 200 [1] is that the base module
>> exports the jdk.net package, thereby violating Principle 4 of JEP 200:
>> a Java SE module must not export any non-SE API packages without
>> qualification.
>> 
>> http://cr.openjdk.java.net/~chegar/8044773/
>> https://bugs.openjdk.java.net/browse/JDK-8044773
>> 
>> Summary:
>> 
>>  - The jdk.net package has been moved to the jdk.net module. The native
>>  code and implementation that provides support for extended socket
>>  options, currently just SO_FLOW_SLA, has been moved too. A private
>>  interface between the base module and the jdk.net module now supports
>>  registration and operation of extended options.
>> 
>>  - The native code has been simplified and cleaned up.
>> 
>>  - Test coverage has been updated to exercise socket options, both with
>>  the jdk.net module present, and absent. The use of the
>>  jdk.launcher.limitmods system property is temporary until jtreg with
>>  support for -limitmods is more widely available.
>> 
>>  - Thanks to Erik for helping with build support that now cooperates
>>  with the intention of the GenModuleInfoSource build tool, to support
>>  unqualified export additions from non-open code.
>> 
>> -Chris.
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8051618
> 

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Chris Hegarty]

On 26 Apr 2016, at 09:20, Alan Bateman  wrote:

> On 25/04/2016 22:01, Chris Hegarty wrote:
>> One of the remaining open issues in JEP 200 [1] is that the base module
>> exports the jdk.net package, thereby violating Principle 4 of JEP 200:
>> a Java SE module must not export any non-SE API packages without
>> qualification.
>> 
>> http://cr.openjdk.java.net/~chegar/8044773/
>> https://bugs.openjdk.java.net/browse/JDK-8044773
>> 
>> 
> I took a first pass over this and it looks very good.

Thanks Alan.

 Webrev updated in-place:
  http://cr.openjdk.java.net/~chegar/8044773/jdk/

> For the package name then sun.net.extnet repeats "net" and maybe sun.net.ext 
> would be better.

Agreed. Done.

> For the BaseExtendedSocketOptions then I think the FileDescriptor parameter 
> should be the first parameter as that is the file descriptor to the socket 
> that the methods operate on.

Agreed. Done.  I had it this way at one point, but I don’t remember why
I changed it.

> In ExtendedSocketOptions then I assume the toString is not needed in 
> checkValueType.

Right. Done. 

> There are "TODOs" in the tests and I assume it would be better to drop those 
> and instead create an issue in JIRA to be addressed once jtreg is promoted 
> with support for -limitmods.

Yes, this is better.
  https://bugs.openjdk.java.net/browse/JDK-8155086

> There is also a TODO in Sockets.java "check this is working ok" that I assume 
> needs attention before this is pushed.

Removed. Sorry, I thought I had removed this after I verified it was indeed
ok.

-Chris.

Re: RFR [9] 8044773: Refactor jdk.net API so that it can be moved out of the base module

[Alan Bateman]

On 25/04/2016 22:01, Chris Hegarty wrote:

One of the remaining open issues in JEP 200 [1] is that the base module
exports the jdk.net package, thereby violating Principle 4 of JEP 200:
a Java SE module must not export any non-SE API packages without
qualification.

http://cr.openjdk.java.net/~chegar/8044773/
https://bugs.openjdk.java.net/browse/JDK-8044773



I took a first pass over this and it looks very good.

For the package name then sun.net.extnet repeats "net" and maybe 
sun.net.ext would be better.


For the BaseExtendedSocketOptions then I think the FileDescriptor 
parameter should be the first parameter as that is the file descriptor 
to the socket that the methods operate on.


In ExtendedSocketOptions then I assume the toString is not needed in 
checkValueType.


There are "TODOs" in the tests and I assume it would be better to drop 
those and instead create an issue in JIRA to be addressed once jtreg is 
promoted with support for -limitmods.


There is also a TODO in Sockets.java "check this is working ok" that I 
assume needs attention before this is pushed.


-Alan