diagnosticCommand.cpp:259: nit: wrong indentation Otherwise: Looks good to me!
Thanks, /Staffan On 17 dec 2012, at 16:01, Frederic Parain <frederic.par...@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.par...@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.par...@oracle.com >>> >> > > -- > Frederic Parain - Oracle > Grenoble Engineering Center - France > Phone: +33 4 76 18 81 17 > Email: frederic.par...@oracle.com >
