PING: RFR: 8165736: Error message should be shown when JVMTI agent cannot be attached
Yasumasa Suenaga
yasuenag at gmail.com
Tue Nov 28 07:15:56 UTC 2017
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.holmes at 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.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