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

[serguei.spit...@oracle.com]

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

 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

 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"?
What if the argument at line 167 was set to null?
Will the extraOptions be ignored in such a case?


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();

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