Ping: RFR: JDK-8234808: jdb quoted option parsing broken

[Alex Menkov]

${subj}
Need 2nd reviewer

--alex

On 08/19/2020 16:46, serguei.spitsyn at oracle.com wrote:
> 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
>>>>>
>>>
>