Hi Alan, Happy New Year!!
Thanks very much for your time reviewing the patch. I will make the changes regarding your comments and send them back here soon. Thanks, Lucy -----Original Message----- From: Alan Bateman [mailto:alan.bate...@oracle.com] Sent: Friday, January 01, 2016 3:18 AM To: Lu, Yingqi <yingqi...@intel.com> Cc: Viswanathan, Sandhya <sandhya.viswanat...@intel.com>; net-dev@openjdk.java.net; Kharbas, Kishor <kishor.khar...@intel.com>; Aundhe, Shirish <shirish.aun...@intel.com>; Kaczmarek, Eric <eric.kaczma...@intel.com> Subject: Re: Patch for adding SO_REUSEPORT socket option 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. 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
