RFR: 8004095: Add support for JMX interface to Diagnostic Framework and Commands,
karen.kinnear at oracle.com
Thu Jan 10 12:28:40 PST 2013
Finally getting to a code review. I like the design and the code looks very clean.
I had a couple of questions please:
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?
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?
who deallocates the JavaPermission strings?
Don't we have the same issue with the name, description and impact already?
That they never get freed?
5. diagnosticFramework.hpp line 216 "DCmdParser parse" -> "DCmdParser parses"
line 220 "relatively" -> "relative"
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
line 259 "are disabled" -> "is disabled"
8. VMManagementImpl.c - for 7150256
line 394 "least one the" -> "least one of the"
DiagnosticCommandMBean.java - for 7150256
line 76 "doesn't" -> "don't"
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
9. sorry - copyrights will all want to be 2013 now
On Dec 17, 2012, at 10:01 AM, Frederic Parain wrote:
> Thank you for the review, I've fixed the code to address all
> the issue you reported. The new webrev is here:
> On 12/17/12 01:27 PM, Staffan Larsen wrote:
>> 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.)
>> On 17 dec 2012, at 12:26, Frederic Parain <frederic.parain at oracle.com> wrote:
>>> 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/
>>> 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:
>>> 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