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

[Alex Menkov]

Still looking for 2nd reviewer

--alex

On 08/24/2020 13:50, Alex Menkov wrote:
> ${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
>>>>>>
>>>>
>>