[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR 8223065: Add jcmd to get the listen address of the debugger
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2019-04-30 18:40:26
Message-ID: f843fbfa-6844-102a-ac77-778d23ea78cd () oracle ! com
[Download RAW message or body]

On 4/30/19 4:30 AM, Schmelter, Ralf wrote:
> Hi Chris.
> 
> > I think print_debug_listen_address() should have some exception checking
> > added after the java calls.
> That is already done by the CHECK_false macro. It checks if an exception is \
> occurred, and returns with false if this is the case. And depending on how the dcmd \
> was called, this exception is then handled (e.g. if called via jcmd, the exception \
> is printed and if called the jmm_ExecuteDiagnosticCommand method, the exception is \
> delivered to the Java code).
Ok.
> 
> > I'm a little unsure why you modified DebugOnCmdStartDCmd to use
> > print_debug_listen_address(), but still have a fallback to print the
> > specified transport and address.
> Yeah, the fallback is really not needed, since the debugger is not listening in \
> this case. I've removed it from the code. 
> > If anything I would have written a
> > get_debug_listen_address() and used it to verify that the specified and
> > actual addresses end up being the same (and then also make
> > print_debug_listen_address() use this API).
> Because the requested address and the actual address the debugger is listening can \
> be different. In the case of sockets, if you request localhost:0, the system will \
> find an unused port, so the listening address will be the actual port on which we \
> listen.
Ok.
> 
> > You need to update copyright date to 2019.
> Fixed.
> 
> > Can you write a test for this new dcmd. You can probably just extend
> > OnJcmdTest.java.
> The GetListenAddressTest tests the new dcmd.
Ok. I missed it because when after reviewing the OnJcmdTest.java changes 
in "frames" mode and then clicking on "next" to review the next file, it 
returns you to the index. Seems this is normal behavior for new files in 
webrevs since there is no "frames" mode for them.

Changes look good.

Chris
> 
> I've updated the webrev (including David's suggestion): \
> http://cr.openjdk.java.net/~rschmelter/webrevs/8223065/webrev.2/ 
> Best regards,
> Ralf


[prev in list] [next in list] [prev in thread] [next in thread]