RFR: JDK-8234808: jdb quoted option parsing broken
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Aug 19 23:14:59 UTC 2020
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
>>
More information about the serviceability-dev
mailing list