Open code review for 8061999 Enhance VM option parsing to allow options to be specified

[David Holmes]

Is there an updated webrev addressing Gerard's comments?

Thanks,
David

On 24/06/2015 5:52 AM, Gerard Ziemski wrote:
> hi Ron,
>
> I'm sending you partial feedback, since I am starting to get a bit
> tired. I will spend more time reviewing your webrev tomorrow, but here
> is what I have so far:
>
> ---
> src/share/vm/utilities/globalDefinitions.hpp
>
> 1. Should we name the method "is_white()", not "iswhite()" since it's
> part of our code base? But then since it's a macro, shouldn't it
> actually be "IS_WHITE()"?
>
> ---
> src/share/vm/runtime/arguments.hpp
>
> 1.
>
> All the arguments in alloc_JVM_options_list(),
> copy_JVM_options_from_buf() and parse_JVM_options_file() use underscores
> in the arguments names except for merge_JVM_options_file(). I think the
> merge_JVM_options_file() should be:
>
> +  static jint merge_JVM_options_file(const struct JavaVMInitArgs *args_in,
> +                                     struct JavaVMInitArgs **args_out);
>
>
> ---
> src/share/vm/runtime/arguments.cpp
>
> 1. Why do FreeVMOptions, DumpVMOptions, DumpVMOption and DumpOption
> start with capital letters? Shouldn’t their names start with a lower
> case letter?
>
> 2. Line 4306. The pattern in Arguments::parse() seems to be to print out
> error message and return the error value if something goes wrong, but we
> do vm_exit(1) instead?
>
> 3. Line 4309. I was told that when it comes to NULL pointer check we
> should do (NULL == args), not the other way around.
>
> 4. Line 4375. Don't we need FreeVMOptions() here? Line 4382 as well?
>
> 5. Question. Why do we need N_MAX_OPTIONS?
>
> 6. Question. Why do we need OPTION_BUFFER_SIZE? Can’t we use “int
> bytes_needed = fseek(stream, 0, SEEK_END); rewind(stream)” and have the
> code dynamically allocate memory without hard coded limits?
>
> 7. Line 3965. Can that comparison ever succeed? “read()” will not read
> more bytes (only less) than as specified by “bytes_alloc” and if it did,
> we would overwrite memory since our buf is only “bytes_alloc” big.
>
>
>
> cheers
>
>
> On 6/22/2015 7:52 AM, Ron Durbin wrote:
>> Webrev URL:
>> http://cr.openjdk.java.net/~rdurbin/8061999_OCR0_JDK9_webrev
>>
>>
>> RFE request:
>> https://bugs.openjdk.java.net/browse/JDK-8061999
>>
>> This RFE allows a file to be specified that holds VM Options that
>> would otherwise be specified on the command line or in an environment
>> variable.
>> Only one options file may be specified on the command line and no
>> options file
>> may be specified in either of the following environment variables
>> "JAVA_TOOL_OPTIONS" or "_JAVA_OPTIONS".
>>
>> The options file feature supports all VM options currently supported on
>> the command line, except the options file option. The option to
>> specify an
>> options file is "-XX:VMOptionsFile=<Filename>".
>> The options file feature supports an options file up to 1024 bytes in
>> size
>> and up to 64 options.
>>
>> This feature has been tested on:
>>    OS:
>>      Solaris, MAC, Windows, Linux
>>    Tests:
>>      Manual unit tests
>>      JPRT with -testset hotspot (including the SQE proposed test
>> coverage for this feature.)
>>
>>
>