RFR(S) : 8252477 : nsk/share/ArgumentParser should expect that jtreg "splits" an argument

[serguei.spitsyn at oracle.com]

Hi Igor,

LGTM++
It is a nice clean. Surprisingly, this code had a lot of typos!

Thanks,
Serguei


On 8/30/20 21:18, David Holmes wrote:
> Hi Igor,
>
> Update looks good.
>
> Thanks,
> David
>
> On 29/08/2020 4:18 am, Igor Ignatyev wrote:
>> Hi David,
>>
>> good point, parseArguments (or rather checkOption) does indeed 
>> validate that passed option is valid and has a valid value, yet for 
>> many options all values are treated as valid, so ill-formed command 
>> lines like  `-debugee.vmkeys="${test.vm.opts} ${test.java.opts} 
>> -transport.address=dynamic` won't be spotted by ArgumentParser and 
>> its sub-classes, and I'm afraid in some cases might change tests' 
>> behavior unnoticeably. thus I've decided to add the check that we 
>> always have even number of double quotes:
>>
>>> diff -r 83f273f313aa 
>>> test/hotspot/jtreg/vmTestbase/nsk/share/ArgumentParser.java
>>> --- a/test/hotspot/jtreg/vmTestbase/nsk/share/ArgumentParser.java 
>>> Thu Aug 27 19:37:51 2020 -0700
>>> +++ b/test/hotspot/jtreg/vmTestbase/nsk/share/ArgumentParser.java 
>>> Fri Aug 28 11:16:24 2020 -0700
>>> @@ -156,6 +156,10 @@
>>>                   arg.append(" ").append(args[++i]);
>>>                   doubleQuotes += numberOfDoubleQuotes(args[i]);
>>>               }
>>> +            if (doubleQuotes % 2 != 0) {
>>> +                throw new TestBug("command-line has odd number of 
>>> double quotes:" + String.join(" ", args));
>>> +            }
>>> +
>>>               list.add(arg.toString());
>>>           }
>>>           setRawArguments(list.toArray(String[]::new));
>>
>>
>> Thanks,
>> -- Igor
>>
>>
>>> On Aug 27, 2020, at 9:09 PM, David Holmes <david.holmes at oracle.com> 
>>> wrote:
>>>
>>> Hi Igor,
>>>
>>> In case there may be a parsing error and the command-line is 
>>> ill-formed, should you abort if you reach the end of the arg list 
>>> without finding an even number of double-quotes? Or will 
>>> parseArguments already handle that?
>>>
>>> Otherwise the changes seem good.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>> On 28/08/2020 12:39 pm, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//8252477/webrev.00/
>>>>> 99 lines changed: 19 ins; 20 del; 60 mod;
>>>> Hi all,
>>>> could you please review the patch which unblocks the rest of 
>>>> 8219140's (get rid of vmTestbase/PropertyResolvingWrapper) sub-tasks?
>>>> background from JBS:
>>>>> jtreg splits command line by space to get the list of arguments 
>>>>> and there is no way to prevent that (nor thru escaping, nor by 
>>>>> adding quotes). currently, PropertyResolvingWrapper handles that 
>>>>> and joins multiple arguments within double quotes into one 
>>>>> argument before passing it to the actual test class. the only 
>>>>> place where it's needed is in the tests which use 
>>>>> nsk/share/ArgumentParser (or more precisely 
>>>>> nsk.share.jpda.DebugeeArgumentHandler and 
>>>>> nsk/share/jdb/JdbArgumentHandler).
>>>>>
>>>>> in preparation for PropertyResolvingWrapper removal, 
>>>>> ArgumentParser should be updated to handle the "split" argument on 
>>>>> its own.
>>>> I've also taken the liberty to slightly clean up ArgumentParser.
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8252477
>>>> webrev: http://cr.openjdk.java.net/~iignatyev//8252477/webrev.00/
>>>> testing: all the tests which use ArgumentParser 
>>>> (:vmTestbase_nsk_aod :vmTestbase_nsk_jdb :vmTestbase_nsk_jdi 
>>>> :vmTestbase_nsk_jdw ,:vmTestbase_nsk_jvmti :vmTestbase_vm_compiler 
>>>> :vmTestbase_vm_mlvm) on {windows,linux,macos}-x64
>>>> Thanks,
>>>> -- Igor
>>