[8u] RFR(m): 8194298: Add support for per Socket configuration of TCP keepalive

[Vyom Tiwari]

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 <sgehwolf at 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 <sgehwolf at 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