RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]

[Ivan Gerasimov]

Thank you Christoph and Chris for the review!

I made the following suggested changes:
- fdAccess made private static final in both TwoStacks and DualStack.
- removed redundant check IS_NULL(iaObj) at bind0 (indeed, the address 
was already checked at Java level).

For now, I would prefer to keep the checks for family == IPv4 as they are.
I totally agree they should be moved to Java, but I'm planning to do 
this during the TwoStacks-DualStack merge step.

I'm also planning to add a regtest to make sure that Inet6Address is 
rejected when fed to a IPv4-only socked.

Are we good to push now?
Please let me know, if you'd like me to send the updated webrev set.

With kind regards,
Ivan


On 3/27/18 6:47 AM, Chris Hegarty wrote:
> Ivan,
>
>> On 26 Mar 2018, at 23:08, Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
>>
>> Hello everyone!
>>
>> A few modifications were done to the patch.
>> Below is the list of updated parts of the patch with comments about what has changed, comparing to the previous version of the patch.
>>
>> The patched JDK builds fine and all the tests pass well.
>>
>> WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/04/000/webrev/
> Ok.
>
>> WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/04/001/webrev/
> Ok.
>
>> WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/04/002/webrev/
> Can we make fdAccess final ( and in DualStack too )
>      static *final* JavaIOFileDescriptorAccess fdAccess
>
> JNIEXPORT void JNICALL Java_java_net_TwoStacksPlainSocketImpl_bind0 ...
>
>   142     if (IS_NULL(iaObj)) {
>   143         JNU_ThrowNullPointerException(env, "inet address argument");
>   144         return;
>   145     }
>   146
>   147     family = getInetAddress_family(env, iaObj);
>   148     if (family != java_net_InetAddress_IPv4) {
>   149         JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
>   150                         "Protocol family not supported");
>   151         return;
>   152     }
>
> The address is already null checked at the java level, so can be removed here, no?
>
> Can the family check be hoisted up into Java? I think this will help when it comes
> to the eventual merge with DualStack.
>
> The native code will then align with DualStack’s.  I prefer to reduce and align
> the native layers as much as possible. It’s easier to deal with differences at
> the Java level.
>
>> WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/04/003/webrev/
>> - Rebased to reflect recent changes to socketListen()
> Ok.
>
>> WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/04/004/webrev/
> Ok.
>
>> WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/04/005/webrev/
> Ok.
>
>> WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/04/006/webrev/
> Ok.
>
>> WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/04/007/webrev/
> Ok.
>
>> WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/04/008/webrev/
>> - The check the line 400 was changed to test if (newfd < 0) and then if (newfd == -2).
>> I don't think the later is quite accurate (cannot find in the documentation that accept can return -2), but it is how it was done in TwoStacks originally, so no regression is expected.
> Ok.
>
>   413     if (sa.sa.sa_family != AF_INET) {
>   414         JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
>   415                         "Protocol family not supported");
>   416         NET_SocketClose(newfd);
>   417         return -1;
>   418     }
>
> This check could be hoisted up to the Java-level too, no? Again, I think it
> would be easier to deal with this kinda differences in Java.
>
>> WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/04/009/webrev/
>> - Added missing 'return -1;' at the line 188.
> Can the family be checked at the java level.
>
>> WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/04/010/webrev/
> Ok.
>
>> WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/04/011/webrev/
> Ok.
>
>> WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/04/012/webrev/
> Ok.
>
>> WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/04/013/webrev/
> Ok.
>
>> WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/04/014/webrev/
> Good.
>
> -Chris.
>
>

-- 
With kind regards,
Ivan Gerasimov