Request for review: 8006298: Specifying malformed JFR options (--XX:+FlightRecorderOptions) outputs non-sensical error

[Mikael Vidstedt]

Thanks Harold!

Cheers,
Mikael

On 2013-01-31 10:39, harold seigel wrote:
> Good catch!
>
> I'll change it to strchr() before checking it in.
>
> Thanks, Harold
>
> On 1/31/2013 12:34 PM, Mikael Vidstedt wrote:
>>
>>
>> Harold,
>>
>> Thanks for making the changes. One further question (sorry for not 
>> asking this the first time around):
>>
>> Is there a fundamental reason for finding the last equals ('=') 
>> instead of the first one - i.e. would it make more sense to use 
>> strchr instead of strrchr? Not that I think we have any options that 
>> accept multiple equals today, but...
>>
>> Cheers,
>> Mikael
>>
>> On 2013-01-31 06:55, harold seigel wrote:
>>> Hi,
>>>
>>> Thank you for your comments!
>>>
>>> Please review this updated webrev: 
>>> http://cr.openjdk.java.net/~hseigel/bug_8006298_2/ 
>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8006298_2/>
>>>
>>> It avoids the 'const' and '\0' issues by determining the length of 
>>> the argument and passing it to find_flag().
>>>
>>> This changes the output messages slightly, by including the '=...' 
>>> text.  For example:
>>>
>>>      % $JAVA_HOME/bin/java -XX:+UseLargePages=8 -version
>>>     Improperly specified VM option 'UseLargePages=8'
>>>
>>>     % $JAVA_HOME/bin/java -XX:-ObjectAlignmentInBytes=16 -version
>>>     Unexpected +/- setting in VM option 'ObjectAlignmentInBytes=16'
>>>
>>> Thanks, Harold
>>>
>>>
>>> On 1/30/2013 10:39 PM, Vitaly Davidovich wrote:
>>>>
>>>> I'd also change "equal_sign[0] = 0;" to "equal_sign[0] = '\0';" if 
>>>> this line is to be kept - it's more idiomatic.
>>>>
>>>> Thanks
>>>>
>>>> Sent from my phone
>>>>
>>>> On Jan 30, 2013 8:34 PM, "Mikael Vidstedt" 
>>>> <mikael.vidstedt at oracle.com <mailto:mikael.vidstedt at oracle.com>> wrote:
>>>>
>>>>
>>>>     Harold,
>>>>
>>>>     Thanks for doing this, I like the improved messages a lot!
>>>>
>>>>     One small question:
>>>>
>>>>     The first argument to process_argument is a "const char* arg".
>>>>     argname is also const, based on arg. You do this:
>>>>
>>>>     char* equal_sign = (char *)strrchr(argname, '=');
>>>>     if (equal_sign > argname)
>>>>     equal_sign[0] = 0;
>>>>
>>>>     Doesn't that effectively break the const'ness of the incoming
>>>>     argument?
>>>>
>>>>     Cheers,
>>>>     Mikael
>>>>
>>>>     On 2013-01-30 10:51, harold seigel wrote:
>>>>>     Hi,
>>>>>
>>>>>     Please review the following change to fix bug 8006298.
>>>>>
>>>>>     Summary:
>>>>>     This change enables hotspot to emit more useful messages when
>>>>>     Java options are specified incorrectly.
>>>>>
>>>>>     This was tested using JCK, UTE, and JTREG tests, and by hand. 
>>>>>     Below are the test cases that were run by hand.  Please let me
>>>>>     know if you have any additional suggestions.  (The "Error:..."
>>>>>     messages appeared in all cases but are only shown for the
>>>>>     initial case.)
>>>>>
>>>>>         $JAVA_HOME/bin/java -XX:UseLargePages -version
>>>>>         Missing +/- setting for VM option 'UseLargePages'
>>>>>         Error: Could not create the Java Virtual Machine.
>>>>>         Error: A fatal exception has occurred. Program will exit.
>>>>>
>>>>>         $JAVA_HOME/bin/java -XX:+UseLargePages=8 -version
>>>>>         Improperly specified VM option 'UseLargePages'
>>>>>
>>>>>         $JAVA_HOME/bin/java -XX:ObjectAlignmentInBytes=v -version
>>>>>         Improperly specified VM option 'ObjectAlignmentInBytes'
>>>>>
>>>>>         $JAVA_HOME/bin/java -XX:-ObjectAlignmentInBytes=16 -version
>>>>>         Unexpected +/- setting in VM option
>>>>>         'ObjectAlignmentInBytes'   <-- 64 bit VM's
>>>>>         Unrecognized VM option
>>>>>         'ObjectAlignmentInBytes'                <-- 32 bit VM's
>>>>>
>>>>>         $JAVA_HOME/bin/java -XX:bogus_option -version
>>>>>         Unrecognized VM option 'bogus_option'
>>>>>
>>>>>     Open webrev at
>>>>>     http://cr.openjdk.java.net/~hseigel/bug_8006298/
>>>>>     <http://cr.openjdk.java.net/%7Ehseigel/bug_8006298/>
>>>>>
>>>>>     Bug link at http://bugs.sun.com/view_bug.do?bug_id=8006298
>>>>>
>>>>>     Thanks!  Harold
>>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130131/a94219ac/attachment-0001.html