RFR: 8004095: Add support for JMX interface to Diagnostic Framework and Commands,

frederic parain frederic.parain at oracle.com
Thu Apr 18 07:03:34 PDT 2013


Karen,

Thanks for the review, new webrevs are available here:

8004095:
http://cr.openjdk.java.net/~fparain/8004095/webrev.05/

7150256:
http://cr.openjdk.java.net/~fparain/7150256/webrev.05/

My answers to your questions are in-lined below:

On 10/01/2013 21:28, Karen Kinnear wrote:
> 1. in the EFP and diagnosticCommand.hpp, why do GC.run and GC. run_finalization not need any special Java permissions?
> Is this backward compatibility with existing ways to do this?

It is already possible to trigger a GC or run finalizers from
existing Platform MBeans without holding special permissions.
So, it would have make no sense to require a special permission
for GC.run and GC.run_finalization diagnostic commands.

> 2. jmm.h
> what happens if you run with code built with an older jmm.h in another process - are there cases where there would
> be a misinterpretation of dcmdInfo, dcmdArgInfo because the new arguments were not added at the end?

There's a risk when mixing code from non-official builds, but not with
official builds or recent hsx/jdk combinations.

> 3. diagnosticFramework.hpp
> who deallocates the JavaPermission strings?
> Don't we have the same issue with the name, description and impact already?
> That they never get freed?

Passing dynamically allocated String from the VM to the JDK is
not recommended because it's hard to know when they can be deallocated.
The trick here is to use string literals:

  static const char* impact() { return "Low"; }

Here's "Low" is a string literal statically allocated in .data section
of the binary, so you don't need to allocate/deallocate them and they
can safely be used from the JDK.

> 5. diagnosticFramework.hpp line 216 "DCmdParser parse" -> "DCmdParser parses"
> line 220 "relatively" -> "relative"

Fixed

> 6. EFP/diagnosticCommand.cpp - this is the only major discussion point
>    what is the plan for ManagementAgent.start/start_local, stop?
>    Are we planning to add a new permission?
>    Seems like a remote stop of the agent stops you in your tracks - i.e. you can now not remotely restart
>    - at one point we discussed a "restart" rather than a stop, which might allow a remote requestor to
>      change arguments without losing the ability to connect

Diagnostic commands related to the ManagementAgent are currently
not exported via the PlatformMBean, preventing you from shooting
yourself in the foot. I'm not aware of an ongoing work to implement
a ManagementAgent.restart command.

> 7. diagnosticCommand.cpp
> line 259 "are disabled" -> "is disabled"

Fixed

> 8. VMManagementImpl.c - for 7150256
> line 394 "least one the" -> "least one of the"

Fixed

> DiagnosticCommandImpl.java - for 7150256
> I tend to catch all exceptions rather than be specific - if they all would result
> in a failure, or e.g. ignoring a diagnostic command - that allows for later code changes
> that still get caught

Diagnostic commands themselves could throw exceptions, so
the code only catches exception it could have triggered itself.

> 9. sorry - copyrights will all want to be 2013 now

Fixed

Thanks,

Fred

