On Sun, 28 Mar 2021 07:53:31 GMT, Yasumasa Suenaga <ysuen...@openjdk.org> wrote:
> `jhsdb debugd` will start RMI registry by default, but we want to prevent it > due to port confliction in some cases. We can control it with > `sun.jvm.hotspot.rmi.startRegistry` system property. However we have no way > to set it excepting system property. jhsdb should provide the way to set it > as a command line option. > > This PR introduces `--disableregistry` to `jhsdb debugd`. Please review > [CSR](https://bugs.openjdk.java.net/browse/JDK-8264021) too. Overall the changes look good. I just have some minor suggestions for comments, code formatting, and help output. src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java line 89: > 87: if (canConnectToRemote) { > 88: System.out.println(" or jhsdb " + mode + " > --connect debugserver"); > 89: System.out.println(" or jhsdb " + mode + " > --connect id@debugserver:1234"); You change here makes it look like if you specify `id@` then you also need to specify the port. I'd suggest also including the original line that just has `id@debugserver`. src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/SALauncher.java line 103: > 101: " This option overrides the system property > 'sun.jvm.hotspot.rmi.port'. If not specified," + > 102: " the system property is used. If the system property is > not set, the default port 1099 is used."); > 103: System.out.println(" --disableregistry Do not start RMI > registry (to use RMI registry that exists)"); "to use RMI registry that exists" doesn't read well. Do you mean "use already started RMI registry"? src/jdk.hotspot.agent/share/man/jhsdb.1 line 188: > 186: If not specified, RMI registry will be started on startup. > 187: Otherwise will it not start, the RMI registry (rmid, etc) is needed > 188: before starting debugd. Is this what you mean: `Otherwise it will not be started, and the already started RMI registry will be used instead.` test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdRmidTest.java line 37: > 35: * @test > 36: * @bug 8263636 > 37: * @requires vm.hasSA Can you add an `@summary` comment? test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdRmidTest.java line 58: > 56: > .redirectError(ProcessBuilder.Redirect.INHERIT) > 57: .start(); > 58: Thread.sleep(3000); // Sleep 3 sec for waiting to start rmid. "Sleep 3 sec to wait for rmid to start". test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdUtils.java line 41: > 39: > 40: private boolean disableRegistry; > 41: I'd suggest removing the existing empty lines rather than follow the pattern of adding them. ------------- Changes requested by cjplummer (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3233
