Hi, > That said perhaps your simple fix to the test suffices here as the code is > simply trying to skip throwing the expected exception. Perhaps include the > whole of 'NumberFormatException: For input string: "apa"' just to be sure it > is the expected NumberFormatException.
Thanks David! I updated it in new webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.06/ Serguei, do we need CSR? I don't know about CSR well, so I want you to help about it if we need it. I think we need to merge this change to jdk repo ASAP because jdk10 repo will be opened soon. I will change fixVersion to 11 if it is difficult. Of course, I want to merge it to jdk10. :-) Yasumasa 2017-11-28 15:21 GMT+09:00 David Holmes <david.hol...@oracle.com>: > Sorry Yasumasa I was away for a few days. > > On 20/11/2017 5:54 PM, Yasumasa Suenaga wrote: >> >> Hi David, >> >> >>> My own feeling is that it is up to the OnAttach function to properly deal >>> with pending exceptions: report and/or clear them. The VM side just has to >>> clear any pending exception to avoid it causing problems for later code. >> >> >> I removed the change to print pending exceptions in new webrev: >> >> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.04/ >> >> >>> test/jdk/com/sun/tools/attach/StartManagementAgent.java >>> >>> The reporting of NumberFormatException may be accurate in terms of the >>> low-level exception, but "Invalid com.sun.management.jmxremote.port number" >>> was much clearer. This makes me wonder about whether the code that >>> previously produced "Invalid com.sun.management.jmxremote.port number" needs >>> updating if this change proceeds. (And alao makes me wonder about the impact >>> of the change in general.) >> >> >> I tested StartManagementAgent.java without this change, and I got failure >> as below: >> -------------- >> JavaTest Message: Test threw exception: >> com.sun.tools.attach.AttachOperationFailedE >> xception: java.lang.RuntimeException: >> jdk.internal.agent.AgentConfigurationError: j >> ava.lang.NumberFormatException: For input string: "apa" >> JavaTest Message: shutting down test >> >> STATUS:Failed.`main' threw exception: >> com.sun.tools.attach.AttachOperationFailedExc >> eption: java.lang.RuntimeException: >> jdk.internal.agent.AgentConfigurationError: jav >> a.lang.NumberFormatException: For input string: "apa" >> -------------- >> >> Should we change this testcase whatever this change is not accepted? > > > Obviously the test was not updated when the exception information changed. > The code that generates the AgentConfigurationError is here: > > public static synchronized JMXConnectorServer > startRemoteConnectorServer(String portStr, Properties props) { > > // Get port number > final int port; > try { > port = Integer.parseInt(portStr); > } catch (NumberFormatException x) { > throw new AgentConfigurationError(INVALID_JMXREMOTE_PORT, x, > portStr); > } > > though I still can't see exactly how the printed exception information would > come about. It makes me think that the code that sends the ACE back to the > originating VM was updated inappropriately ... which may mean it was one of > the earlier fixes in this area that broke the test. > > That said perhaps your simple fix to the test suffices here as the code is > simply trying to skip throwing the expected exception. Perhaps include the > whole of 'NumberFormatException: For input string: "apa"' just to be sure it > is the expected NumberFormatException. > > Thanks, > David > > >> >> Thanks, >> >> Yasumasa >> >> >> On 2017/11/20 6:41, David Holmes wrote: >>> >>> Hi Yasumasa, >>> >>> I've been trying to leave these reviews to serviceability folk ... >>> >>> I've gone back through the original RFR from September last year to see >>> what we did and what was left. >>> >>> The current proposal raises some concern for me - and IIRC Dmitry was >>> also concerned about it last time: printing of the pending exception. If we >>> print the pending exception we will report an error and throw >>> AgentLoadException, even if execution of the OnAttach function returned >>> JNI_OK. If that exception was not critical to the success of the loading the >>> agent, and the agent was just sloppy about clearing it, then it will now >>> fail to load - which would be a compatibility concern. >>> >>> Further, if the exception indicates an error and the OnAttach function >>> returns JNI_ERR then we won't report that cleanly because the printing of >>> the exception will prevent matching with "return code: -1". >>> >>> My own feeling is that it is up to the OnAttach function to properly deal >>> with pending exceptions: report and/or clear them. The VM side just has to >>> clear any pending exception to avoid it causing problems for later code. >>> >>> Some specific comments: >>> >>> HotSpotVirtualMachine.java >>> >>> The regex code seems overkill for the basic parsing you are doing. You >>> just need to see if the strings starts with "return code: " and then parse >>> the next bit as an integer to get the return code. >>> >>> --- >>> >>> test/jdk/com/sun/tools/attach/StartManagementAgent.java >>> >>> The reporting of NumberFormatException may be accurate in terms of the >>> low-level exception, but "Invalid com.sun.management.jmxremote.port number" >>> was much clearer. This makes me wonder about whether the code that >>> previously produced "Invalid com.sun.management.jmxremote.port number" needs >>> updating if this change proceeds. (And alao makes me wonder about the impact >>> of the change in general.) >>> >>> --- >>> >>> Sorry - not the quick second review you were looking for. >>> >>> David >>> ----- >>> >>> On 19/11/2017 11:38 PM, Yasumasa Suenaga wrote: >>>> >>>> PING: >>>> >>>> Could you review it? >>>> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.03/ >>>> >>>> >>>> I want to merge this change to jdk 10. So I need a second reviewer. >>>> >>>> >>>> Yasumasa >>>> >>>> >>>> >>>> On 2017/11/16 21:09, Yasumasa Suenaga wrote: >>>>> >>>>> Hi David, Serguei, >>>>> >>>>>>> The test logic is adding it in AttachFailedTestBase.java: >>>>>>> >>>>>>> 45 return Paths.get(System.getProperty("test.nativepath"), >>>>>>> "lib", libname) >>>>>>> 46 .toAbsolutePath() >>>>>>> 47 .toString(); >>>>> >>>>> >>>>> Thanks! >>>>> I've fixed it in new webrev: >>>>> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.03/ >>>>> >>>>> >>>>> I've tested it as below. It works fine: >>>>> >>>>> $ $JT_HOME/bin/jtreg -ignore:quiet -nativepath:$NATIVE_PATH >>>>> hotspot/jtreg/serviceability/dcmd/jvmti/AttachFailed >>>>> $ echo $NATIVE_PATH >>>>> /<Path to configuration>/images/test/hotspot/jtreg/native >>>>> >>>>> >>>>> Thanks, >>>>> >>>>> Yasumasa >>>>> >>>>> >>>>> On 2017/11/16 16:49, serguei.spit...@oracle.com wrote: >>>>>> >>>>>> On 11/15/17 23:29, David Holmes wrote: >>>>>>> >>>>>>> On 16/11/2017 4:43 PM, serguei.spit...@oracle.com wrote: >>>>>>>> >>>>>>>> On 11/15/17 18:11, David Holmes wrote: >>>>>>>>> >>>>>>>>> Hi Serguei, >>>>>>>>> >>>>>>>>> > >>>>>>>>> > /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so' >>>>>>>>> >>>>>>>>> There should not be any "/lib/" in that path >>>>>>>> >>>>>>>> >>>>>>>> Right, it should not be. >>>>>>> >>>>>>> >>>>>>> The test logic is adding it in AttachFailedTestBase.java: >>>>>>> >>>>>>> 45 return Paths.get(System.getProperty("test.nativepath"), >>>>>>> "lib", libname) >>>>>>> 46 .toAbsolutePath() >>>>>>> 47 .toString(); >>>>>>> >>>>>>> but it shouldn't. >>>>>> >>>>>> >>>>>> Nice catch! >>>>>> I looked right to these lines and overlooked it. :) >>>>>> >>>>>> Thanks, >>>>>> Serguei >>>>>> >>>>>>> >>>>>>> David >>>>>>> ----- >>>>>>> >>>>>>> >>>>>>>> This is the script I'm using to run the tests: >>>>>>>> >>>>>>>> #!/bin/sh >>>>>>>> >>>>>>>> REPO=/var/tmp/sspitsyn/jdk.attach >>>>>>>> IMAGES=${REPO}/build/linux-x86_64-normal-server-release/images >>>>>>>> export JAVA_HOME=${IMAGES}/jdk >>>>>>>> export NATIVE_PATH=${IMAGES}/../support/test/hotspot/jtreg/native >>>>>>>> export NATIVE_PATH=${IMAGES}/test/hotspot/jtreg/native >>>>>>>> echo "JAVA_HOME = $JAVA_HOME" >>>>>>>> >>>>>>>> /java/re/jtreg/4.2/nightly/binaries/jtreg/bin/jtreg >>>>>>>> -nativepath:${NATIVE_PATH} \ >>>>>>>> $REPO/open/test/hotspot/jtreg/serviceability/dcmd/jvmti/AttachFailed >>>>>>>> >>>>>>>> >>>>>>>> This is a part of log with the reported error from the >>>>>>>> AttachException.jtr: >>>>>>>> >>>>>>>> [TestNG] Running: >>>>>>>> serviceability/dcmd/jvmti/AttachFailed/AttachException.java >>>>>>>> >>>>>>>> Running DCMD 'JVMTI.agent_load >>>>>>>> /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so' >>>>>>>> through 'PidJcmdExecutor' >>>>>>>> Executing command >>>>>>>> '[/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/jdk/bin/jcmd, >>>>>>>> 8689, JVMTI.agent_load >>>>>>>> /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so]' >>>>>>>> Command returned with exit code 0 >>>>>>>> ---------------- stdout ---------------- >>>>>>>> 8689: >>>>>>>> >>>>>>>> /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native*/lib*/libException.so >>>>>>>> was not loaded. >>>>>>>> >>>>>>>> /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native*/lib*/libException.so: >>>>>>>> cannot open shared object file: No such file or directory >>>>>>>> >>>>>>>> >>>>>>>> These are the locations of the libException.so: >>>>>>>> >>>>>>>> build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/libException.so >>>>>>>> >>>>>>>> build/linux-x86_64-normal-server-release/support/test/hotspot/jtreg/native/lib/libException.so >>>>>>>> >>>>>>>> >>>>>>>> The tests fail with the >>>>>>>> "NATIVE_PATH=${IMAGES}/test/hotspot/jtreg/native" >>>>>>>> but pass with the "export >>>>>>>> NATIVE_PATH=${IMAGES}/../support/test/hotspot/jtreg/native". >>>>>>>> >>>>>>>> >>>>>>>> When the "export >>>>>>>> NATIVE_PATH=${IMAGES}/../support/test/hotspot/jtreg/native" is used >>>>>>>> the log has this line: >>>>>>>> >>>>>>>> Running DCMD 'JVMTI.agent_load >>>>>>>> /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/../support/test/hotspot/jtreg/native*/lib*/libException.so' >>>>>>>> through 'JMXExecutor' >>>>>>>> >>>>>>>> >>>>>>>> Apparently, the sub-directory name "/lib" is added to the path. >>>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Serguei >>>>>>>> >>>>>>>> >>>>>>>>> David >>>>>>>>> >>>>>>>>> On 16/11/2017 4:34 AM, serguei.spit...@oracle.com wrote: >>>>>>>>>> >>>>>>>>>> Hi Yasumasa and David, >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 11/15/17 04:56, David Holmes wrote: >>>>>>>>>>> >>>>>>>>>>> On 15/11/2017 10:15 PM, Yasumasa Suenaga wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Serguei, >>>>>>>>>>>> >>>>>>>>>>>>> Do the new tests pass in your runs? >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Of course. >>>>>>>>>>>> It seems not to exist jtreg native libraries. >>>>>>>>>>>> I've tested as below: >>>>>>>>>>>> >>>>>>>>>>>> $ make build-test-hotspot-jtreg-native >>>>>>>>>>>> $ cd test >>>>>>>>>>>> $ $JT_HOME/bin/jtreg -ignore:quiet >>>>>>>>>>>> -nativepath:<builddir>/<confdir>/support/test/hotspot/jtreg/native/lib >>>>>>>>>>>> hotspot/jtreg/serviceability/attach >>>>>>>>>>>> hotspot/jtreg/serviceability/dcmd/jvmti >>>>>>>>>>>> jdk/com/sun/tools/attach >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thanks. >>>>>>>>>> I missed to add the -nativepath flag, sorry. >>>>>>>>>> >>>>>>>>>>> Please check that: >>>>>>>>>>> >>>>>>>>>>> make test-image >>>>>>>>>>> >>>>>>>>>>> followed by jtreg >>>>>>>>>>> -nativepath:<build-dir>/images/test/hotspot/jtreg/native >>>>>>>>>>> >>>>>>>>>>> also works. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> It fails with the error: >>>>>>>>>> >>>>>>>>>> 63 Running DCMD 'JVMTI.agent_load >>>>>>>>>> /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so' >>>>>>>>>> through 'PidJcmdExecutor' >>>>>>>>>> 64 Executing command >>>>>>>>>> '[/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/jdk/bin/jcmd, >>>>>>>>>> 28407, JVMTI.agent_load >>>>>>>>>> /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg >>>>>>>>>> /native/lib/libException.so]' >>>>>>>>>> 65 Command returned with exit code 0 >>>>>>>>>> 66 ---------------- stdout ---------------- >>>>>>>>>> 67 28407: >>>>>>>>>> 68 >>>>>>>>>> /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so >>>>>>>>>> was not loaded. >>>>>>>>>> 69 >>>>>>>>>> /var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so: >>>>>>>>>> cannot open shared object file: No such file or directory >>>>>>>>>> 70 >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> It seems, the '/lib' folder is added to the nativepath. >>>>>>>>>> >>>>>>>>>> Yasumasa, could you, double check it please? >>>>>>>>>> >>>>>>>>>> I'm using the jtreg: >>>>>>>>>> /java/re/jtreg/4.2/promoted/latest/binaries/jtreg/bin/jtreg >>>>>>>>>> >>>>>>>>>> which is: >>>>>>>>>> >>>>>>>>>> % ls -l /java/re/jtreg/4.2/promoted/latest >>>>>>>>>> lrwxrwxrwx 1 uucp 143 7 Nov 6 21:49 >>>>>>>>>> /java/re/jtreg/4.2/promoted/latest -> fcs/b10/ >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Serguei >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> David >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> Good news is that the attach-related tests from closed >>>>>>>>>>>>> repository are passed. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Thanks! >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Yasumasa >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 2017/11/15 16:38, serguei.spit...@oracle.com wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Yasumasa, >>>>>>>>>>>>> >>>>>>>>>>>>> Do the new tests pass in your runs? >>>>>>>>>>>>> >>>>>>>>>>>>> In my runs 3 of 4 tests are failed with the errors like this: >>>>>>>>>>>>> >>>>>>>>>>>>> 109 Running DCMD 'JVMTI.agent_load >>>>>>>>>>>>> /var/tmp/sspitsyn/tst/jdk.attach/JTwork/scratch/null/lib/libException.so' >>>>>>>>>>>>> through 'PidJcmdExecutor' >>>>>>>>>>>>> 110 Executing command >>>>>>>>>>>>> '[/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/jdk/bin/jcmd, >>>>>>>>>>>>> 21951, JVMTI.agent_load >>>>>>>>>>>>> /var/tmp/sspitsyn/tst/jdk.attach/JTwork/scratch/null/lib/libException.so]' >>>>>>>>>>>>> 111 Command returned with exit code 0 >>>>>>>>>>>>> 112 ---------------- stdout ---------------- >>>>>>>>>>>>> 113 21951: >>>>>>>>>>>>> 114 >>>>>>>>>>>>> /var/tmp/sspitsyn/tst/jdk.attach/JTwork/scratch/null/lib/libException.so >>>>>>>>>>>>> was >>>>>>>>>>>>> not loaded. >>>>>>>>>>>>> 115 >>>>>>>>>>>>> /var/tmp/sspitsyn/tst/jdk.attach/JTwork/scratch/null/lib/libException.so: >>>>>>>>>>>>> cannot open shared object file: No such file or directory >>>>>>>>>>>>> 116 >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Good news is that the attach-related tests from closed >>>>>>>>>>>>> repository are passed. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> Serguei >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On 11/14/17 16:40, serguei.spit...@oracle.com wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Yasumasa, >>>>>>>>>>>>>> >>>>>>>>>>>>>> It looks good to me. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>> Serguei >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 11/7/17 22:38, Yasumasa Suenaga wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>> >
