Open code review for 8061999 Enhance VM option parsing to allow options to be specified
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Jul 17 18:47:52 UTC 2015
Ron,
The code that we've just reviewed and I'm sponsoring from Jeremy Manson
directly conflicts with this code. Jeremy has added a RESII class for
JavaVMInitArgs that you should use. I have other initial comments but I
haven't gotten to reading all of this thread yet. Please wait for my
code review.
Thanks,
Coleen
On 7/16/15 4:00 PM, Ron Durbin wrote:
> I will file it and thanks for the "r" review.
>
>> -----Original Message-----
>> From: gerard ziemski
>> Sent: Thursday, July 16, 2015 1:48 PM
>> To: Ron Durbin; hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: Open code review for 8061999 Enhance VM option parsing to allow options to be specified
>>
>> hi Ron,
>>
>> I have no more comments, just one question: you say "A future
>> enhancement will be to refactor the options file processing to grow
>> memory like the environment variables do now." - is that enhancement
>> filed yet?
>>
>> Please consider this reviewed with small "r".
>>
>>
>> cheers
>>
>>
>> On 07/14/2015 05:21 PM, Ron Durbin wrote:
>>> 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.)
>>>>
>>>>
>>>>
More information about the hotspot-runtime-dev
mailing list