RFR:8060449:Proper error messages for newly obsolete command line flags.

[Daniel D. Daugherty]

On 11/12/14 1:04 PM, Max Ockner wrote:
> Dan,
> I have reformatted  the  "){" fragment on line 336 as you recommended. 
> Thanks for catching that.

Thanks.


> For your second recommendation, I think I have a use case where the 
> recommended code would not function properly:
>
> Let's say there is a boolean flag SomeFlag, and let's say that the 
> user tries to type  "-XX:SomeFlagg".
>
> The first if statement passes because strlen("SomeFlagg") = 
> strlen("SomeFlag")+1.
> The second conditional checks if (strncmp(flag_status.name, s, f_len) 
> == 0).  But f_len, the length of "SomeFlag" is 8. The result is that 
> the 9th character of the user's input, which is where s differs from 
> flag_status.name, is not checked,so this condition is passed as well.

Your use case catches a bug in what I posted. I had originally
planned to change the two strncmp() calls to strcmp() so that
we get a complete match, but then I couldn't remember if a
straight strcmp() triggers Parfait warnings so I couldn't
finish reasoning my way through that maze...

Switching the 'f_len' parameter to 's_len' would solve the
problem without triggering Parfait, but it is totally your
call.

Dan

>
> Thanks,
> Max
>
>
>
> On 11/12/2014 1:41 PM, Daniel D. Daugherty wrote:
>> On 11/7/14 12:13 PM, Max Ockner wrote:
>>> ID: 8060449
>>> webrev: http://cr.openjdk.java.net/~coleenp/8060449/
>>
>> src/share/vm/runtime/arguments.cpp
>>
>>     line 336: ) {
>>         This fragment is on a line by itself and far left.
>>         Minimally, it should align like this:
>>
>>         line 331:     if (...
>>         line 336:        ) {
>>
>>     However, I recommend a slightly different structure to
>>     this logic:
>>
>>     size_t f_len = strlen(flag_status.name);
>>     size_t s_len = strlen(s);
>>     if (f_len == s_len || (f_len + 1) == s_len) {
>>       // this flag is the right length for a possible match
>>       if (strncmp(flag_status.name, s, f_len) == 0) ||
>>           ((s[0] == '+' || s[0] == '-') &&
>>            strncmp(flag_status.name, &s[1], f_len) == 0)) {
>>         // this flag is an exact match
>>         if (JDK_Version::current().compare(flag_status.accept_until) 
>> == -1) {
>>         ...
>>         }
>>       }
>>     }
>>     i++;
>>
>> I have no idea if the above formatting is going to be
>> preserved by e-mail clients...
>>
>> Dan
>>
>>
>>>
>>> Summary: A "newly obsolete" command line option is one which is no 
>>> longer supported, but still is acknowledged. There is a list of 
>>> these in arguments.cpp.
>>> It used to be that only a fixed number of characters were checked 
>>> when comparing a given command line option to the list of obsolete 
>>> flags (strncmp was used, where the number of characters to check is 
>>> equal to the length of the flag name from the table.)
>>> As a result, an arbitrary string appended to the end of an obsolete 
>>> argument goes unnoticed.
>>> This issue is fixed by comparing the lengths of the given flag and 
>>> the flags from the obsolete flags table.
>>> When a misspelled flag is fuzzy-matched to an obsolete flag, an 
>>> appropriate warning is given to save the user a few key strokes: (1) 
>>> unrecognized option [bad option]. (2) Did you mean [option]? (3) 
>>> [option] is obsolete as of [version])
>>>
>>> A new test for this feature checks for the presence of all three 
>>> components of the above error message.
>>>
>>> Tested with: vm.quick.testlist
>>>              hotspot jtreg tests
>>>              jprt
>>>
>>> Thanks for your help!
>>> Max Ockner
>>>
>>>
>>
>