RFR: JDK-8184770: JDWP support for IPv6
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sat Apr 13 01:52:21 UTC 2019
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.
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190412/2ebbe513/attachment-0001.html>
More information about the serviceability-dev
mailing list