RFR: JDK-8184770: JDWP support for IPv6

[Alex Menkov]

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.spitsyn at 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.
>