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