RFR (S): 7130391: Add a framework for vendor-specific validation control of setting command-line switches in Hotspot

[David Holmes]

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
>