>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.06/ > > > The update looks good to me. > >> Serguei, do we need CSR? > > > After some thinking, I'd say - Not.
Thanks Serguei! David, can I list you as a reviewer? I guess that latest webrev is resolved your concern about Agent_OnAttach and testcase fix in StartManagementAgent.java. Yasumasa 2017-11-28 16:20 GMT+09:00 serguei.spit...@oracle.com <serguei.spit...@oracle.com>: > Hi Yasumasa, > > > On 11/27/17 23:15, Yasumasa Suenaga wrote: >> >> 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/ > > > The update looks good to me. > >> Serguei, do we need CSR? > > > After some thinking, I'd say - Not. > > Thanks, > Serguei > > > >> 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 >>>>>>>>>>>>>>>>>>>>>>>> >
