Hi Severin,
thanks for clarification, we can still simplify the ExtendedOptionsImpl.c.
Please have a look on below change and do let me know if it makes sense.
I moved the "#if defined(__linux__) || defined(MACOSX)" inside the
corresponding methods and this way we will eliminate lot's of duplicate
code.
Thanks,
Vyom
diff -r beb15266ba1a src/solaris/native/java/net/ExtendedOptionsImpl.c
--- a/src/solaris/native/java/net/ExtendedOptionsImpl.c Thu Feb 27 19:01:36
2020 +0000
+++ b/src/solaris/native/java/net/ExtendedOptionsImpl.c Mon Jun 29 21:12:16
2020 +0530
@@ -25,6 +25,10 @@
#include <jni.h>
#include <string.h>
+#if defined(__linux__) || defined(MACOSX)
+#include <netinet/tcp.h>
+#include <netinet/in.h>
+#endif
#include "net_util.h"
#include "jdk_net_SocketFlow.h"
@@ -328,9 +332,204 @@
return JNI_FALSE;
}
+// Keep alive options are available for MACOSX and Linux only for
+// the time being.
+#if defined(__linux__) || defined(MACOSX)
+
+// Map socket option level/name to OS specific constant
+#ifdef __linux__
+#define SOCK_OPT_LEVEL SOL_TCP
+#define SOCK_OPT_NAME_KEEPIDLE TCP_KEEPIDLE
+#define SOCK_OPT_NAME_KEEPIDLE_STR "TCP_KEEPIDLE"
+#endif
+#ifdef MACOSX
+#define SOCK_OPT_LEVEL IPPROTO_TCP
+#define SOCK_OPT_NAME_KEEPIDLE TCP_KEEPALIVE
+#define SOCK_OPT_NAME_KEEPIDLE_STR "TCP_KEEPALIVE"
+#endif
+
+static jint socketOptionSupported(jint sockopt) {
+ jint one = 1;
+ jint rv, s;
+ s = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
+ if (s < 0) {
+ return 0;
+ }
+ rv = setsockopt(s, SOCK_OPT_LEVEL, sockopt, (void *) &one, sizeof
(one));
+ if (rv != 0 && errno == ENOPROTOOPT) {
+ rv = 0;
+ } else {
+ rv = 1;
+ }
+ close(s);
+ return rv;
+}
+
+static void handleError(JNIEnv *env, jint rv, const char *errmsg) {
+ if (rv < 0) {
+ if (errno == ENOPROTOOPT) {
+ JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
+ "unsupported socket option");
+ } else {
+ JNU_ThrowByNameWithLastError(env, "java/net/SocketException",
errmsg);
+ }
+ }
+}
+
+#endif /* __linux__ || MACOSX*/
+
#endif /* __solaris__ */
JNIEXPORT jboolean JNICALL Java_sun_net_ExtendedOptionsImpl_flowSupported
(JNIEnv *env, jclass UNUSED) {
return flowSupported0();
}
+
+/*
+ * Class: sun_net_ExtendedOptionsImpl
+ * Method: keepAliveOptionsSupported
+ * Signature: ()Z
+ */
+JNIEXPORT jboolean JNICALL
Java_sun_net_ExtendedOptionsImpl_keepAliveOptionsSupported
+(JNIEnv *env, jobject unused) {
+ #if defined(__linux__) || defined(MACOSX)
+ return socketOptionSupported(SOCK_OPT_NAME_KEEPIDLE) &&
socketOptionSupported(TCP_KEEPCNT)
+ && socketOptionSupported(TCP_KEEPINTVL);
+ #else
+ return JNI_FALSE;
+}
+
+/*
+ * Class: sun_net_ExtendedOptionsImpl
+ * Method: setTcpKeepAliveProbes
+ * Signature: (Ljava/io/FileDescriptor;I)V
+ */
+JNIEXPORT void JNICALL
Java_sun_net_ExtendedOptionsImpl_setTcpKeepAliveProbes
+(JNIEnv *env, jobject unused, jobject fileDesc, jint optval) {
+ #if defined(__linux__) || defined(MACOSX)
+ int fd = getFD(env, fileDesc);
+ if (fd < 0) {
+ NET_ERROR(env, JNU_JAVANETPKG "SocketException", "socket
closed");
+ return;
+ } else {
+ jint rv = setsockopt(fd, SOCK_OPT_LEVEL, TCP_KEEPCNT, &optval,
sizeof (optval));
+ handleError(env, rv, "set option TCP_KEEPCNT failed");
+ }
+ #else
+ JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
"unsupported socket option");
+ #endif
+}
+
+/*
+ * Class: sun_net_ExtendedOptionsImpl
+ * Method: setTcpKeepAliveTime
+ * Signature: (Ljava/io/FileDescriptor;I)V
+ */
+JNIEXPORT void JNICALL Java_sun_net_ExtendedOptionsImpl_setTcpKeepAliveTime
+(JNIEnv *env, jobject unused, jobject fileDesc, jint optval) {
+ #if defined(__linux__) || defined(MACOSX)
+ int fd = getFD(env, fileDesc);
+ if (fd < 0) {
+ NET_ERROR(env, JNU_JAVANETPKG "SocketException", "socket
closed");
+ return;
+ } else {
+ jint rv = setsockopt(fd, SOCK_OPT_LEVEL,
SOCK_OPT_NAME_KEEPIDLE, &optval, sizeof (optval));
+ handleError(env, rv, "set option " SOCK_OPT_NAME_KEEPIDLE_STR
" failed");
+ }
+ #else
+ JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
"unsupported socket option");
+ #endif
+}
+
+/*
+ * Class: sun_net_ExtendedOptionsImpl
+ * Method: setTcpKeepAliveIntvl
+ * Signature: (Ljava/io/FileDescriptor;I)V
+ */
+JNIEXPORT void JNICALL
Java_sun_net_ExtendedOptionsImpl_setTcpKeepAliveIntvl
+(JNIEnv *env, jobject unused, jobject fileDesc, jint optval) {
+ #if defined(__linux__) || defined(MACOSX)
+ int fd = getFD(env, fileDesc);
+ if (fd < 0) {
+ NET_ERROR(env, JNU_JAVANETPKG "SocketException", "socket
closed");
+ return;
+ } else {
+ jint rv = setsockopt(fd, SOCK_OPT_LEVEL, TCP_KEEPINTVL,
&optval, sizeof (optval));
+ handleError(env, rv, "set option TCP_KEEPINTVL failed");
+ }
+ #else
+ JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
"unsupported socket option");
+ #endif
+}
+
+/*
+ * Class: sun_net_ExtendedOptionsImpl
+ * Method: getTcpKeepAliveProbes
+ * Signature: (Ljava/io/FileDescriptor;)I
+ */
+JNIEXPORT jint JNICALL
Java_sun_net_ExtendedOptionsImpl_getTcpKeepAliveProbes
+(JNIEnv *env, jobject unused, jobject fileDesc) {
+ #if defined(__linux__) || defined(MACOSX)
+ int fd = getFD(env, fileDesc);
+ if (fd < 0) {
+ NET_ERROR(env, JNU_JAVANETPKG "SocketException", "socket
closed");
+ return -1;
+ } else {
+ jint optval, rv;
+ socklen_t sz = sizeof (optval);
+ rv = getsockopt(fd, SOCK_OPT_LEVEL, TCP_KEEPCNT, &optval, &sz);
+ handleError(env, rv, "get option TCP_KEEPCNT failed");
+ return optval;
+ }
+ #else
+ JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
"unsupported socket option");
+ #endif
+}
+
+/*
+ * Class: sun_net_ExtendedOptionsImpl
+ * Method: getTcpKeepAliveTime
+ * Signature: (Ljava/io/FileDescriptor;)I
+ */
+JNIEXPORT jint JNICALL Java_sun_net_ExtendedOptionsImpl_getTcpKeepAliveTime
+(JNIEnv *env, jobject unused, jobject fileDesc) {
+ #if defined(__linux__) || defined(MACOSX)
+ int fd = getFD(env, fileDesc);
+ if (fd < 0) {
+ NET_ERROR(env, JNU_JAVANETPKG "SocketException", "socket
closed");
+ return -1;
+ } else {
+ jint optval, rv;
+ socklen_t sz = sizeof (optval);
+ rv = getsockopt(fd, SOCK_OPT_LEVEL, SOCK_OPT_NAME_KEEPIDLE,
&optval, &sz);
+ handleError(env, rv, "get option " SOCK_OPT_NAME_KEEPIDLE_STR
" failed");
+ return optval;
+ }
+ #else
+ JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
"unsupported socket option");
+ #endif
+}
+
+/*
+ * Class: sun_net_ExtendedOptionsImpl
+ * Method: getTcpKeepAliveIntvl
+ * Signature: (Ljava/io/FileDescriptor;)I
+ */
+JNIEXPORT jint JNICALL
Java_sun_net_ExtendedOptionsImpl_getTcpKeepAliveIntvl
+(JNIEnv *env, jobject unused, jobject fileDesc) {
+ #if defined(__linux__) || defined(MACOSX)
+ int fd = getFD(env, fileDesc);
+ if (fd < 0) {
+ NET_ERROR(env, JNU_JAVANETPKG "SocketException", "socket
closed");
+ return -1;
+ } else {
+ jint optval, rv;
+ socklen_t sz = sizeof (optval);
+ rv = getsockopt(fd, SOCK_OPT_LEVEL, TCP_KEEPINTVL, &optval,
&sz);
+ handleError(env, rv, "get option TCP_KEEPINTVL failed");
+ return optval;
+ }
+ #else
+ JNU_ThrowByName(env, "java/lang/UnsupportedOperationException",
"unsupported socket option");
+ #endif
+}
On Mon, Jun 29, 2020 at 5:27 PM Severin Gehwolf <sgehw...@redhat.com> wrote:
> Hi Vyom,
>
> On Mon, 2020-06-29 at 17:11 +0530, Vyom Tiwari wrote:
> > Hi Severin,
> >
> > Latest code looks good
>
> Thanks!
>
> > I think in src/solaris/native/java/net/ExtendedOptionsImpl.c, you
> > don't need #ifdef blocks. In case of "flowOption" it was required
> > because flow is specific to Solaris.
> >
> > In case of KEEPALIVE we are using the POSIX
> > api(setsockopt/getsockopt) which exists on all POSIX OS's. If a OS
> > does not support KEEPALIVE then
> > Java_sun_net_ExtendedOptionsImpl_keepAliveOptionsSupported will
> > return false and #else will never execute.
> > For Windows we have different files and for all other platforms same
> > code will work without explicit #ifdef.
>
> Not quite.
>
> For example, on MACOSX some macros have a diferent name than on Linux.
> Thus, the ifdef magic to work around that. I don't have any insight as
> to what they're called on, say, solaris or aix, or, if they're
> different at all. Anyway, I'd rely on somebody else doing the testing.
> Without that, I don't think we can add this to other platforms and
> potentially break them. Support for them could get added later if
> somebody with the relevant systems steps up to do it.
>
> > Please do let me know if i am missing something.
>
> FWIW, those options are only enabled on Linux/Mac for JDK 11u and
> jdk/jdk. Those changes would have to be done there first and then
> backported.
>
> Thanks,
> Severin
>
> >
> > On Mon, Jun 29, 2020 at 2:25 PM Severin Gehwolf <sgehw...@redhat.com>
> > wrote:
> > > Hi Vyom!
> > >
> > > On Fri, 2020-06-26 at 15:55 +0530, Vyom Tiwari wrote:
> > > > Hi Severin,
> > > >
> > > > Overall code changes looks ok to me, I build & tested at my local
> > > REL
> > > > it worked fine for me.
> > >
> > > Thanks for the review!
> > >
> > > > Below are few minor comments.
> > > >
> > > > 1-> Net.java
> > > > 1.1-> I think you don't need to explicitly convert value to
> > > integer and then pass it. You can avoid the local int variable
> > > creation as follows.
> > > > ExtendedOptionsImpl.setTcpKeepAliveIntvl(fd,
> > > (Integer)value);
> > > > 1.2-> In getSocketOption you don't need to explicitly cast it
> > > to Integer.
> > > > return ExtendedOptionsImpl.getTcpKeepAliveIntvl(fd);
> > > > 2-> PlainSocketImpl.java
> > > > 1.1 -> In setOption(SocketOption<T> name, T value) you can
> > > avoid the local int variable.
> > >
> > > Should all be fixed.
> > >
> > > > 3-> Any specific reason for two set of functions
> > > "setTcpKeepAliveTime0 and setTcpKeepAliveTime" for all three socket
> > > options ?
> > >
> > > Not really, other than keeping the backport aligned to the JDK 11u
> > > patch. I've updated it in the latest webrev:
> > >
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8194298/jdk8/05/webrev/
> > >
> > > Thanks,
> > > Severin
> > >
> > >
> > > > Thanks,
> > > > Vyom
> > > >
> > > > On Fri, Jun 26, 2020 at 1:08 PM Severin Gehwolf <
> > > sgehw...@redhat.com> wrote:
> > > > > Hi,
> > > > >
> > > > > On Thu, 2020-06-25 at 23:55 +0000, Bernd Eckenfels wrote:
> > > > > > This would be a great addition.
> > > > >
> > > > > Thanks.
> > > > >
> > > > > > I do not understand why it does not support the options
> > > available for
> > > > > > Windows. Especially given the fact that it actually
> > > implements 6
> > > > > > native methods to print "Unsupported".
> > > > > >
> > > > > > But I guess that's less a question to the backport and more
> > > to the
> > > > > > general implementation.
> > > > >
> > > > > Yes, indeed. This should be brought up for discussion in JDK
> > > head first
> > > > > and implemented there before we can consider a backport.
> > > > >
> > > > > Thanks,
> > > > > Severin
> > > > >
> > > > >
> > > >
> > > >
> > >
> >
> >
>
>
--
Thanks,
Vyom
