PING: RFR: 8165736: Error message should be shown when JVMTI agent cannot be attached
David Holmes
david.holmes at oracle.com
Tue Nov 28 06:23:34 UTC 2017
Hi Yasumasa,
No further comments from me.
Thanks,
David
On 22/11/2017 12:48 AM, Yasumasa Suenaga wrote:
> Hi Serguei,
>
> On 2017/11/21 16:17, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>> Thank you for the update.
>>
>> Some comments.
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.04/src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java.frames.html
>>
>>
>> 94 BufferedReader reader = new BufferedReader(new InputStreamReader(in));
>> 95 try (reader) { A classic way for the above is: try (BufferedReader
>> reader = new BufferedReader(new InputStreamReader(in))) {
>
> I fixed it in new webrev:
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.05/
>
>
>> Also, I have a doubt about the change in HotSpotVirtualMachine.java:
>>
>> private void loadAgentLibrary(String agentLibrary, boolean
>> isAbsolute, String options)
>> throws AgentLoadException, AgentInitializationException,
>> IOException
>> {
>> + String msgPrefix = "return code: ";
>> InputStream in = execute("load",
>> agentLibrary,
>> isAbsolute ? "true" : "false",
>> options);
>> - try {
>> - int result = readInt(in);
>> - if (result != 0) {
>> - throw new AgentInitializationException("Agent_OnAttach failed",
>> result);
>> + BufferedReader reader = new BufferedReader(new InputStreamReader(in));
>> + try (reader) {
>> + String result = reader.readLine();
>> + if (result == null) {
>> + throw new AgentLoadException("Target VM did not respond");
>> + } else if (result.startsWith(msgPrefix)) {
>> + int retCode = Integer.parseInt(result.substring(msgPrefix.length()));
>> + if (retCode != 0) {
>> + throw new AgentInitializationException("Agent_OnAttach failed",
>> retCode);
>> + }
>> + } else {
>> + throw new AgentLoadException(result);
>> }
>> - } finally {
>> - in.close();
>> -
>> }
>> }
>>
>>
>> Now the AgentLoadException is thrown where it was not before. So that
>> there is a change in behavior.
>> On the other hand the AgentLoadException was already specified to be
>> thrown by the loadAgentLibrary.
>> I wonder, if this is worth to file a CSR for this.
>
> IMHO this change need not to CSR because the spec is not changed.
> (AgentLoadException is already added to `throws`.)
> I've not changed method signature in HotSpotVirtualMachine.java .
>
>
> Thanks,
>
> Yasumasa
>
>
>> Thanks,
>> Serguei
>>
>>
>> On 11/19/17 23:54, 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?
>>>
>>>
>>> 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.spitsyn at oracle.com wrote:
>>>>>>> On 11/15/17 23:29, David Holmes wrote:
>>>>>>>> On 16/11/2017 4:43 PM, serguei.spitsyn at 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.spitsyn at 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.spitsyn at 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.spitsyn at 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.spitsyn at oracle.com
>>>>>>>>>>>>>>>> <serguei.spitsyn at oracle.com>:
>>>>>>>>>>>>>>>>> On 11/6/17 04:31, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>> Hi Serguei,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 2017/11/06 20:06, serguei.spitsyn at 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.spitsyn at 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
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>
More information about the serviceability-dev
mailing list