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 host 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
