RFR: 8036979: Support java.net.SocketOption<> in java.net socket types

[Michael McMahon]

Thanks Alan. All comments accepted, except as clarified below.

Michael

On 09/04/14 15:07, Alan Bateman wrote:
> 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 notice that all permission types that extend BasicPermission have one. 
Maybe some types of Policy
object expect it, though strictly it shouldn't be necessary.

> 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?
>
in this case no. It acts as a regression check for a bug that was 
triggered by invoking the method
prior to doing any other operation with sockets.