RE: RFR 15 8243099: SO_INCOMING_NAPI_ID support

[Ivanov, Vladimir A]

Thanks a lot Daniel! I missed these double checks.
Updated webrev may be reviewed  as 
http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.14 
I use only one condition for the 'if' in the 'startup' method while kernel 
should support or not both types of sockets together.

 Thanks, Vladimir

-----Original Message-----
From: Daniel Fuchs <daniel.fu...@oracle.com> 
Sent: Thursday, May 14, 2020 2:37 AM
To: Ivanov, Vladimir A <vladimir.a.iva...@intel.com>; Patrick Concannon 
<patrick.concan...@oracle.com>; Alan Bateman <alan.bate...@oracle.com>; OpenJDK 
Network Dev list <net-dev@openjdk.java.net>
Subject: Re: RFR 15 8243099: SO_INCOMING_NAPI_ID support

Hi Vladimir, Patrick,

The new tests look very good. There's just a small issue I noticed.
It's present in all the new tests - here is an example on
AsynchronousSocketChannelNAPITest.java:

This code in @BeforeTest:

   63         try (var s = AsynchronousSocketChannel.open()) {
   64             if (!s.supportedOptions().contains(SO_INCOMING_NAPI_ID))
   65                 throw new SkippedException("NAPI ID not supported 
on this system");
   66         }

will prevent this code in @Test

   77             } else {
   78                 assertThrows(UOE, () -> 
sc.getOption(SO_INCOMING_NAPI_ID));
   79                 assertThrows(UOE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, 42));
   80                 assertThrows(UOE, () -> 
sc.setOption(SO_INCOMING_NAPI_ID, null));
   81             }


from ever being run. So I would suggest moving that check (lines 77-81 and 
92-96) into the @BeforeTest method as below, and simply assume that the option 
is supported when you reach the @Test:

   60     @BeforeTest
   61     public void setup() throws IOException {
   62         IPSupport.throwSkippedExceptionIfNonOperational();
   63         try (var s = AsynchronousSocketChannel.open()) {
                  if (!s.supportedOptions().contains(SO_INCOMING_NAPI_ID)) {
                      assertThrows(UOE, () -> 
sc.getOption(SO_INCOMING_NAPI_ID));
                      assertThrows(UOE, () -> sc.setOption(SO_INCOMING_NAPI_ID, 
42));
                      assertThrows(UOE, () -> sc.setOption(SO_INCOMING_NAPI_ID, 
null));

   65                 throw new SkippedException("NAPI ID not supported 
on this system");
   66         }
   67         hostAddr = InetAddress.getLocalHost();
   68     }

Then the @Test can be simplified as follows:

   70     @Test
   71     public void testSetGetOptionSocketChannel() throws IOException {
   72         try (var sc = AsynchronousSocketChannel.open()) {
                  assertEquals((int) sc.getOption(SO_INCOMING_NAPI_ID), 0);
                  assertThrows(SE, () -> sc.setOption(SO_INCOMING_NAPI_ID, 42));
                  assertThrows(IAE, () -> sc.setOption(SO_INCOMING_NAPI_ID, 
null));
   82         }
   83     }

best regards,

-- daniel

On 13/05/2020 21:50, Ivanov, Vladimir A wrote:
> Thanks a lot. The updated webrev available as 
> http://cr.openjdk.java.net/~sviswanathan/Vladimir/8243099/webrev.13/
> 
> Thanks, Vladimir