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

[Frederic Parain]

On 12/7/11 2:32 AM, Mandy Chung wrote:
> On 12/2/2011 5:57 AM, Frederic Parain wrote:
>
>> http://cr.openjdk.java.net/~fparain/7104647/webrev.jdk.00/
> L112: Since you are in this file, can you fix the typo
> "java.security.SecurityException" to "java.lang.SecurityException"?

Fixed.

> 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.

This changeset contains the framework, and the framework
only throws NPE or IAE. However, a diagnostic command
impementation is free to throw any other exception. If such
an exception is thrown, it is simply printed on the output
stream if the command has been invoked through the attachAPI,
or it is propagated like any other exception through JMX
if the command has been invoked from the HotSpotDiagnosticMXBean.

> HotSpotDiagnostic.java
> L45-49: jvm is not used and can be removed.

Removed.

> L133: do you need to check if command == null and throw NPE?
> or NPE will be thrown somewhere?

The native code is protected against null commands, it will
throw the NPE.

> L159: NPE should be thrown instead of IAE, right? Unless
> the javadoc for the execute method is modified to reflect that.

Fixed.

> 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).

It doesn't really help, because native code has allocations
to performs anyway (for argument info).

> ManagementFactoryHelper.java
> L270: The new parameter "jvm" doesn't seem to be needed.

Fixed.

> 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.

Fixed

> HotSpotDiagnostic.c
> L44, 51, 76-85: indentation needs fixing.

Fixed.

> 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?

If a mismatch is detected, the implementation now throws an
UnsupportedOperationException("Diagnostic commands are not supported by 
this VM").

> jcmd.1 (man page)
> Should PerfCounter.print be listed in the man page? Or
> treat it like other diagnostic commands that are
> implementation-dependent?

PerfCounter is a built-in command, it doesn't appear in the
list of commands returned by the 'help' command. This is why
it is documented in the jcmd man page.

I'll upload new webrevs tomorrow morning.

Thanks,

Fred

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at oracle.com