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
