Hi Alex,
Great debugging feature!
While I'm still reading all the details, could you, please, fix some
minor format issues?
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html
+ * If <code>host</code> is a literal IPv6 address, it may be in square
brackets. Extra space before "square".
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html
I'd suggest to unify comments before functions:
- start comment with a capital latter and ended with a dot
- use comment format like this:
/*
*/
Examples of comments that need this change:
262 /* result must be release with dbgsysFreeAddrInfo */ => /* * Result
must be release with dbgsysFreeAddrInfo. */
325 // input is sockaddr just because all clients have it =>
/* * Input is sockaddr just because all clients have it. */
1129 /* reads boolean system value,
1130 * sets *result to trueValue if the ptoperty is "true",
1131 * to falseValue if the property if "false",
1132 * doesn't change *result if the property is not set or failed to read.
1133 */ => /* * Read boolean system value andset result to: * -
trueValue if the property is "true"
* - falseValue if the property is "false" * * Return JNI_OK if result is set,
return JNI_ERR otherwise.
*/
. . .
293 * use IPv6 socket (to accept IPv6 and mapped Ipv4),...
342 * (with AF_INET6 Ipv4 addresses are not parsed even with AI_V4MAPPED
and AI_ALL flags) ...345 hints.ai_family = AF_UNSPEC; // IPv6 or mapped
Ipv4 ... 360 } else { // Ipv4 address Replace Ipv4 with IPv4 for
unification with IPv6
For unification replace: convertIpv4ToIpv6 => convertIPv4ToIPv6
297 hints.ai_flags |= AI_PASSIVE
298 | (allowOnlyIPv4 ? 0 : AI_V4MAPPED | AI_ALL); Better to have just
one line
1135 JNIEnv* env, . . . 1165 JNIEnv* jniEnv = NULL;
A suggestion is to use the same name for JNIEnv*:
1135 JNIEnv* jni, . . .
1165 JNIEnv* jni = NULL;
Reformat:
608 if ((pass == 0 && ai->ai_family == preferredAddressFamily) 609 ||
(pass == 1 && ai->ai_family != preferredAddressFamily)) and
828 if ((pass == 0 && ai->ai_family == preferredAddressFamily) 829 ||
(pass == 1 && ai->ai_family != preferredAddressFamily)) => if ((pass ==
0 && ai->ai_family == preferredAddressFamily) || (pass == 1 &&
ai->ai_family != preferredAddressFamily))
Even better, replace it with logical XOR:
if ((pass == 0 ^^ ai->ai_family == preferredAddressFamily)
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/ListeningConnector/startListening/startlis001.java.frames.html
102 /* Check that listening address returned by
ListeningConnector.startListening()
103 * matches the address which was set via connector's arguments.
104 * Empty host address causes listening for local connections only
(loopback interface)
105 * */ Dot is missed at the end. Replace "* */" with "*/".
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/test/jdk/com/sun/jdi/JdwpAllowTest.java.frames.html
162 // generate allow address by changing random bit in the local address
163 // and calculate 2 masks (prefix length) - one is matches original
local address
164 // and another doesn't. Replace with /* style of comment.
249 positiveTest("PositiveMaskTest(" + test.localAddress + ")",
test.allowAddress + "/" + test.prefixLengthGood);
250 positiveTest("NegativeMaskTest(" + test.localAddress + ")",
test.allowAddress + "/" + test.prefixLengthBad);
A suggestion to move second argument to additional line:
positiveTest("PositiveMaskTest(" + test.localAddress + ")",
test.allowAddress + "/" + test.prefixLengthGood);
positiveTest("NegativeMaskTest(" + test.localAddress + ")",
test.allowAddress + "/" + test.prefixLengthBad);
Thanks,
Serguei
On 4/2/19 4:14 PM, Alex Menkov wrote:
Updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/
- added support for addresses enclosed in square brackets;
- updated SocketTransportService.java to handle empty hostname the
same way as JDWP agent (listen/attach to loopback address);
Has to update nsk/jdi/ListeningConnector/startListening/startlis001.java
(all other com/sun/jdi, nsk/jdi, nsk/jdwp, nsk/jdb test are passed).
--alex
On 04/01/2019 11:21, Alex Menkov wrote:
Hi Chris,
On 04/01/2019 04:50, Chris Hegarty wrote:
Alex,
On 29/03/2019 22:07, Alex Menkov wrote:
(added net-dev as suggested by Alan)
Net gurus, please assist in reviewing socket-related code.
New webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.01/
Specifically on SocketTransportService.java. What Arthur has
proposed is better ( changing to lastIndexOf alone is not
sufficient ). Or is your assumption that the IPv6 literal is
not enclosed in square brackets?
I didn't know about enclosing IPv6 in square brackets, but looks like
that's standard way to alleviate conflict between IPv6 address and
colon as port separator.
Will update the fix to handle them in both JDI connectors
(SocketTransportService.java) and debugger agent (socketTransport.c)
If keeping Arthur's `static class HostPort` please make the
fields final.
>> Would it make sense to support the preference properties?
>> java.net.preferIPv4Stack
>> java.net.preferIPv6Addresses
I'm not sure about this, especially given the property name
prefixes. I need to think a little more on it.
In the initial version of the fix I didn't check the properties.
The rationale here is backward compatibility - is address is empty,
old debuggers tries to connect to IPv4 address only.
But handling this properties we will better handle clients with
properties set (as jdb or any other debugger which uses JDI
connectors are affected by the properties).
BTW fix provided by Arthur implements listening on localhost
differently - it creates several sockets and binds them to both IPv4
and IPv6 addresses. But the problem here (and that's the reason I
decide to not implement it this way) - how to handle the case when we
successfully bind on one address (for example IPv4), but fail to bind
on other (for example the port is busy for IPv6 stack). Arthur's
version just fail in the case (i.e. the whole Java process
terminates) and I don't think this is good behavior.
There is quite a bit of new native code, is it possible to
rewrite any of this in Java, e.g. reading of the system
properties ( if that is to remain )?
socketTransport.c is a debugger agent which is completely native.
--alex
-Chris.
