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

[Yasumasa Suenaga]

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?


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


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