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

[serguei.spitsyn at oracle.com]

Hi Yasumasa,

The changes looks good.
Thank you for making them!



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



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

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