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

[Chris Hegarty]

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.