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.par...@oracle.com
