Hi Yyom,
On Mon, 2020-06-29 at 21:18 +0530, Vyom Tiwari wrote:
> 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.
It's a possibility. IMHO, it doesn't really make the code easier to
read, though. Some duplication for clarity seems OK to me in this case.
I'm not too fond of over-use of ifdef so I'd rather keep it at v5.
YMMV.
Let's see what others think.
Thanks,
Severin
> 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
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > >
> > >
> > >
> >
>
>
