Hi Alex,
Thank you for the update!
One more round of minor formatting change requests for better
readability. :)
#1:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html
+ port = Integer.decode(hostPort.substring(splitIndex+1));
. . .
+ } else if (hostPort.charAt(0) == ‘[’ &&
hostPort.charAt(splitIndex-1) == ‘]’) {
Need spaces around ‘+’ and ‘-’ signs.
#2:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c.udiff.html
+ //make the socket a dual mode socket
missed space at the start of comment
Now, comments for:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html
#3:
276 /* check for host:port or port */
277 colon = strrchr(address, ‘:’);
278 port = (colon == NULL ? address : colon + 1);
<add empty line here>
279 /* ensure the port is valid (getaddrinfo allows port to be empty) */
280 if (getPortNumber(port) < 0) {
#4:
298 hints.ai_family = allowOnlyIPv4 ? AF_INET : AF_INET6;
299 hints.ai_flags |= AI_PASSIVE | (allowOnlyIPv4 ? 0 :
AI_V4MAPPED | AI_ALL);
300 <Unneeded empty line>
301 } else {
#5: Replace “fills” with “fills in” in:
341 * Parses address (IPv4 or IPv6), fills result by parsed address.
383 * Parses prefix length from buffer (integer value), fills result
with corresponding net mask.
485 * Parses ‘allow’ argument (fills list of allowed peers (global
_peers variable)).
#6:
410 // generate mask for prefix length
411 memset(result, 0, sizeof(*result));
<Need empty line here>
412 // prefixLen <= 128, so we won’t go over result’s size
413 for (int i = 0; prefixLen > 0; i++, prefixLen -= :sunglasses: {
#7:
623 socketTransport_startListening(jdwpTransportEnv* env, const char*
address,
624 char** actualAddress)
625 {
. . . .
1173 static int readBooleanSysProp(int *result, int trueValue, int
falseValue,
1174 JNIEnv* jniEnv, jclass sysClass, jmethodID getPropMethod, const
char *propName)
1175 {
Move ‘{’ to the end of 624 and 1174. (edited)
#8:
1176 jstring value;
1177 jstring name = (*jniEnv)->NewStringUTF(jniEnv, propName);
<!!Add empty line here!!>
1178 if (name == NULL) {
. . . .
1259 } while (0);
<!!Add empty line here!!>
1260 if (jniEnv != NULL && (*jniEnv)->ExceptionCheck(jniEnv)) {
Thanks!
Serguei
On 4/12/19 4:58 PM, Alex Menkov wrote:
updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/
changes (vs webrev.02) are non-functional (added/edited comments, code
reformatting, renaming convertIpv4ToIpv6 function to
convertIPv4ToIPv6, renaming env variable to jniEnv (env is already
used in one of the functions)).
About pass/preferredAddressFamily conditions - there is no "logical
xor" in C/C++. Also I think that the current condition is clearer.
--alex
On 04/11/2019 17:18, serguei.spit...@oracle.com wrote:
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.
