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