RFR: JDK-8184770: JDWP support for IPv6

Fri Apr 12 00:18:19 UTC 2019

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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190411/160e5ef3/attachment-0001.html>