PING: RFR: 8165736: Error message should be shown when JVMTI agent cannot be attached

Wed Nov 15 00:40:05 UTC 2017

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
>>>>>>>>