Hi Vladimir,
In LinuxSocketOptions.c we have lot's of duplicate code, can you please
reuse "socketOptionSupported" & "handleError" as follows, this we remove
lot's of duplicate code.
Thanks,
Vyom
diff -r b6b4506a7827 src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
--- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c Fri Apr 24
15:28:57 2020 +0800
+++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c Fri Apr 24
13:37:36 2020 +0530
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2017, 2018, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -33,6 +33,10 @@
#include "jni_util.h"
#include "jdk_net_LinuxSocketOptions.h"
+#ifndef SO_INCOMING_NAPI_ID
+#define SO_INCOMING_NAPI_ID 56
+#endif
+
/*
* Class: jdk_net_LinuxSocketOptions
* Method: setQuickAck
@@ -44,15 +48,7 @@
int rv;
optval = (on ? 1 : 0);
rv = setsockopt(fd, SOL_SOCKET, TCP_QUICKACK, &optval, sizeof
(optval));
- if (rv < 0) {
- if (errno == ENOPROTOOPT) {
- JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
- "unsupported socket option");
- } else {
- JNU_ThrowByNameWithLastError(env, "java/net/SocketException",
- "set option TCP_QUICKACK failed");
- }
- }
+ handleError(env, rv, "set option TCP_QUICKACK failed");
}
/*
@@ -65,15 +61,7 @@
int on;
socklen_t sz = sizeof (on);
int rv = getsockopt(fd, SOL_SOCKET, TCP_QUICKACK, &on, &sz);
- if (rv < 0) {
- if (errno == ENOPROTOOPT) {
- JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
- "unsupported socket option");
- } else {
- JNU_ThrowByNameWithLastError(env, "java/net/SocketException",
- "get option TCP_QUICKACK failed");
- }
- }
+ handleError(env, rv, "get option TCP_QUICKACK failed");
return on != 0;
}
@@ -83,31 +71,18 @@
* Signature: ()Z
*/
JNIEXPORT jboolean JNICALL
Java_jdk_net_LinuxSocketOptions_quickAckSupported0
-(JNIEnv *env, jobject unused) {
- int one = 1;
- int rv, s;
- s = socket(PF_INET, SOCK_STREAM, 0);
- if (s < 0) {
- return JNI_FALSE;
- }
- rv = setsockopt(s, SOL_SOCKET, TCP_QUICKACK, (void *) &one, sizeof
(one));
- if (rv != 0 && errno == ENOPROTOOPT) {
- rv = JNI_FALSE;
- } else {
- rv = JNI_TRUE;
- }
- close(s);
- return rv;
+(JNIEnv *env, jobject unused) {
+ return socketOptionSupported(SOL_SOCKET, TCP_QUICKACK);
}
-static jint socketOptionSupported(jint sockopt) {
+static jint socketOptionSupported(jint level, jint optname) {
jint one = 1;
jint rv, s;
s = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
if (s < 0) {
return 0;
}
- rv = setsockopt(s, SOL_TCP, sockopt, (void *) &one, sizeof (one));
+ rv = setsockopt(s, level, optname, (void *) &one, sizeof (one));
if (rv != 0 && errno == ENOPROTOOPT) {
rv = 0;
} else {
@@ -135,8 +110,8 @@
*/
JNIEXPORT jboolean JNICALL
Java_jdk_net_LinuxSocketOptions_keepAliveOptionsSupported0
(JNIEnv *env, jobject unused) {
- return socketOptionSupported(TCP_KEEPIDLE) &&
socketOptionSupported(TCP_KEEPCNT)
- && socketOptionSupported(TCP_KEEPINTVL);
+ return socketOptionSupported(SOL_TCP, TCP_KEEPIDLE) &&
socketOptionSupported(SOL_TCP, TCP_KEEPCNT)
+ && socketOptionSupported(SOL_TCP, TCP_KEEPINTVL);
}
/*
@@ -213,3 +188,27 @@
handleError(env, rv, "get option TCP_KEEPINTVL failed");
return optval;
}
+
+/*
+ * Class: jdk_net_LinuxSocketOptions
+ * Method: IncomingNapiIdSupported0
+ * Signature: (I)I;
+ */
+JNIEXPORT jboolean JNICALL
Java_jdk_net_LinuxSocketOptions_IncomingNapiIdSupported0
+(JNIEnv *env, jobject unused) {
+ return socketOptionSupported(SOL_SOCKET, SO_INCOMING_NAPI_ID);
+}
+
+/*
+ * Class: jdk_net_LinuxSocketOptions
+ * Method: getIncomingNapiId0
+ * Signature: (I)I;
+ */
+JNIEXPORT jint JNICALL Java_jdk_net_LinuxSocketOptions_getIncomingNapiId0
+(JNIEnv *env, jobject unused, jint fd) {
+ jint optval, rv;
+ socklen_t sz = sizeof (optval);
+ rv = getsockopt(fd, SOL_SOCKET, SO_INCOMING_NAPI_ID, &optval, &sz);
+ handleError(env, rv, "get option SO_INCOMING_NAPI_ID failed");
+ return optval;
+}
On Fri, Apr 24, 2020 at 12:43 AM Ivanov, Vladimir A <
vladimir.a.iva...@intel.com> 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'.
>
> Thanks, Vladimir
>
> -----Original Message-----
> From: Chris Hegarty <chris.hega...@oracle.com>
> Sent: Tuesday, April 21, 2020 7:29 AM
> To: Ivanov, Vladimir A <vladimir.a.iva...@intel.com>; core-libs-dev <
> core-libs-...@openjdk.java.net>; net >> OpenJDK Network Dev list <
> net-dev@openjdk.java.net>
> Subject: Re: RFR 15 8243099: Adding ADQ support to JDK
>
> Vladimir,
>
> > On 18 Apr 2020, at 00:13, Ivanov, Vladimir A <
> vladimir.a.iva...@intel.com> wrote:
> >
> > Hello everyone,
> > I would like to add support for the Application Device Queue (ADQ)
> technology to the JDK.
> >
> > The JBS entry and webrev were created for this improvement:
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8243099
> > Webrev:
>
> Here is an incomplete set of specific comments relating to the webrev.
>
> 1) I get a compilation error on my Linux Ubuntu 18.04 ( admittedly my gcc
> version may be more recent/strict than that of the one you built with ):
>
> $ gcc --version
> gcc (Ubuntu 8.3.0-6ubuntu1~18.10) 8.3.0
> Copyright (C) 2018 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is
> NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR
> PURPOSE.
>
> Compilation error:
>
> /home/chhegar/repos/jdk/adq/jdk/open/src/
> jdk.net/linux/native/libextnet/LinuxSocketOptions.c: In function
> 'Java_jdk_net_LinuxSocketOptions_IncomingNapiIdSupported0':
> /home/chhegar/repos/jdk/adq/jdk/open/src/
> jdk.net/linux/native/libextnet/LinuxSocketOptions.c:231:72: error:
> passing argument 5 of 'getsockopt' makes pointer from integer without a
> cast [-Werror=int-conversion]
> 231 | rv = getsockopt(s, SOL_SOCKET, SO_INCOMING_NAPI_ID, (void *)
> &one, sizeof (one));
> |
> ^~~~~~~~~~~~
> |
> |
> |
> long unsigned int
> In file included from /home/chhegar/repos/jdk/adq/jdk/open/src/
> jdk.net/linux/native/libextnet/LinuxSocketOptions.c:25:
> /var/tmp/jib-chhegar/install/jpg/infra/builddeps/devkit-linux_x64/gcc9.2.0-OL6.4+1.0/devkit-linux_x64-gcc9.2.0-OL6.4+1.0.tar.gz/x86_64-linux-gnu-to-x86_64-linux-gnu/x86_64-linux-gnu/sysroot/usr/include/sys/socket.h:192:32:
> note: expected 'socklen_t * restrict' {aka 'unsigned int * restrict'} but
> argument is of type 'long unsigned int'
> 192 | socklen_t *__restrict __optlen) __THROW;
> | ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
> cc1: all warnings being treated as errors
>
> Simple fix:
>
> diff --git a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
> b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
> --- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
> +++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
> @@ -224,11 +224,12 @@
> ( JNIEnv *env, jobject unused) {
> int one = 1;
> int rv, s;
> + socklen_t sz = sizeof (one);
> s = socket(PF_INET, SOCK_STREAM, 0);
> if (s < 0) {
> return JNI_FALSE;
> }
> - rv = getsockopt(s, SOL_SOCKET, SO_INCOMING_NAPI_ID, (void *) &one,
> sizeof (one));
> + rv = getsockopt(s, SOL_SOCKET, SO_INCOMING_NAPI_ID, (void *) &one,
> &sz);
> if (rv != 0 && errno == ENOPROTOOPT) {
> rv = JNI_FALSE;
> } else {
>
>
> 2) test/jdk/java/net/SocketOption/AfterClose.java fails
>
> This test iterates over all socket options reported to be supported by a
> particular socket, and asserts expected behaviour after the socket has been
> closed. Clearly, this new option results in unexpected after-close
> behaviour.
>
> One example failed scenario output:
> test AfterClose.closedBoundDatagramSocket(SO_INCOMING_NAPI_ID, null):
> failure
> java.lang.NullPointerException
> at AfterClose.closedBoundDatagramSocket(AfterClose.java:248)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
> Method)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:564)
> at
> org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:85)
> at org.testng.internal.Invoker.invokeMethod(Invoker.java:639)
> at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:821)
> at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1131)
> at
> org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:124)
> at
> org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
> at org.testng.TestRunner.privateRun(TestRunner.java:773)
> at org.testng.TestRunner.run(TestRunner.java:623)
> at org.testng.SuiteRunner.runTest(SuiteRunner.java:357)
> at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:352)
> at org.testng.SuiteRunner.privateRun(SuiteRunner.java:310)
> at org.testng.SuiteRunner.run(SuiteRunner.java:259)
> at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
> at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
> at org.testng.TestNG.runSuitesSequentially(TestNG.java:1185)
> at org.testng.TestNG.runSuitesLocally(TestNG.java:1110)
> at org.testng.TestNG.run(TestNG.java:1018)
> at
> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
> Method)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:564)
> at
> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
> at java.base/java.lang.Thread.run(Thread.java:832)
>
> 3) Interestingly, should this option be supported for datagram sockets too
> ( as the prior test output seems to suggest ).
>
> 4) test/jdk/java/nio/channels/etc/PrintSupportedOptions.java fails
>
> Example output:
> ----------System.err:(18/1162)----------
> java.lang.InternalError: Unexpected option SO_INCOMING_NAPI_ID
> at
> jdk.net/jdk.net.ExtendedSocketOptions$1.setOption(ExtendedSocketOptions.java:250)
> at java.base/sun.nio.ch.Net.setSocketOption(Net.java:349)
> at java.base/sun.nio.ch.Net.setSocketOption(Net.java:335)
> at java.base/sun.nio.ch
> .SocketChannelImpl.setOption(SocketChannelImpl.java:241)
> at java.base/sun.nio.ch
> .SocketChannelImpl.setOption(SocketChannelImpl.java:67)
> at PrintSupportedOptions.test(PrintSupportedOptions.java:66)
> at PrintSupportedOptions.main(PrintSupportedOptions.java:49)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
> Method)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> at
> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:564)
> at
> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
> at java.base/java.lang.Thread.run(Thread.java:832)
>
> 5) Optionally define SO_INCOMING_NAPI_ID
>
> diff --git a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
> b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
> --- a/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
> +++ b/src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
> @@ -33,7 +33,10 @@
> #include "jni_util.h"
> #include "jdk_net_LinuxSocketOptions.h"
>
> -#define SO_INCOMING_NAPI_ID 56
> +#ifndef SO_INCOMING_NAPI_ID
> +#define SO_INCOMING_NAPI_ID 56
> +#endif
> +
> /*
> * Class: jdk_net_LinuxSocketOptions
> * Method: setQuickAck
>
>
> That’s all for now.
>
> -Chris.
>
>
--
Thanks,
Vyom
