Thanks for your review. yes, the SocketException looks better here than the IOE. The updated webrev available as http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.08/
Also 'positive' was added to Javadoc to describe napi id. Could you describe the testing scenario that should be covered? Now this test have: - simple check (zero for non-initialized and exception for 'set' option for the ServerSocket/Socket/DatagramSocket). - server/client testing for the ServerSocket/Socket; - send/receive testing for the DatagramSocket. The simple check may be easily extended to channels. The server/client testing require special classes that increase the testing size and reduce maintainability. Thanks, Vladimir -----Original Message----- From: Alan Bateman <alan.bate...@oracle.com> Sent: Tuesday, May 5, 2020 1:08 AM To: Ivanov, Vladimir A <vladimir.a.iva...@intel.com>; OpenJDK Network Dev list <net-dev@openjdk.java.net> Subject: Re: RFR 15 8243099: SO_INCOMING_NAPI_ID support On 04/05/2020 22:21, Ivanov, Vladimir A wrote: > Thanks for your comments. > > Updated version of the patch available as > http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.07/ > The exception part for the setOption method was updated > + } else if (option == SO_INCOMING_NAPI_ID) { > + if (!incomingNapiIdOptSupported) > + throw new UnsupportedOperationException("Attempt to > set unsupported option " + option); > + else > + throw new IOException("Attempt to set read > + only option " + option); > > It required to update the method's description. As side effect some > application may report compile error for the 'setOption' method usage while > one more exception should be added to processing. > > The Javadoc and the ExtOptionNAPITest.java were updated to use IOE instead of > UOE. > > Tests "test/jdk/jdk/net/Sockets > test/jdk/java/net/SocketOption/AfterClose.java > test/jdk/java/nio/channels/etc/PrintSupportedOptions.java" passed on > the > RHEL8 (with NAPI support) and the RHEL7.7 (without NAPI support). A SocketException is an IOException so no need for both in the setOption declaration. The method is internal so it's not going to impact anyone using any of the supported APIs. That said, it might be more consistent to throw SocketException here and that will avoid needing to change the declarations. So assuming s/IOException/SocketException/ then I think the javadoc looks okay. A residual question from the discussion is whether you want to make it clear that the identifier is a positive Integer. Once we have the javadoc agreed here then you should get the CSR submitted. I don't think ExtOptionNAPITest is ready for review. I think we need a simpler test that is reliable and easy to maintain. It will need to exercise each of the network channels (defined in java.nio.channels) in addition to the socket APIs defined in java.net. It is important that the listener binds to an ephemeral port as we can't used fixed ports in tests. It's also important that all tests do cleanup (the current ExtOptionNAPITest leaves many sockets open). We can iterate on the test here in parallel with the CSR. -Alan
