[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: JDK-8184770: JDWP support for IPv6
From: Alex Menkov <alexey.menkov () oracle ! com>
Date: 2019-04-23 23:35:44
Message-ID: d5eb0a83-061f-f619-487a-60839958d89e () oracle ! com
[Download RAW message or body]
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@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@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.
> > >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic