RFR: JDK-8234808: jdb quoted option parsing broken

[serguei.spitsyn at oracle.com]

Thank you for the update, Alex!
It looks good.

Thanks,
Serguei

On 8/19/20 16:35, Alex Menkov wrote:
> Updated webrev:
>
> http://cr.openjdk.java.net/~amenkov/jdk16/jdb_options/webrev.02/
>
> --alex
>
> On 08/19/2020 16:14, serguei.spitsyn at oracle.com wrote:
>> On 8/19/20 15:11, Alex Menkov wrote:
>>> Hi Serguei,
>>>
>>> thank you for the feedback.
>>>
>>> On 08/19/2020 13:58, serguei.spitsyn at oracle.com wrote:
>>>> Hi Alex,
>>>>
>>>> Sorry, I've overlooked this request for review.
>>>> The fix looks good in general.
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~amenkov/jdk16/jdb_options/webrev/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/VMConnection.java.frames.html 
>>>>
>>>>
>>>> 81 private Map <String, com.sun.jdi.connect.Connector.Argument> 
>>>> parseConnectorArgs(Connector connector,
>>>> 82 String argString,
>>>> 83 String extraOptions) {
>>>>
>>>> To make it more elegant I'd suggest to place the returned type on a 
>>>> separate line like below:
>>>> private Map<String, com.sun.jdi.connect.Connector.Argument>
>>>> parseConnectorArgs(Connector connector, String argString, String 
>>>> extraOptions) {
>>>
>>> Do you mean second line indent should be the same as 1st?
>>> or make it 8 spaces:
>>>
>>> private Map<String, com.sun.jdi.connect.Connector.Argument>
>>>         parseConnectorArgs(Connector connector, String argString, 
>>> String extraOptions) {
>>
>> No indent is needed, I think.
>> My suggestion is to use extra line for method return type instead of 
>> method arguments.
>>
>>
>>>>
>>>> 127 sb.append(extraOptions).append(" ");
>>>> 128 // set extraOptions to null to not set it again
>>>> 129 extraOptions = null;
>>>>
>>>> What about rewording the comment like below? :
>>>>       // set extraOptions to null to avoid appending it again
>>>
>>> ok.
>>>
>>>>
>>>> 165 if (extraOptions != null) {
>>>> 166 // there was no "option" specified in argString
>>>> 167 Connector.Argument argument = arguments.get("options");
>>>> 168 if (argument != null) {
>>>> 169 argument.setValue(extraOptions);
>>>> 170 }
>>>> 171 }
>>>>
>>>> Should the "option" in the comment be replaced with "options"?
>>>
>>> right.
>>>
>>>> What if the argument at line 167 was set to null?
>>>> Will the extraOptions be ignored in such a case?
>>>
>>> extraOptions makes sense only for CommandLineLaunch connector which 
>>> launches new VM (and only this connector has "options" argument).
>>> Other connectors (attach or listen) connect to existing VM and 
>>> cannot set its options.
>>
>> Okay, thank you for explanation.
>>
>> Thanks,
>> Serguei
>>
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~amenkov/jdk16/jdb_options/webrev/test/jdk/com/sun/jdi/JdbOptions.java.html 
>>>>
>>>>
>>>> This line is probably not needed anymore:
>>>>
>>>>   157             //jdb.quit();
>>>>
>>>
>>> will delete.
>>>
>>> --alex
>>>
>>>>
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>> On 8/7/20 15:09, Alex Menkov wrote:
>>>>> Hi all,
>>>>>
>>>>> please review the fix for
>>>>> https://bugs.openjdk.java.net/browse/JDK-8234808
>>>>> webrev:
>>>>> http://cr.openjdk.java.net/~amenkov/jdk16/jdb_options/webrev/
>>>>>
>>>>> Some background:
>>>>> when jdb launches debuggee process it passes java options from 
>>>>> "options" value for CommandLineLaunch connector and forward 
>>>>> options specified before command.
>>>>>
>>>>> The fix solves several discovered issues:
>>>>> - proper handling of java options with spaces
>>>>> - if both way are used to specify java options, forwarded options 
>>>>> override options from "options" value
>>>>>
>>>>> VMConnection class implements tricky logic for "options" field 
>>>>> parsing for JFR needs (handling of single and double quotes). I 
>>>>> decided to keep it as is to avoid massive test failures with JFR 
>>>>> (there is no test coverage for this functionality and I'm not sure 
>>>>> I understand all requirements).
>>>>>
>>>>> --alex
>>>>
>>