RFR(XS): 7154809 JDI: update JDI/JDB debugee commandline option parsing (allow nested comma delimited options) + sponsor request

[Dmitry Samersoff]

Looks good for me!
-Dmitry


On 2012-04-05 15:43, 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/
> 
> 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: serviceability-dev at openjdk.java.net
>> Cc: hotspot-runtime-dev at openjdk.java.net
>> 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; serviceability-dev at openjdk.java.net; hotspot-
>>>> runtime-dev at openjdk.java.net
>>>> 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


-- 
Dmitry Samersoff
Java SE development team, SPB04
* There will come soft rains ...