[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: RFR(S) : 8252477 : nsk/share/ArgumentParser should expect that jtreg "splits" an argument
From:       David Holmes <david.holmes () oracle ! com>
Date:       2020-08-31 4:18:18
Message-ID: a15cbac3-4dc4-8b6c-cb40-9fbd19504688 () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]