RFR: JDK-8234808: jdb quoted option parsing broken

[Alex Menkov]

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