Yasumasa,
jvmtiExport.cpp:
I'm reluctant to change printing drastically at this stage of jdk9.
So it might be better to refactor the code to keep only one line in
output and print either
st->print_cr("return code: %d", result);
or
st->print_cr("%s was not loaded because of %s", ebuf);
Pending exception situation is less clean for me. We continue to load
agent ever if an exception happens. I would recommend to leave the code
as is and address this issue separately.
diagnosticCommand.cpp:
No comments
HotSpotVirtualMachine.java:
68 Agent_OnAttach failed: " + result
We know that result is null here.
71 We don't need to use regex here.
The only case where agent is loaded correctly returns exactly
"return code: 0"
so we can check for presence of this string to return OK and throw an
exception with a message read from agent in all other case.
-Dmitry
On 2016-09-08 14:28, Yasumasa Suenaga wrote:
> Hi David,
>
>> This one is tricky to get indentation reasonable but at the moment it
>> seems inconsistent maybe something like:
>
> Thanks!
> I will fix it after discussion on [1].
>
>
>> 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.
>
>
> My patch uses regex, and checks the match before getting the value:
> -----------------
> + Matcher matcher = Pattern.compile("^return code: (\\d+)$")
> + .matcher(result);
> + if (matcher.matches()) {
> + int retCode = Integer.parseInt(matcher.group(1));
> + if (retCode != 0) {
> + throw new AgentInitializationException(
> + "Agent_OnAttach failed",
> retCode);
> + }
> + }
> -----------------
>
> So I think matcher.group(1) will not return null.
>
>
> Thanks,
>
> Yasumasa
>
>
> [1]
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-September/020385.html
>
>
>
> On 2016/09/08 19:18, David Holmes wrote:
>> 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.spit...@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.spit...@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" <yasue...@gmail.com
>>>>>>>>> <mailto:yasue...@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
>>>>>>>>>
>>>>>
>>>>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
