RFR: JDK-8184770: JDWP support for IPv6

Tue Apr 23 23:35:44 UTC 2019

Hi Serguei,

updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/

Fixed everything except #7 (for consistency - in the file if function 
arguments spread on several lines, opening curly bracket is placed to 
separate line to separate function declaration from the function body).

--alex

On 04/12/2019 18:52, serguei.spitsyn at oracle.com wrote:
> 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.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.
>>>
> 