Hi Serguei, I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.02/ >>> I'd expect a check for some exception name, not for details like: For >>> input string: "apa". >> >> >> Should we remove this comparison? > > > I don't understand. Why do remove? > Would it better to check for the exception name instead? I've changed to check exception name (NumberFormatException) in StartManagementAgent.java. > I will sponsor this fix and run these tests before the push. Thanks! I'm waiting for second reviewer. Yasumasa 2017-11-08 11:55 GMT+09:00 serguei.spit...@oracle.com <serguei.spit...@oracle.com>: > On 11/6/17 04:31, Yasumasa Suenaga wrote: >> >> Hi Serguei, >> >> On 2017/11/06 20:06, serguei.spit...@oracle.com wrote: >>> >>> Hi Yasumasa, >>> >>> The changes looks good. >>> Thank you for making them! >> >> >> Thanks! >> >> >>> On 11/3/17 05:10, Yasumasa Suenaga wrote: >>>> >>>> Hi Serguei, >>>> >>>> Thank you for your comment! >>>> I uploaded new webrev: >>>> >>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.01/ >>>> >>>> >>>>> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.00/test/jdk/com/sun/tools/attach/StartManagementAgent.java.udiff.html >>>>> >>>>> - if (!ex.getMessage().contains("Invalid >>>>> com.sun.management.jmxremote.port number")) { >>>>> + if (!ex.getMessage().contains("For input string: \"apa\"")) { >>>>> >>>>> >>>>> What is the motivation for this change? >>>>> It seems, the original comparison is better. >>>> >>>> >>>> "ex" is AttachOperationFailedException. >>>> We can get the result as below when we run StartManagementAgent: >>>> >>>> ------------- >>>> [runApplication] Error: Invalid com.sun.management.jmxremote.port >>>> number: apa >>>> [runApplication] jdk.internal.agent.AgentConfigurationError: >>>> java.lang.NumberFormatException: For input string: "apa" >>>> [runApplication] at >>>> jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.startRemoteConnectorServer(ConnectorBootstrap.java:336) >>>> ------------- >>>> >>>> I think we should check exception message in >>>> AttachOperationFailedException. >>>> In fact, jtreg fails at this point in my environment. >>> >>> >>> >>> I'd expect a check for some exception name, not for details like: For >>> input string: "apa". >> >> >> Should we remove this comparison? > > > I don't understand. Why do remove? > Would it better to check for the exception name instead? > > >>>>> What tests did you run to make sure there are no regressions? >>>> >>>> >>>> I've tested the following testcases: >>>> >>>> - hotspot/jtreg/serviceability/attach >>>> - hotspot/jtreg/serviceability/dcmd/jvmti >>>> - jdk/com/sun/tools/attach >>> >>> >>> There are more tests related to dynamic attach in closed, >>> nsk.aod.testlist and 30+ attach tests in nsk.jvmti.testlist. >>> I'm not sure, if they are included into any of the Mach5 testing levels. >>> Will need to check. >>> We have to make sure these tests are still passed. >> >> >> I cannot access JPRT and closed testcases because I'm not an Oracle >> employee. >> Can you run them with this change? > > > Ok. > I will sponsor this fix and run these tests before the push. > > It seems, another update and one more review is needed. > > Thanks, > Serguei > >> >> >> Thanks, >> >> Yasumasa >> >> >>> Thanks, >>> Serguei >>> >>>> Thanks, >>>> >>>> Yasumasa >>>> >>>> >>>> On 2017/11/03 16:31, serguei.spit...@oracle.com wrote: >>>>> >>>>> Hi Yasumasa, >>>>> >>>>> Some comments. >>>>> >>>>> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.00/test/jdk/com/sun/tools/attach/StartManagementAgent.java.udiff.html >>>>> >>>>> - if (!ex.getMessage().contains("Invalid >>>>> com.sun.management.jmxremote.port number")) { >>>>> + if (!ex.getMessage().contains("For input string: \"apa\"")) { >>>>> >>>>> >>>>> What is the motivation for this change? >>>>> It seems, the original comparison is better. >>>>> >>>>> >>>>> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.00/test/hotspot/jtreg/serviceability/dcmd/jvmti/AttachFailed/AttachException.java.html >>>>> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.00/test/hotspot/jtreg/serviceability/dcmd/jvmti/AttachFailed/AttachIncorrectLibrary.java.html >>>>> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.00/test/hotspot/jtreg/serviceability/dcmd/jvmti/AttachFailed/AttachNoEntry.java.html >>>>> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.00/test/hotspot/jtreg/serviceability/dcmd/jvmti/AttachFailed/AttachReturnError.java.html >>>>> >>>>> 37 public void run(CommandExecutor executor) { >>>>> 38 try{ >>>>> >>>>> A space is missed after 'try'. >>>>> >>>>> >>>>> It is odd that all test java classes define exactly the same >>>>> methods: sharedObjectName(), jmx() and cli(). >>>>> Would it better to defin a common base class with these methods? >>>>> >>>>> >>>>> Otherwise, it looks good. >>>>> Thank you for taking care about it! >>>>> >>>>> What tests did you run to make sure there are no regressions? >>>>> >>>>> Thanks, >>>>> Serguei >>>>> >>>>> >>>>> >>>>> On 11/1/17 05:59, Yasumasa Suenaga wrote: >>>>>> >>>>>> PING: Could you review and sponsor it? >>>>>> >>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.00/ >>>>>> >>>>>> >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Yasumasa >>>>>> >>>>>> >>>>>> On 2017/09/29 13:24, Yasumasa Suenaga wrote: >>>>>>> >>>>>>> Hi all, >>>>>>> >>>>>>> If we try to attach invalid JVMTI agent via JVMTI.agent_load dcmd, we >>>>>>> will get "Command executed successfully". However, it implies error >>>>>>> in >>>>>>> JVMTIAgentLoadDCmd. >>>>>>> This message is from JCmd.java when jcmd does not receive any output >>>>>>> from target VM. >>>>>>> >>>>>>> I think HotSopt/jcmd should return useful error message to users to >>>>>>> understand the cause of failure. >>>>>>> >>>>>>> I uploaded webrev for this issue. Could you review it? >>>>>>> >>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.00/ >>>>>>> >>>>>>> >>>>>>> This change is work fine on Fedora 26 x86_64 as following jtreg >>>>>>> testcases: >>>>>>> >>>>>>> - hotspot/jtreg/serviceability/attach >>>>>>> - hotspot/jtreg/serviceability/dcmd/jvmti >>>>>>> - jdk/com/sun/tools/attach >>>>>>> >>>>>>> I cannot access JPRT. So I need a sponsor. >>>>>>> (I cannot test it on other platforms.) >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Yasumasa >>>>>>> >>>>> >>> >
