Thanks Dan, I intend to put down Dan, Dmitry and Staffan as reviewers for this change.
Thank you Markus > -----Original Message----- > From: Daniel D. Daugherty > Sent: den 6 april 2012 03:49 > To: Markus Grönlund > Cc: [email protected]; hotspot-runtime- > [email protected] > Subject: Re: RFR(XS): 7154809 JDI: update JDI/JDB debugee commandline > option parsing (allow nested comma delimited options) + sponsor request > > On 4/5/12 5:43 AM, Markus Grönlund wrote: > > Dan, Dmitry, all, > > > > Thanks for the feedback so far, I have updated the change, please see > this update: > > > > Webrev: > > http://cr.openjdk.java.net/~mgronlun/7154809/webrev06/ > > Thumbs up! > > src/share/classes/com/sun/tools/example/debug/tty/VMConnection.java > > nit line 119: should be 'chars' (not a contraction) > > nit line 184: indented 1 space too much > > nit line 226: space around '+' > > Dan > > > > Bug: > > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7154809 > > > > > > > > The use case we are trying to support is the parsing of VM Options > being passed in via the "options=" > > > > These VM options will either be: > > > > 1. undecorated (-XX:+PrintVMOptions) > > 2. double quoted ("-XX:+PrintVMOptions") > > 3. single quoted ('-XX:+PrintVMOptions') > > 4. nested quoted ('"-XX:+PrintVMOptions"', or "'- > XX:+PrintVMOptions'") > > > > Above change results in: > > > > -XX:+PrintVMOptions > > > > For all the above. > > > > > > Example (the extra \ qualifiers is to avoid shell resolution for > windows commandline): > > > > java TestApplication \"-client\" \"-Xmixed\" \"-XX:+PrintVMOptions\" > \"'-XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'\" > > Printing raw arguments: > > "-client" > > "-Xmixed" > > "-XX:+PrintVMOptions" > > "'-XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'" > > > > Printing raw arguments stringed together: "-client" "-Xmixed" "- > XX:+PrintVMOptions" "'- > XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'" > > > > strings after splitStringAtNonEnclosedWhitespace(): > > > > "-client" > > now sending string: "-client" off to isEnclosed() > > After processing string in isEnclosed: > > -client > > > > "-Xmixed" > > now sending string: "-Xmixed" off to isEnclosed() > > After processing string in isEnclosed(): > > -Xmixed > > > > "-XX:+PrintVMOptions" > > now sending string: "-XX:+PrintVMOptions" off to isEnclosed() > > After processing string in isEnclosed(): > > -XX:+PrintVMOptions > > > > "'-XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'" > > now sending string: "'- > XX:ExtendedExampleOptions=value1=true,value2=true,value3=false'" off to > isEnclosed() > > After processing string in isEnclosed(): > > -XX:ExtendedExampleOptions=value1=true,value2=true,value3=false > > > > Final result > > -client -Xmixed -XX:+PrintVMOptions - > XX:ExtendedExampleOptions=value1=true,value2=true,value3=false > > > > > > Thank you > > Markus > > > > > > p.s. > > > > Dan, thanks for the esoteric example. The output from the example > string (using debug version of code): > > > > The following is a single quote: ' Do you see it? > > > > Is (depending on how you pass it in (quoted or unquoted)): > > > > D:\devjava\1.6.0\projects\enclosed\source>java TestApplication a > single quote: ' do you see it? > > > > Printing raw arguments: > > > > a > > single > > quote: > > ' > > do > > you > > see > > it? > > > > Printing raw arguments stringed together: a single quote: ' do you > see it? > > > > strings after splitStringAtNonEnclosedWhitespace: > > > > a > > now sending string: a off to isEnclosed() > > After processing string in isEnclosed(): > > a > > > > single > > now sending string: single off to isEnclosed() > > After processing string in isEnclosed(): > > single > > > > quote: > > now sending string: quote: off to isEnclosed() > > After processing string in isEnclosed(): > > quote: > > > > ' > > now sending string: ' off to isEnclosed() > > After processing string in isEnclosed(): > > ' > > > > do > > now sending string: do off to isEnclosed() > > After processing string in isEnclosed: > > do > > > > you > > now sending string: you off to isEnclosed() > > After processing string in isEnclosed(): > > you > > > > see > > now sending string: see off to isEnclosed() > > After processing string in isEnclosed(): > > see > > > > it? > > now sending string: it? off to isEnclosed() > > After processing string in isEnclosed(): > > it? > > > > Final result > > a single quote: ' do you see it? > > > > D:\devjava\1.6.0\projects\enclosed\source>java TestApplication \"a > single quote: ' do you see it?\" > > > > Printing raw arguments: > > "a > > single > > quote: > > ' > > do > > you > > see > > it?" > > > > Printing raw arguments stringed together: "a single quote: ' do you > see it?" > > > > strings after splitStringAtNonEnclosedWhitespace > > "a single quote: ' do you see it?" > > now sending string: "a single quote: ' do you see it?" off to > isEnclosed() > > After processing string in isEnclosed(): > > a single quote: ' do you see it? > > > > Final result > > a single quote: ' do you see it? > > > > > > D:\devjava\1.6.0\projects\enclosed\source>java TestApplication 'a > single quote: \" do you see it?' > > > > Printing raw arguments > > 'a > > single > > quote: > > " > > do > > you > > see > > it?' > > Printing raw arguments stringed together: 'a single quote: " do you > see it?' > > strings after splitStringAtNonEnclosedWhitespace > > 'a single quote: " do you see it?' > > now sending string: 'a single quote: " do you see it?' off to > isEnclosed() > > After processing string in isEnclosed: > > a single quote: " do you see it? > > > > Final result > > a single quote: " do you see it? > > > > Cheers > > Markus > > > > > > > > > > > > > > > > > > > >> -----Original Message----- > >> From: Daniel D. Daugherty > >> Sent: den 29 mars 2012 22:30 > >> To: [email protected] > >> Cc: [email protected] > >> Subject: Re: RFR(XS): 7154809 JDI: update JDI/JDB debugee > commandline > >> option parsing (allow nested comma delimited options) + sponsor > request > >> > >> On 3/28/12 3:13 PM, Markus Grönlund wrote: > >>> Dan, Dmitry, everybody, > >>> > >>> Thank you so far for your input. > >>> > >>> Dan, you are absolutely right in your reflection about "<word1> > >> <word2>" combined with 'split("\\s+")' regexp - this will split the > >> strings to"<word1> and<word2>" which does not work. We should also > try > >> to cover this. > >>> This turned out a lot trickier than I originally thought - I have > >> tried to come up with some good regexps that would combine the > >> combination of what's needed (nested spaces, nested quotations > >> (different quotations), several levels...), however I did not get it > to > >> work correctly in all combinations with the regexps - I eventually > >> decided to take control over this down to the char level - hence > this > >> suggestion with a C-like array iteration (maybe slow yeah, however I > >> found this so much more debuggable and actually a chance to see > what's > >> going on. Also, this parsing for the "values"-string associated with > >> the "option=" is only parsed once at startup, just when getting the > >> correct VM Option parameters for/of the debugee). > >>> Updated webrev: > >>> http://cr.openjdk.java.net/~mgronlun/7154809/webrev05/ > >> src/share/classes/com/sun/tools/example/debug/tty/VMConnection.java > >> nit lines 118-119: extra space after '*' > >> > >> line 178: what happens when null is passed to format()? > >> lines 182, many others - missing space after '//' > >> > >> line 217 - please consider changing comment to: > >> > >> // this DOUBLEQ does not complete this enclosing - skip to > >> next > >> > >> line 224: please change comment to match line 256: > >> > >> //set up the target char for ending enclosing > >> > >> line 229 - please consider changing comment to: > >> > >> // this DOUBLEQ does not start a new enclosing - skip to > next > >> > >> line 233: add "is there" to beginning of comment to match line > 201 > >> > >> line 249 - please consider changing comment to: > >> > >> // this SINGLEQ does not complete this enclosing - skip to > >> next > >> > >> line 261 - please consider changing comment to: > >> > >> // this SINGLEQ does not start a new enclosing - skip to > next > >> > >> line 286: This check might be hit for an input string that > looks > >> like: > >> > >> The following is a single quote: ' Do you see it? > >> > >> Pretty bogus example so throwing that exception might be > the > >> right thing to do. The idea I have is that if you have a > >> singleton DOUBLEQ or SINGLEQ in the input, then you don't > >> have an enclosing state... May not be worth the hassle... > >> > >> Dan > >> > >> > >> > >>> There was eventually some more code ending up in this one (trying > to > >> be very explicit here) - it might make more sense to refactor this > to > >> outside of VMConnection.java someplace - if you have any ideas... > >>> Some of the word combinations supported with this change: > >>> > >>> "'<word1> <word2>'"<word3> '"<word4> <word5>"'"<word6> > <word7>" > >> '<word8> <word9>'<word10> "<wor"d11>" '<word'12>''<wor"d11>' > >> "<wor"d'12>""<word13> " .... > >>> I think I should need to track the changes to the options > >> passing/parsing test code in nsk testbase as well in a separate bug > for > >> correlation with these changes - might make more sense. I will file > a > >> test bug on the updates needed on the test side of things. > >>> Thanks again for your help > >>> Markus > >>> > >>> > >>> > >>>> -----Original Message----- > >>>> From: Daniel D. Daugherty > >>>> Sent: den 27 mars 2012 21:29 > >>>> To: Markus Grönlund > >>>> Cc: Dmitry Samersoff; [email protected]; > hotspot- > >>>> [email protected] > >>>> Subject: Re: RFR(XS): 7154809 JDI: update JDI/JDB debugee > >> commandline > >>>> option parsing (allow nested comma delimited options) + sponsor > >> request > >>>> On 3/27/12 11:56 AM, Markus Grönlund wrote: > >>>>> Hi again all, > >>>>> > >>>>> Thanks for the input Dan and Dmitry. > >>>>> > >>>>> I took a closer look at this, we can actually pass options to the > >> VM > >>>> which could involve filenames and these *could* have quotation > >>>> qualifiers embedded (ccstr type parameters); this means the > original > >>>> suggestion will not work correctly, in doing > >> value.replaceAll("['\"]", > >>>> ""), as it will also remove all quotations for any embedded > >> VMOptions > >>>> of ccstr type... > >>>>> Neither will Dmitrys suggestion of removing just the edge > >> quotations > >>>> work, at least not as-is, it would need foreach value iteration. > >>>>> I have updated the webrev with such a modification (which will > >>>> recursively remove quotations from outside-in for each value). > This > >>>> will allow for "'test'", '" test "', "" test "", ''test'', "test", > >>>> 'test' ...etc. > >>>>> Please see updated webrev here: > >>>>> > >>>>> http://cr.openjdk.java.net/~mgronlun/7154809/webrev04/ > >>>> > src/share/classes/com/sun/tools/example/debug/tty/VMConnection.java > >>>> I like the newer version and the concept of "enclosing > >> quotes". > >>>> One thing I'm not quite sure about is the use of 'split()'. > >> What > >>>> will the new logic do with the following value? > >>>> > >>>> "<word1> <word2>" > >>>> > >>>> I'm pretty sure the split() call will break the above into > two > >>>> tokens: '"<word1>' and'<word2>"'. The subsequent calls to > >>>> isEnclosed() will return false and we won't strip the > double > >>>> quotes. Maybe I'm misunderstanding how 'split("\\s+")' > >> works... > >>>> BTW, this options parsing stuff is exceedingly difficult to get > >>>> working right and working the same on all platforms. > >>>> > >>>> Dan > >>>> > >>>> > >>>> > >>>>> Thanks you so much for your help > >>>>> > >>>>> Cheers > >>>>> Markus
