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

[Staffan Larsen]

diagnosticCommand.cpp:259: nit: wrong indentation

Otherwise: Looks good to me!

Thanks,
/Staffan

On 17 dec 2012, at 16:01, Frederic Parain <frederic.parain at oracle.com> 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
>