PING: Could you reivew it? We need one more reviewer. > http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.02/
Thanks, Yasumasa 2017-11-08 15:38 GMT+09:00 Yasumasa Suenaga <yasue...@gmail.com>: > 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 >>>>>>>> >>>>>> >>>> >>
