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

[Coleen Phillimore]

I think this looks better also.   I have a small nit, can you add these 
{}s to:

*+   if (equal_sign == NULL)*
*+     arg_len = strlen(argname);*
*+   else*
*+     arg_len = equal_sign - argname;

*

It's sort of in the coding standard, not strongly enforced, but in 
general we do have the full set of {}s to keep people from making 
errors.   Or use ?:  which would make it shorter.

*+   if (equal_sign == NULL)*  {
*+     arg_len = strlen(argname);*
*+   } else*  {
*+     arg_len = equal_sign - argname;
+   }
*


Thanks,
Coleen

On 01/31/2013 09:55 AM, 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/12ae0794/attachment.html