> thanks,
> Karen
> On Dec 17, 2012, at 10:01 AM, Frederic Parain wrote:
>
>> Staffan,
>>
>> Thank you for the review, I've fixed the code to address all
>> the issue you reported. The new webrev is here:
>>
>> http://cr.openjdk.java.net/~fparain/8004095/webrev.01/
>>
>> Regards,
>>
>> Fred
>>
>> On 12/17/12 01:27 PM, Staffan Larsen wrote:
>>> Frederic,
>>>
>>> Looks good! Just a few small comments below.
>>>
>>> diagnosticCommand.cpp:255: When DisableExplicitGC is set, it would be great if the SystemGC command printed on error message so that the caller knows that the GC didn't happen as intended. I also think we should add an entry to GCCause for GCs caused by the DCmd (but that isn't new to this change).
>>>
>>> diagnosticFramework.hpp:423: nit: misspelled "enabled" in the method name
>>>
>>> diagnosticFramework.cpp:435: seems some testing code has slipped in
>>>
>>> diagnosticFramework.cpp:509: nit: missing spaces around '&'. (Consider adding () to clarify the precedence.)
>>>
>>> Thanks,
>>> /Staffan
>>>
>>> On 17 dec 2012, at 12:26, Frederic Parain <frederic.parain at oracle.com> wrote:
>>>
>>>> Hi,
>>>>
>>>> Please review the following change for CR 8004095.
>>>>
>>>> This changeset is the JDK side of two parts project.
>>>>
>>>> The goal of this feature is to allow remote invocations of
>>>> Diagnostic Commands via JMX.
>>>>
>>>>
>>>> Diagnostic Commands are actually invoked from the jcmd
>>>> tool, which is a local tool using the attachAPI. It was
>>>> not possible to remotely invoke these commands.
>>>> This project creates a new PlatformMBean, remotely
>>>> accessible via the Platform MBean Server, providing
>>>> access to Diagnostic Commands too.
>>>>
>>>> The JDK side of this project, including the new
>>>> Platform MBean, is covered in CR 7150256.
>>>>
>>>> This changeset contains the modifications in the
>>>> JVM to support the new API.
>>>>
>>>> There are two types of modifications:
>>>>   1 - extension of the diagnostic command framework
>>>>   2 - extension of existing diagnostic commands
>>>>
>>>> Modifications of the framework:
>>>>
>>>> The first modification of the framework is the mechanism
>>>> to communicate with the DiagnosticCommandsMBean defined
>>>> in the JDK. Communications are performed via the jmm.h
>>>> interface. In addition to a few new entries in the
>>>> jmm structure, several data structures have been declared
>>>> to export diagnostic command meta-data to the JDK.
>>>>
>>>> The second modification of the framework consists in an
>>>> export control mechanism. Diagnostic Commands are executed
>>>> inside the JVM, they have access to critical information
>>>> and can perform very disruptive operations. Even if the
>>>> DiagnosticCommandsMBean includes some security checks
>>>> using Java Permissions, it sometime better to simply
>>>> make a diagnostic command not available from the JMX
>>>> API. The export control mechanism gives to diagnostic
>>>> command developers full control on how the command will
>>>> be accessible. When a diagnostic command is registered
>>>> to the framework, a list of interfaces allowed to
>>>> export this command must be provided. The current
>>>> implementation defines three interfaces: JVM internal,
>>>> attachAPI and JMX. A diagnostic command can be
>>>> exported to any combination of these interfaces.
>>>>
>>>> Modifications of existing diagnostic commands:
>>>>
>>>> Because this feature allows remote invocations, it
>>>> is important to be able to control who is invoking
>>>> and what is invoked. Java Permission checks have
>>>> been introduced in the DiagnosticCommandsMBean and
>>>> are performed before a remote invocation request
>>>> is forwarded to the JVM. So, each Diagnostic Command
>>>> can require a Java Permission to be invoked remotely.
>>>> Note that invocations from inside the JVM or via the
>>>> attachAPI do not check these permissions. Invocations
>>>> from inside the JVM are considered trusted, and the
>>>> attachAPI has its own security model based on EUID/
>>>> EGID.
>>>> Some existing diagnostic commands that have been
>>>> identified as begin potentially harmful have been
>>>> extended to specify the Java Permission required
>>>> for their execution.
>>>>
>>>> The webrev is here:
>>>>
>>>> http://cr.openjdk.java.net/~fparain/8004095/webrev.00/
>>>>
>>>> Thanks,
>>>>
>>>> Fred
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Frederic Parain - Oracle
>>>> Grenoble Engineering Center - France
>>>> Phone: +33 4 76 18 81 17
>>>> Email: Frederic.Parain at Oracle.com
>>>>
>>>
>>
>> --
>> Frederic Parain - Oracle
>> Grenoble Engineering Center - France
>> Phone: +33 4 76 18 81 17
>> Email: Frederic.Parain at Oracle.com
>>
>

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com


More information about the hotspot-runtime-dev mailing list