Thanks for the comments. The webrev was updated: http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.02/
Only the code was changed here. Changes in the documentation will be discussed separately. Changes: 1. The LinuxSocketOptions.c was updated according to Vyom comment. Note, the 'socketOptionSupported' implemented through the "setsockopt" call that always return error for the read only properties. It was updated to use 'getsockopt'. The tests set 'test/jdk/java/net test/jdk/java/nio/channels test/jdk/javax/net test/jdk/jdk/net test/jdk/sun/net' have same run results on RHEL7.7 as clean jdk/jdk workspace. Please, notify me if testing should be extended. 2. The 'IncomingNapiIdSupported0' was renamed to the 'incomingNapiIdSupported0.' 3. The test 'PrintSupportedOptions' was updated to use Set<String> for read only options. 4. The test ExtOptionNAPITest.java was added. Now the test 'ExtOptionTest.java' do nothing with napi id. Thanks, Vladimir -----Original Message----- From: Alan Bateman <alan.bate...@oracle.com> Sent: Friday, April 24, 2020 12:09 AM To: Ivanov, Vladimir A <vladimir.a.iva...@intel.com>; OpenJDK Network Dev list <net-dev@openjdk.java.net> Subject: Re: RFR 15 8243099: Adding ADQ support to JDK On 23/04/2020 20:11, Ivanov, Vladimir A wrote: > Thanks a lot to Chris and Alan for detailed comments. > The updated version of patch available at > http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.01/ > > Changes: > 1. in native code the common pattern was used for the 'getsockopt' call. > 2. condition to define SO_INCOMING_NAPI_ID was added. > 3. the DatagarmSocket was added to the ExtOptionTest 4. testing on my > side was extended to the subset 'test/jdk/java/net test/jdk/java/nio/channels > test/jdk/javax/net test/jdk/jdk/net test/jdk/sun/net'. > Results are same for the patched and non-patched builds on the RHEL7.7 OS. > Tests test/jdk/java/net/SocketOption/AfterClose.java and > test/jdk/java/nio/channels/etc/PrintSupportedOptions.java were updated to > support read only properties. > 5. description for the NAPI_ID was updated 6. the > UnsupportedOperationException was added to the 'setOption' call for the > 'SO_INCOMING_NAPI_ID'. > (Dropping core-libs-dev as net-dev is the more appropriate list for this area). Thanks for the update. The updated javadoc looks better but will need a few iterations. It would be good if it could start by explaining what the value of the socket option is, e.g. "The value of this socket option is an Integer representing the network device queue ...". I think it needs to be clearer on the types of sockets that support this option, does it support both stream-oriented and datagram-oriented sockets? Does it return a value when invoked on a ServerSocketChannel? Instead of @throws, the text will need to that attempting to set the socket option will cause UOE to be thrown. Once the javadoc is agreed then the CSR can be submitted and the code review can continue in parallel. The implementation changes mostly look okay although IncomingNapiIdSupported0 should probably be renamed to start with lower case "i". Also someone might need to check that you can create an IPv4 socket when a system is configured as an IPv6-only system. Tests. I think the the new tests in ExtOptionTest will need closer examination as I can't tell how reliable they are. It might be that it needs a completely new test. The update to the NIO PrintSupportedOptions test define READ_ONLY_OPTS as Set<String>. -Alan
