Hi Vladimir, changes looks much better now, i still see that function(IncomingNapiIdSupported) & variable(IncomingNapiIdOptSupported) does not follow Java naming convention.
private static final boolean IncomingNapiIdOptSupported =+ platformSocketOptions.IncomingNapiIdSupported(); Thanks, Vyom On Tue, Apr 28, 2020 at 3:36 AM Ivanov, Vladimir A < vladimir.a.iva...@intel.com> wrote: > 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 > -- Thanks, Vyom
