RFR: JDK-8164913: JVMTI.agent_load dcmd should show useful error message

[David Holmes]

On 8/09/2016 1:47 PM, Yasumasa Suenaga wrote:
> Hi all,
>
> This changes has been approved. But it was not passed JPRT.
> It is caused by checking return value in
> HotSpotVirtualMachine#loadAgentLibrary().
>
> I've fixed it and uploaded webrev. This changes has been passed JPRT.
> (Thanks David!)
> Could you review again?
>
>   hotspot:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.02/hotspot/

Looks ok.

>       jdk: http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.02/jdk/

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java

63         try(BufferedReader reader = new BufferedReader(new 
InputStreamReader(
   64                                execute("load", agentLibrary,
   65                                      Boolean.toString(isAbsolute), 
options)))) {

This one is tricky to get indentation reasonable but at the moment it 
seems inconsistent maybe something like:

try (BufferedReader reader =
          new BufferedReader(
                             new InputStreamReader(
                                                   execute("load", 
agentLibrary,
 
Boolean.toString(isAbsolute), options)
))) {

I can't get the above to display correctly in my mail client but 
hopefully you get the idea. Also note space after 'try'.

67             if (result == null) {
68                 throw new AgentInitializationException(
69                                      "Agent_OnAttach failed: " + result);

You know result is null here.

74                     int retCode = Integer.parseInt(matcher.group(1));

This can throw NumberFormatException which is not declared for this 
method. The original code wraps this in an IOException.

Thanks,
David

>
> Thanks,
>
> Yasumasa
>
>
> On 2016/09/07 16:47, serguei.spitsyn at oracle.com wrote:
>> On 9/7/16 00:45, Dmitry Samersoff wrote:
>>> Serguei,
>>>
>>> I'm OK with the fix and OK to be listed as a reviewer.
>>
>> Thanks, Dmitry!
>> Serguei
>>>
>>> -Dmitry
>>>
>>> On 2016-09-07 07:08, serguei.spitsyn at oracle.com wrote:
>>>> Hi Dmitry,
>>>>
>>>> As a sponsor I'm going to push this fix if you are Ok with the fix.
>>>> Please, let me know if you still have any concerns.
>>>> Also, confirm if you are Ok to be in the list of reviewers.
>>>>
>>>>
>>>> On 9/5/16 06:46, Dmitry Samersoff wrote:
>>>>> Yasumasa,
>>>>>
>>>>> I'll look closely to the fix.
>>>>>
>>>>> Please, notice:
>>>>>
>>>>> 1. We typically avoid printing attach error messages on
>>>>> the target VM side.
>>>> Some message was already printed:
>>>>
>>>> 2410 // Agent_OnAttach executed so completion status is JNI_OK
>>>> 2411 st->print_cr("%d", result);
>>>> 2412 result = JNI_OK;
>>>>
>>>> It is just a replacement. :)
>>>>
>>>>
>>>>> 2. We are in RDP1 for jdk9.
>>>>>
>>>>>
>>>>> http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-August/004777.html
>>>>>
>>>> I've set the priority to P3, so it can be pushed to the jdk9/hs.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>> -Dmitry
>>>>>
>>>>> On 2016-09-05 16:25, Yasumasa Suenaga wrote:
>>>>>> PING: Could you review and sponsor it?
>>>>>>
>>>>>>>        http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2016/09/01 12:47, Yasumasa Suenaga wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I think RDP1 has been started.
>>>>>>> Cannot I fix this?
>>>>>>>
>>>>>>> This problem is that jcmd shows incorrect status when JVMTI agent
>>>>>>> cannot be attached.
>>>>>>> I think this problem should be fixed in 9 GA.
>>>>>>> The users who want.to <http://want.to> attach JVMTI agent want to
>>>>>>> know
>>>>>>> whether it succeed.
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> 2016/08/29 15:42 "Yasumasa Suenaga" <yasuenag at gmail.com
>>>>>>> <mailto:yasuenag at gmail.com>>:
>>>>>>>
>>>>>>>          This comment no longer matches the code and should be
>>>>>>> deleted:
>>>>>>>
>>>>>>>          2412       // Agent_OnAttach executed so completion
>>>>>>> status is
>>>>>>> JNI_OK
>>>>>>>          2413       st->print_cr("return code: %d", result);
>>>>>>>
>>>>>>>
>>>>>>>      Thanks David!
>>>>>>>      I removed it in new webrev.
>>>>>>>
>>>>>>>        http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/
>>>>>>> <http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.01/>
>>>>>>>
>>>>>>>
>>>>>>>      Yasumasa
>>>>>>>
>>>>>>>
>>>>>>>      On 2016/08/29 12:59, David Holmes wrote:
>>>>>>>
>>>>>>>          Hi Yasumasa,
>>>>>>>
>>>>>>>          On 28/08/2016 10:47 PM, 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
>>>>>>> output from
>>>>>>>              target VM.
>>>>>>>              So we should send error message from
>>>>>>> JVMTIAgentLoadDCmd.
>>>>>>>
>>>>>>>              I uploaded a webrev for it. Could you review it?
>>>>>>>
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.00/
>>>>>>> <http://cr.openjdk.java.net/~ysuenaga/JDK-8164913/webrev.00/>
>>>>>>>
>>>>>>>
>>>>>>>          This seems reasonable.
>>>>>>>
>>>>>>>          src/share/vm/prims/jvmtiExport.cpp
>>>>>>>
>>>>>>>          This comment no longer matches the code and should be
>>>>>>> deleted:
>>>>>>>
>>>>>>>          2412       // Agent_OnAttach executed so completion
>>>>>>> status is
>>>>>>> JNI_OK
>>>>>>>          2413       st->print_cr("return code: %d", result);
>>>>>>>
>>>>>>>          Thanks,
>>>>>>>          David
>>>>>>>
>>>>>>>              I cannot access JPRT.
>>>>>>>              So I need a sponsor.
>>>>>>>
>>>>>>>
>>>>>>>              Thanks,
>>>>>>>
>>>>>>>              Yasumasa
>>>>>>>
>>>
>>