Re: PING: Re: RFR: JDK-8184770: JDWP support for IPv6
Even though I showed our internal patches in this area earlier, I haven't
really looked into exactly what this patch does (since it was written by
someone else).
I can confirm that it fixes jshell in an IPv6 only environment though.
*From: *
*Date: *Tue, May 7, 2019 at 11:42 AM
*To: *Alex Menkov, , net-dev, Chris
Hegarty
Hi guys,
>
> We need a couple of partial reviews for this enhancement:
>
> - From the net-dev to check IPv6-addresses related part.
>It does not need to be a thorough review.
>We need another pair of eyes to check for obvious typos or misses.
>Included Chris H. to mailing list in hope for some assistance.
>(Chris, we need some help to find one of the IPv6 experts.)
>
> - From the serviceability-dev to check if compatibility
>is not broken and nothing is missed.
>This includes part of code that is not directly involved
>into manipulations with the IPv4/IPv6 addresses.
>The related spec update is tracked by a sub-task:
> https://bugs.openjdk.java.net/browse/JDK-8221707
>
>
> Related CSR:
> https://bugs.openjdk.java.net/browse/JDK-8223104
>
> The latest webrev:
> http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/
>
>
> Thanks,
> Serguei
>
>
> On 5/6/19 3:27 PM, serguei.spit...@oracle.com wrote:
>
> Hi Alex,
>
> It looks great and very solid in general!
>
> Some minor comments are below.
>
>
> http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html
>
> 263 * Result must be release with dbgsysFreeAddrInfo.
>
> A typo: "must be release" => "must be released"
>
> 80 typedef struct { 81 struct in6_addr subnet; 82 struct in6_addr
> netmask;
> 83 } AllowedPeerInfo;
> . . .
> 431 parseAllowedPeersInternal(char *buffer) {
> . . . 483 }
> . . .
> 524 isPeerAllowed(struct sockaddr_storage *peer) { 525 struct in6_addr
> tmp; 526 struct in6_addr *addr6; 527 if (peer->ss_family == AF_INET)
> { 528 convertIPv4ToIPv6((struct sockaddr *)peer, ); 529
> addr6 = 530 } else { 531 addr6 = &(((struct sockaddr_in6
> *)peer)->sin6_addr); 532 } 533 534 for (int i = 0; i < _peers_cnt;
> ++i) { 535 if (isAddressInSubnet(addr6, &(_peers[i].subnet),
> &(_peers[i].netmask))) {
> 536 return 1;
> 537 }
> 538 }
>
> The allowed pears are converted into and used/checked in the IPv6 format.
> Some short comments about it in all three places above would be helpful.
> I'd consider to do the same in parseAllowedAddr() before this fragment:
>
> 367 if (addrInfo->ai_family == AF_INET6) { 368 memcpy(result,
> &(((struct sockaddr_in6 *)(addrInfo->ai_addr))->sin6_addr), sizeof(*result));
> 369 *isIPv4 = 0; 370 } else {// IPv4 address 371
> struct in6_addr addr6; 372 convertIPv4ToIPv6(addrInfo->ai_addr,
> ); 373 memcpy(result, , sizeof(*result)); 374
> *isIPv4 = 1;
> 375 }
>
>
> and in parseAllowedMask() before the line:
>
> 420 result->s6_addr[i] = (char)(0xFF << (8 - prefixLen));
>
>
> This fragment needs a comment:
> 402 if (isIPv4) { 403 prefixLen += 96;
> 404 }
>
>
>
> We have to find out at least one more reviewer for this fix!
>
> Thanks,
> Serguei
>
>
> On 5/3/19 18:14, serguei.spit...@oracle.com wrote:
>
> Hi Alex,
>
> Thank you for creating the CSR!
> I've added myself as a reviewer and corrected a couple of places.
> Feel free to change my changes if necessary. :)
> It would be nice to get one more CSR review if possible, so I've added the
> net-dev mailing list.
>
> I hope to finish the webrev review soon.
>
> Thanks,
> Serguei
>
> On 5/1/19 10:54 AM, Alex Menkov wrote:
>
> Hi all,
>
> I created CSR for the feature:
> https://bugs.openjdk.java.net/browse/JDK-8223104
>
> The latest webrev:
> http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/
>
> --alex
>
>
>
>
>
PING: Re: RFR: JDK-8184770: JDWP support for IPv6
Hi guys,
We need a couple of partial reviews for this enhancement:
- From the net-dev to check IPv6-addresses related part.
It does not need to be a thorough review.
We need another pair of eyes to check for obvious typos or misses.
Included Chris H. to mailing list in hope for some assistance.
(Chris, we need some help to find one of the IPv6 experts.)
- From the serviceability-dev to check if compatibility
is not broken and nothing is missed.
This includes part of code that is not directly involved
into manipulations with the IPv4/IPv6 addresses.
The related spec update is tracked by a sub-task:
https://bugs.openjdk.java.net/browse/JDK-8221707
Related CSR:
https://bugs.openjdk.java.net/browse/JDK-8223104
The latest webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/
Thanks,
Serguei
On 5/6/19 3:27 PM, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks great and very solid in general!
Some minor comments are below.
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html
263 * Result must be release with dbgsysFreeAddrInfo.
A typo: "must be release" => "must be released"
80 typedef struct {
81 struct in6_addr subnet;
82 struct in6_addr netmask;
83 } AllowedPeerInfo;
. . .
431 parseAllowedPeersInternal(char *buffer) { . . .
483 } . . . 524 isPeerAllowed(struct sockaddr_storage *peer) {
525 struct in6_addr tmp;
526 struct in6_addr *addr6;
527 if (peer->ss_family == AF_INET) {
528 convertIPv4ToIPv6((struct sockaddr *)peer, );
529 addr6 =
530 } else {
531 addr6 = &(((struct sockaddr_in6 *)peer)->sin6_addr);
532 }
533
534 for (int i = 0; i < _peers_cnt; ++i) {
535 if (isAddressInSubnet(addr6, &(_peers[i].subnet),
&(_peers[i].netmask))) {
536 return 1;
537 }
538 }
The allowed pears are converted into and used/checked in the IPv6 format.
Some short comments about it in all three places above would be helpful.
I'd consider to do the same in parseAllowedAddr() before this fragment:
367 if (addrInfo->ai_family == AF_INET6) {
368 memcpy(result, &(((struct sockaddr_in6
*)(addrInfo->ai_addr))->sin6_addr), sizeof(*result));
369 *isIPv4 = 0;
370 } else { // IPv4 address
371 struct in6_addr addr6;
372 convertIPv4ToIPv6(addrInfo->ai_addr, );
373 memcpy(result, , sizeof(*result));
374 *isIPv4 = 1;
375 }
and in parseAllowedMask() before the line:
420 result->s6_addr[i] = (char)(0xFF << (8 - prefixLen));
This fragment needs a comment: 402 if (isIPv4) {
403 prefixLen += 96;
404 }
We have to find out at least one more reviewer for this fix!
Thanks,
Serguei
On 5/3/19 18:14, serguei.spit...@oracle.com wrote:
Hi Alex,
Thank you for creating the CSR!
I've added myself as a reviewer and corrected a couple of places.
Feel free to change my changes if necessary. :)
It would be nice to get one more CSR review if possible, so I've
added the net-dev mailing list.
I hope to finish the webrev review soon.
Thanks,
Serguei
On 5/1/19 10:54 AM, Alex Menkov wrote:
Hi all,
I created CSR for the feature:
https://bugs.openjdk.java.net/browse/JDK-8223104
The latest webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/
--alex
Fwd: Re: RFR: JDK-8223416: Exclude javax/management/monitor/DerivedGaugeMonitorTest.java until JDK-8042211 is fixed.
Original Message Subject: Re: RFR: JDK-8223416: Exclude javax/management/monitor/DerivedGaugeMonitorTest.java until JDK-8042211 is fixed. Date: Tue, 7 May 2019 13:59:57 -0400 From: Daniel D. Daugherty Reply-To: daniel.daughe...@oracle.com To: gary.ad...@oracle.com, Internal Serviceability Gary, Thumbs up! But... The test is open so problem listing the test should also be open. The bug is closed because the description contains an internal URL. Dan On 5/7/19 1:56 PM, Gary Adams wrote: Trivial update to ProblemList test while underlying fix is developed. diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt --- a/test/jdk/ProblemList.txt +++ b/test/jdk/ProblemList.txt @@ -566,6 +566,8 @@ java/lang/management/ThreadMXBean/ThreadMXBeanStateTest.java 8081652 generic-all java/lang/management/ThreadMXBean/AllThreadIds.java 8131745 generic-all +javax/management/monitor/DerivedGaugeMonitorTest.java 8042211 generic-all + # jdk_io
Re: RFR 8223065: Add jcmd to get the listen address of the debugger
On 07/05/2019 14:13, Schmelter, Ralf wrote: Hi Christoph, thanks for the review. I've updated the webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.4/ Specifying onjcmd=y to the debugger agent looks very strange. Was there a CSR submitted for JDK-8214892? I suspect that sub-option needs further discussion before additional features on built on it. -Alan
RE: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi Christoph, thanks for the review. I've updated the webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.4/ Best regards, Ralf
RE: RFR 8223065: Add jcmd to get the listen address of the debugger
Hi Ralf, the change looks good to me, overall. I found a few minor nits in the tests. test/jdk/com/sun/jdi/OnJcmdTest.java 3 * Copyright (c) 2018, 2019, SAP SE. All rights reserved. -> according to our guidelines, we should not have a ',' after the last year for the SAP copyrights 33 * @run compile --add-exports java.base/jdk.internal.vm=ALL-UNNAMED -g OnJcmdTest.java -> can you replace this with @modules java.base/jdk.internal.vm ? The -g option is also not required, I guess (unless somebody debugs the test) Then you should be able to change 34 * @run main/othervm --add-exports java.base/jdk.internal.vm=ALL-UNNAMED -agentlib:jdwp=transport=dt_socket,address=localhost:0,onjcmd=y,server=y OnJcmdTest into @run main/othervm -agentlib:jdwp=transport=dt_socket,address=localhost:0,onjcmd=y,server=y OnJcmdTest 37 import java.lang.reflect.Method; -> can be removed test/jdk/com/sun/jdi/GetListenAddressTest.java -> SAP copyright header needs fixing like above -> 33 * @run compile -g GetListenAddressTest.java should be removed -> 37 import java.lang.ProcessBuilder.Redirect; -> is unnecessary, should be removed I'll sponsor the change for you. Best regards Christoph > -Original Message- > From: serviceability-dev On > Behalf Of Baesken, Matthias > Sent: Montag, 6. Mai 2019 12:55 > To: serviceability-dev@openjdk.java.net > Subject: [CAUTION] Re: RFR 8223065: Add jcmd to get the listen address of > the debugger > > Hello, looked at the latest web rev ( > http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.3/ ) , > looks good to me ! > (not a Reviewer however ) > > > Best regards, Matthias > > > > > > On 30/04/2019 9:33 pm, Schmelter, Ralf wrote: > > > Hi David, > > > > > > good catch. I've moved the vm->native transition back to the start of the > > method and instead do a native->vm transition before calling > > print_debug_listen_address() method. > > > > > > webrev: > > http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.2/ > > > > Yep that works too. :) > > > > Not a review as I didn't look at the rest of the code. > > > > Cheers, > > David > > > > > Best regards, > > > Ralf > > > > > > >
Re: RFR(m): 8223306: Remove threads linked list (use ThreadsList's array in SA)
Hi David,
On 5/7/19 9:47 AM, David Holmes wrote:
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java -
original code:
1556 new Command("where", "where { -a | id }", false) {
1557 public void doit(Tokens t) {
...
1564 for (JavaThread thread = threads.first(); thread !=
null; thread = thread.next()) {
1565 ByteArrayOutputStream bos = new
ByteArrayOutputStream();
1566 thread.printThreadIDOn(new PrintStream(bos));
1567 if (all || bos.toString().equals(name)) {
1568 out.println("Thread " + bos.toString() + "
Address: " + thread.getAddress());
...
1577 }
1578 if (!all) return;
That looks like an early return to me.
Yes, thanks, it should not have been converted then.
I'll revisit CommandProcessor.java and the other sites.
/Robbin
Cheers,
David
-
Thanks, Robbin
Thanks,
David
On 6/05/2019 5:31 pm, Robbin Ehn wrote:
Hi, please review.
Old threads linked list remove and updated SA to use ThreadsList array instead.
Issue:
https://bugs.openjdk.java.net/browse/JDK-8223306
Webrev:
http://cr.openjdk.java.net/~rehn/8223306/webrev/
Passes t1-3 (which includes SA tests).
Thanks, Robbin
Re: RFR(m): 8223306: Remove threads linked list (use ThreadsList's array in SA)
Hi Robbin,
On 7/05/2019 4:50 pm, Robbin Ehn wrote:
Hi David,
On 5/7/19 12:40 AM, David Holmes wrote:
Hi Robbin,
I have a few concerns here.
First I can't see how you are actually integrating the SA with the
ThreadSMR. You've exposed the _java_thread_list for it to iterate but
IIRC that list can be updated when threads are added/removed and I'm
not seeing how the SA is left iterating a valid list - we'd normally
using a ThreadsListHandle for that ?? (I may need a refresher on how
this list is actually maintained.)
The processes must be paused. If the processes would be running the
linked list is also broken since if we unlink and delete a JavaThread
and then later SA follows that _next pointer.
Ah good point. Thanks for clarifying.
The conversion from external iteration of the list (the for loop) to
internal iteration (passing a lambda to JavaThreadsDo) is also
problematic. First I'd be very wary about introducing lambda
expressions into the SA code - lambda drags in a lot of supporting
code that could have an impact on the way SA functions. There are
places where we have to avoid lambdas due to
bootstrapping/initialization issues and I think the SA may be an area
where we also want to keep the code extremely simple.
There are already several usages of lambdas in SA code, e.g.
LinuxDebuggerLocal.java. SA is not a core module, it's an application,
there is not a bootstrap issue or so.
Hmm okay. Lambda carries a lot of baggage. But if its already being used ...
Second by using lambda's with internal iteration you've lost the
ability to terminate the iteration loop! In the existing code if we
have a return in the for-loop then we not only terminate the loop but
the enclosing method. With the lambda the "return" just ends the
current iteration and JavaThreadsDo will then continue with the next
thread - so we don't even terminate the iteration let alone the method
performing the iteration. So for places were we want to process one
thread, for example, we will continue to iterate all remaining threads
and just do nothing with them. That's very inefficient.
That's why I only used the internal iteration where we didn't have early
returns.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java
- original code:
1556 new Command("where", "where { -a | id }", false) {
1557 public void doit(Tokens t) {
...
1564 for (JavaThread thread = threads.first();
thread != null; thread = thread.next()) {
1565 ByteArrayOutputStream bos = new
ByteArrayOutputStream();
1566 thread.printThreadIDOn(new PrintStream(bos));
1567 if (all || bos.toString().equals(name)) {
1568 out.println("Thread " + bos.toString()
+ " Address: " + thread.getAddress());
...
1577 }
1578 if (!all) return;
That looks like an early return to me.
Cheers,
David
-
Thanks, Robbin
Thanks,
David
On 6/05/2019 5:31 pm, Robbin Ehn wrote:
Hi, please review.
Old threads linked list remove and updated SA to use ThreadsList
array instead.
Issue:
https://bugs.openjdk.java.net/browse/JDK-8223306
Webrev:
http://cr.openjdk.java.net/~rehn/8223306/webrev/
Passes t1-3 (which includes SA tests).
Thanks, Robbin
Re: RFR(m): 8223306: Remove threads linked list (use ThreadsList's array in SA)
Hi David, On 5/7/19 12:40 AM, David Holmes wrote: Hi Robbin, I have a few concerns here. First I can't see how you are actually integrating the SA with the ThreadSMR. You've exposed the _java_thread_list for it to iterate but IIRC that list can be updated when threads are added/removed and I'm not seeing how the SA is left iterating a valid list - we'd normally using a ThreadsListHandle for that ?? (I may need a refresher on how this list is actually maintained.) The processes must be paused. If the processes would be running the linked list is also broken since if we unlink and delete a JavaThread and then later SA follows that _next pointer. The conversion from external iteration of the list (the for loop) to internal iteration (passing a lambda to JavaThreadsDo) is also problematic. First I'd be very wary about introducing lambda expressions into the SA code - lambda drags in a lot of supporting code that could have an impact on the way SA functions. There are places where we have to avoid lambdas due to bootstrapping/initialization issues and I think the SA may be an area where we also want to keep the code extremely simple. There are already several usages of lambdas in SA code, e.g. LinuxDebuggerLocal.java. SA is not a core module, it's an application, there is not a bootstrap issue or so. Second by using lambda's with internal iteration you've lost the ability to terminate the iteration loop! In the existing code if we have a return in the for-loop then we not only terminate the loop but the enclosing method. With the lambda the "return" just ends the current iteration and JavaThreadsDo will then continue with the next thread - so we don't even terminate the iteration let alone the method performing the iteration. So for places were we want to process one thread, for example, we will continue to iterate all remaining threads and just do nothing with them. That's very inefficient. That's why I only used the internal iteration where we didn't have early returns. Thanks, Robbin Thanks, David On 6/05/2019 5:31 pm, Robbin Ehn wrote: Hi, please review. Old threads linked list remove and updated SA to use ThreadsList array instead. Issue: https://bugs.openjdk.java.net/browse/JDK-8223306 Webrev: http://cr.openjdk.java.net/~rehn/8223306/webrev/ Passes t1-3 (which includes SA tests). Thanks, Robbin
