Hi Robert,
I've added serviceability to the cc list.
On 17/01/2012 12:04 PM, Robert Ottenhag wrote:
Webrev: http://cr.openjdk.java.net/~rottenha/7130391/webrev.00
This fix adds optional validation control to the setting of command-line
switches in Hotspot, and allows it to have vendor-specific extensions if
necessary.
Does this imply that the Java management APIs (eg
com.sun.management.VMOption) need to be changed to reflect these
restrictions? Presently VMOptions are either writeable or not, but this
makes them conditionally-writeable.
The design follows the previously added framework for vendor-specific
command-line switch extensions in CR7117389.
The validation control is handled by new boolean methods
Flag::is_valid_<type>(value,origin) that are called at the beginning of each call to
CommandLineFlags[Ex]::<type>AtPut() to verify that the new value and origin are valid
replacements for the current value and origin for this flag.
When parsing the command line options, a failed validation will typically result in an error
message and exit with "Unrecognized VM option '<flag-name>'". When used
dynamically using the attach API or management API the resulting operation will fail, leaving
it up to the caller to handle it as appropriate.
The error message doesn't really seem appropriate - it may well be a
recognized option, you just can't set it to that value in that way.
Ideally there would be a way for the validation logic to supply a
meaningful error message. In its absence the top-level message should
reflect the new type of error.
Also some of the failures lead to crashes - which seems wrong to me -
see below.
----
src/share/vm/services/management.cpp:
1821 if (!succeed) {
1822 THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
1823 "This flag is not writeable with this value or origin.");
That's a rather cryptic error message. How about:
"Flag can not be set to the requested value using this API"
?
----
src/share/vm/runtime/globals_ext.hpp
With all the
inline bool Flag::is_valid_ext_T(T value, FlagValueOrigin origin)
functions, is it necessary to include the type T in the function name?
-----
src/share/vm/runtime/globals.cpp
The use of the guarantees seems wrong as it means an invalid option will
trigger a VM crash rather than a clean exit during initialization. It
seems to me that none of the code in arguments.cpp that uses the
FLAG_SET_* macros (which in turn use the CommandLineFlagsEx functions
you added the guarantees to) anticipates any possibility for failure. I
think if you are going this path then you have no choice but to change
the CommandLineFlagsEx methods to return bool and update the FLAG_SET
macros to try and perform appropriate error handling.
David
-----
A simple use case for validation is a manageable flag whose current value can
not be less than the previous value, while a more complex example may base the
validation on multiple other flags, etc.
Thanks,
/Robert
