On 08/04/2014 18:49, Michael McMahon wrote:
Could I get the following reviewed please?
In addition to providing generic support for java.net.SocketOption,
it also includes 8032808, which implements a platform specific
socket option SO_FLOW_SLA (in jdk.net)
There are changes to two repos:
http://cr.openjdk.java.net/~michaelm/8036979/jdk/01/
I've looked through this webrev.
First I think it is great to see setOption/getOption added to the
java.net socket types, this aligns it with we did in java.nio.channels
in JDK 7 and makes it extensible as you've done with SO_FLOW_SLA.
A minor comment when reading through the java sources is that the
formatting is a bit odd in places. The try-with-resources in OptionTest
is one case. Another common one is the starting brace on its own line
for cases where it would look much better on the previous line.
The javadoc looks good. A minor typo with "{{" in the package-info. Also
in the javadoc for some of the enum member in SocketFlow.Status start in
lowercase when I assume they should be capital.
You should double check that @since is added to the new classes. Looks
to be missing from NetworkPermission and SocketFlow.Status at least.
I note that supportedOptions uses the class object for synchronization,
it's probably okay initially but it creates the potential for VM-wide
contention and may need to be looked at in the future.
Is the 2-arg NetworkPermission required? Maybe this is there just for
consistency?
I assume that java.net.Sockets does not need to load libnet, it's
already loaded by the class that defines the native methods.
A minor comment on the name setSecurityCheck, I wonder if
checkSetOptionPermission might be a better name for this method.
Sockets/Test.java - L51, did you mean to check the result?
That's all I have.
-Alan
