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