8196751: Add jhsdb option to specify debug server RMI connector port

[Yasumasa Suenaga]

Hi Daniil,


- SALauncher.java
     - checkBasicOptions() is needed? I think you can remove this method and embed it in caller.
     - I think registryPort should be checked with Integer.parseInt() like others (rmiPort and pid) rather than regex.
     - Shutdown hook is very good idea. You can implement more simply if you use lambda expression.

- SADebugDTest.java
     - Please add bug ID to @bug.
     - Why do you declare portInUse and testResult as array? Their length is 1, so I think you don't need to use array.


Thanks,

Yasumasa


On 2020/03/06 10:15, Daniil Titov wrote:
> Hi Yasumasa, Serguei and Alex,
> 
> Please review a new version of the fix [1] that addresses your comments. The new version in addition to RMI connector
> port option introduces two more options to specify RMI registry port and RMI connector host name. Currently, these
> last two settings could be specified using the system properties but the system properties have the following disadvantages
> comparing to the command line options:
>     -  It’s hard to know about them: they are not listed in tool’s help.
>     -  They have long names that hard to remember
>     -   It is easy to mistype them  in the command line and you will not get any warning about it.
> 
> The CSR [2] was also updated and needs to be reviewed.
> 
> Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker
> container  and connecting  to it with the GUI debugger.  Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded.
> 
> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.02/
> [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
> [3] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751
> 
> Thank you,
> Daniil
> 
> On 2/24/20, 5:45 AM, "Yasumasa Suenaga" <suenaga at oss.nttdata.com> wrote:
> 
>      Hi Daniil,
>      
>         - SALauncher::buildAttachArgs is not only to build arguments but also to check consistency of arguments.
>           Thus you should use buildAttachArgs() in runDEBUGD(). If you do so, runDEBUGD() would be more simply.
>      
>         - SADebugDTest::testWithPidAndRmiPort would retry until `--rmiport` can be used.
>           But you can use same port number as RMI registry (1099).
>           It is same as relation between jmxremote.port and jmxremote.rmi.port.
>      
>      
>      Thanks,
>      
>      Yasumasa
>      
>      
>      On 2020/02/24 13:21, Daniil Titov wrote:
>      > Please review change that adds a new command line option to jhsdb tool for the debugd mode to specify a RMI connector port.
>      > Currently a random port is used that prevents the debug server from being used behind a firewall or in a container.
>      >
>      > New CSR [3] was created for this change and it needs to be reviewed as well.
>      >
>      > Man pages for jhsdb will be updated in a separate issue.
>      >
>      > The current implementation (sun.jvm.hotspot.SALauncher)  parses the command line options passed to jhsdb tool,
>      > converts them to the ones for the debug server and then delegates the call  to sun.jvm.hotspot.DebugServer.main().
>      >
>      >                // delegate to the actual SA debug server.
>      >   367         DebugServer.main(newArgArray.toArray(new String[0]));
>      >
>      > However,  sun.jvm.hotspot.DebugServer  doesn't support named options and that prevents from efficiently adding new options to the tool.
>      > I found it more suitable to start Hotspot agent directly in  SALauncher rather than  adding a new option in  both sun.jvm.hotspot.SALauncher
>      >   and sun.jvm.hotspot.DebugServer and  delegating the call.  With this change I think sun.jvm.hotspot.DebugServer could be marked as a deprecated
>      > but I would prefer to address it in a separate issue.
>      >
>      > Testing: Manual testing with attaching the debug server to the running Java process or to the core file inside a docker
>      >                  container  and connecting  to it with the GUI debugger.
>      >                 Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd tests) succeeded.
>      >
>      > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.01
>      > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8196751
>      > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
>      >
>      > Thank you,
>      > Daniil
>      >
>      >
>      
> 
>