[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Ping: RFR: JDK-8234808: jdb quoted option parsing broken
From:       Alex Menkov <alexey.menkov () oracle ! com>
Date:       2020-08-24 20:50:03
Message-ID: bd9a3cee-7a8c-a381-4d7c-8626c09a783f () oracle ! com
[Download RAW message or body]

${subj}
Need 2nd reviewer

--alex

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


[prev in list] [next in list] [prev in thread] [next in thread]