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

[Severin Gehwolf]

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