On Mon, Jan 4, 2016 at 8:02 PM, Lu, Yingqi <yingqi...@intel.com> wrote: > Hi Volker, > > Happy New Year and welcome back! Based on the feedback from you and Alan, > here is what I plan to do. Please let me know if you have any concerns. > > 1. I will make HPUX, MACOSX and AIX as known and set them to 0x0200. As > previous, Linux is set to 15 and Solaris is set to 0x100. The rest of the > unix based OSes will be treated as unknown and will be set to 0. Windows will > be set to 0 as well. > > 2. Regarding to the code snippet in net_util_md.c, the reason I check for > ENOPROTOOPT is to enable SO_REUSEPORT in the situation that the feature is > supported but something else happens during setsocksopts call. I totally > agree this is much less safer than just disable the feature whenever the > setsockopts returns error. Alan, please let me know what your take on this > is. I can quick change it if you both think it is more appropriate to remove > the ENOPROTOOPT check. >
I just think we should play safe here. You also return JNI_FALSE when the call to socket() just before the one to setsockopt() fails. But lets here what's Alan's opinion. > Again, really appreciate the quick responses and helpful feedback from both > of you :-) > > Thanks, > Lucy > > -----Original Message----- > From: Volker Simonis [mailto:volker.simo...@gmail.com] > Sent: Monday, January 04, 2016 10:43 AM > To: Alan Bateman <alan.bate...@oracle.com> > Cc: Lu, Yingqi <yingqi...@intel.com>; Kaczmarek, Eric > <eric.kaczma...@intel.com>; Viswanathan, Sandhya > <sandhya.viswanat...@intel.com>; Kharbas, Kishor <kishor.khar...@intel.com>; > Aundhe, Shirish <shirish.aun...@intel.com>; net-dev@openjdk.java.net > Subject: Re: Patch for adding SO_REUSEPORT socket option > > Hi Lucy, Alan, > > I'm back from vacation so here we go :) > > On Fri, Jan 1, 2016 at 12:18 PM, Alan Bateman <alan.bate...@oracle.com> wrote: >> >> On 21/12/2015 17:53, Lu, Yingqi wrote: >>> >>> Hi All, >>> >>> Sorry for the late reply. Here is the link to the most recent >>> reversion of the patch (version #6). >>> http://cr.openjdk.java.net/~mcberg/jdk/6432031/webrev.06/ >>> >>> In this version, we have done following modifications based on the feedback >>> we received. Please review and let us know if there is anything missing. >>> Thank you very much! >>> >>> >> I went through the latest webrev and it is look quite good. >> >> One thing that doesn't seem quite right is SocketImpl::isReusePortAvailable. >> Whether SO_REUSEPORT is supported or not depends on the concrete >> implementation of the SocketImpl. So I think this needs to be moved down to >> AbstractPlainSocketImpl. >> >> In net_util_md.h then SO_REUSEPORT is defined as 0x0200 on "unknown" >> platforms. I wonder if it would be better to not set it and instead have >> reuseport_supported return JNI_FALSE when SO_REUSEPORT is not defined. >> >> >> StandardSocketOptions.SO_REUSEPORT is referenced in shared code >> (sun/nio/ch/*Impl.java) so shouldn't it be defined to something on all >> platforms? I ask because genSocketOptionRegistry.c open emits a definition >> on non-Windows builds. Maybe it needs to emit it with a value of 0 on >> Windows. Also like net_util_md.c then maybe it should be emitted as 0 rather >> than 0x0200 when the value is not known. >> > > Alan, what do you mean by "unknown" platform? Currently, as far as I know, > 0x0200 is used by AIX and MacOS X. Do you suggest to name these platforms > explicitly and set it to 0 otherwise? Leaving the default at > 0x0200 has the advantage that it would implicitly work on other platforms > like HPUX at the risk that it may be the wrong value on other, yet still > unknown platforms. So in general I'm not against setting it to 0 on unknown > platfroms (as this is the most secure > setting) but in that case we should definitely name AIX and MacOS X as > "known" platforms. > > > I'm also a bit concerned about the following code in net_util_md.c > > 451 rv = setsockopt(s, SOL_SOCKET, SO_REUSEPORT, (void *)&one, > sizeof(one)); > 452 if (rv != 0 && errno == ENOPROTOOPT) { > 453 rv = JNI_FALSE; > 454 } else { > 455 rv = JNI_TRUE; > 456 } > > Why do we need the extra check for 'ENOPROTOOPT'? If the setsockopt() call, > for whatever reason, returns with an error code other than 'ENOPROTOOPT' we > will still return JNI_TRUE which I think would be bad. I think it would be > more robust to always return JNI_FALSE if > setsockopt() returns an error. > > Some tests I'm just running are still pending, but I think the rest looks > pretty good. > > Regards, > Volker > >> The javadoc looks good, a minor nit is that SocketOptions.SO_REUSEPORT needs >> @since 9. >> >> A few other minor nits in passing: >> >> - src/java.base/share/classes/java/net/AbstractPlainDatagramSocketImpl.java >> indentation doesn't look right. >> >> - sun.nio.ch.Net.checkedReusePort - shouldn't be a need to initialize the >> volatile boolean to its default value (there is a patch in progress to >> remove many of these). >> >> - SdpSupport.convert0 - I assume this should only use getsockopt when the >> socket option is supported. I think this means it will need #ifdef >> SO_REUSEPORT. >> >> - A place uses have "//SO.." when "// SO..." would be more consistent. >> >> I think that's it. >> >> -Alan >> >> >>
