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

[Gerard Ziemski]

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.)
>   
>
>