Open code review for 8061999 Enhance VM option parsing to allow options to be specified
ron.durbin at oracle.com
Fri Jul 10 12:33:17 UTC 2015
>From: David Holmes
>Sent: Thursday, July 09, 2015 11:57 PM
>To: hotspot-runtime-dev at openjdk.java.net
>Subject: Re: Open code review for 8061999 Enhance VM option parsing to allow options to be specified
>Is there an updated webrev addressing Gerard's comments?
>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:
>> 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()"?
>> 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);
>> 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.
>> On 6/22/2015 7:52 AM, Ron Durbin wrote:
>>> Webrev URL:
>>> RFE request:
>>> 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
>>> 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
>>> and up to 64 options.
>>> This feature has been tested on:
>>> Solaris, MAC, Windows, Linux
>>> Manual unit tests
>>> JPRT with -testset hotspot (including the SQE proposed test
>>> coverage for this feature.)
More information about the hotspot-runtime-dev