RFR: 8263636: Add --disableregistry option to jhsdb debugd

[Chris Plummer]

On Sun, 28 Mar 2021 07:53:31 GMT, Yasumasa Suenaga <ysuenaga at 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 at 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 at 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