Hi Severin, Latest code looks good, 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. Please do let me know if i am missing something. Thanks, Vyom 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
