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 <[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
