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

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