RFR(S) : 8252477 : nsk/share/ArgumentParser should expect that jtreg "splits" an argument
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Sep 1 08:09:27 UTC 2020
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
>>
More information about the serviceability-dev
mailing list