Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments (original) (raw)

Gerard Ziemski gerard.ziemski at oracle.com
Mon Jun 1 21:24:40 UTC 2015

hi Bengt,

Thank you for taking the time to provide your feedback. My answers are provided in-line:

On 5/29/2015 1:35 AM, Bengt Rutisson wrote:

Hi Gerard, Not a full review, but just a couple of questions. The new constraint methods added in commandLineFlagConstraintsGC.cpp are all just implementations of the existing constraints for the GC flags, right? Basically these checks are just moved from arguments.cpp to the constraint methods, or have any new ones been added?

The constraints were moved, as well as many ranges, though about 60% of ranges are new and were implemented based on comments and/or their names (ie. percentage in name implies 0..100)

All methods in commandLineFlagConstraintsGC.cpp and commandLineFlagConstraintsCompiler.cpp start with this check: if ((CommandLineFlags::finishedInitializing() == true) Only the runtime flags, checked in commandLineFlagConstraintsRuntime.cpp, are checked even when initialization has not been completed. Why is it important to be able to check the constraints even before we have finished initializing? Wouldn't it be simpler to just call the constraint methods once after we've reached CommandLineFlags::finishedInitializing()? Then all the constraint methods wouldn't have to remember to check for that.

The ideal flow for the flags is "define --> check --> use", however, that flow does not represent all the flags. There are flags that are set and then used to set other flags and parts of VM before the final check and therefore require validation earlier, hence the need for the "finishedInitiliazing". Personally, I don't like the added complexity myself either, but I think this is the best we can do for now without making lots of other changes to the code.

(BTW, it is error prone to use == for boolean values. Better to just use "if (CommandLineFlags::finishedInitializing())" .)

Fixed.

I will be posting up a new webrev shortly.

cheers

More information about the hotspot-dev mailing list