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 <[email protected]> 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
