Request for Review (XXL): 7104647: Adding a diagnostic command framework

[Mandy Chung]

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