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
