On 12/2/2011 5:57 AM, Frederic Parain wrote:
http://cr.openjdk.java.net/~fparain/7104647/webrev.jdk.00/
ManagementPermission.java
The diagnostic command is not part of the platform.
It doesn't seem right to add this new "diagnosticCommand"
ManagementPermission. Are the existing "control" and
"monitor" permission not adequate for the diagnostic
command use?
On the other hand, I expect that the required
ManagementPermission should be specified in the new methods
added in HotSpotDiagnosticMXBean (see below).
HotSpotDiagnosticMXBean.java
sun.managment.HotSpotDiagnostic does check if the execute
method has the "diagnosticCommand" permission. The javadoc
must specify the SecurityException be thrown under what
condition.
To me, the execute methods could require
ManagementPermission("control") as the setVMOption method.
I'm not certain if DiagnosticCommandInfo is considered as
non-sensitive information. Have you checked with the
security team to get their advice?
L112: Since you are in this file, can you fix the typo
"java.security.SecurityException" to "java.lang.SecurityException"?
The execute method doesn't throw any exception other than
IAE. What happens if the specified command execution fails
in the target VM? If no exception is thrown, how does the
caller detect if the command succeeds or not and handle it
gracefully? It seems that this mechanism should provide
a way to detect if a command succeeds or fails programmatically
rather than burying the error message in the returned string.
The spec needs some clarification about the list of diagnostic
commands may change during JVM execution.
HotSpotDiagnostic.java
L45-49: jvm is not used and can be removed.
L133: do you need to check if command == null and throw NPE?
or NPE will be thrown somewhere?
L159: NPE should be thrown instead of IAE, right? Unless
the javadoc for the execute method is modified to reflect that.
L176: minor: I wonder if it's any simpler for this native method
to take the DiagnosticCommandInfo[] argument (i.e. the caller
method does the array allocation than in the native method
implementation).
ManagementFactoryHelper.java
L270: The new parameter "jvm" doesn't seem to be needed.
Util.java
See comments in ManagementPermission.
jmm.h
L316-326: the parameters should be lined up with the first one.
L316-317 was aligned improperly and it's good to fix them
in this batch.
HotSpotDiagnostic.c
L44, 51, 76-85: indentation needs fixing.
L158, 172: this should never happen unless the hotspot/jdk is
mismatched. But would it better to throw an exception with
an informative message to help diagnostic in case the mismatch
does happen?
jcmd.1 (man page)
Should PerfCounter.print be listed in the man page? Or
treat it like other diagnostic commands that are
implementation-dependent?
That's all the comments I have on the jdk side.
Mandy
