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

[Ron Durbin]

Gerard,

Thanks for your time in reviewing the code and providing comments.
My responses to your comments inline below:


>-----Original Message-----
>From: Gerard Ziemski
>Sent: Tuesday, June 23, 2015 1:53 PM
>To: hotspot-runtime-dev at openjdk.java.net
>Cc: Ron Durbin
>Subject: Re: Open code review for 8061999 Enhance VM option parsing to allow options to be specified
>
>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.hp
>
>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
>[
 
[Ron] While researching my answer to this comment, I realized
       that I should have used the already existing isspace() function
       instead of creating the iswhite() macro. Will fix.
 
>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);
>


[Ron] This one is a reasonable clean up

>
>---
>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?
>

[Ron] This one is a reasonable clean up

>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?
>


[Ron ] The cases that will trigger this exit  on fail are extreme:
     - missing or otherwise in accessible options files
     -  Un-parsable options file, too big, too many options, nonwhite space terminated options
     - Unable to allocate memory for options files processing

>3. Line 4309. I was told that when it comes to NULL pointer check we should do (NULL == args), not the other way
>around.
>

[Ron] This one is a reasonable clean up

>4. Line 4375. Don't we need FreeVMOptions() here? Line 4382 as well?
 

[Ron] Gerard  this one is a reasonable clean up

>5. Question. Why do we need N_MAX_OPTIONS?


[Ron] Until recently N_MAX_OPTIONS applied to environment variables too
       N_MAX_OTIIONS is used  to limit the number of options and thus the memory allocated. 
       A future enhancement will be to refactor the options file processing to grow memory like the
       environment variables do now.

>
>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?
>

[Ron>] Until recently OPTION_BUFFER_SIZE applied to environment variables too. \
       OPTION_BUFFER_SIZE is used to limit the memory allocated for reading the options.
       A future enhancement will be to refactor the options file processing to grow memory like the
       environment variables do now.


>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.
>
[Ron>] Yes that comparison can succeed. We only support an
       options file that is <= OPTION_BUFFER_SIZE bytes in
       size.

        We allocate a read buffer that is OPTION_BUFFER_SIZE + 1
        bytes in size for two reasons:

       1) to have space for a NULL terminator when the
       options file is the maximum number of bytes
       in length
       2) easy detection of an options file that is too large;
       we try to read OPTION_BUFFER_SIZE + 1 bytes. If we
       succeed, then the file is too 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.)
>
>
>