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 <[email protected]> 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
