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.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
--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com
