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

serguei.spit...@oracle.com Thu, 08 Sep 2016 10:37:59 -0700

Hi Yasumasa,

On 9/8/16 06:39, Yasumasa Suenaga wrote:

Hi Dmitry,


for jvmtiExport.cpp , can we change as below?
----------------------

--- a/src/share/vm/prims/jvmtiExport.cpp Wed Sep 07 23:17:242016 +0200+++ b/src/share/vm/prims/jvmtiExport.cpp Thu Sep 08 22:34:012016 +0900

@@ -2407,9 +2407,7 @@
         delete agent_lib;
       }

-      // Agent_OnAttach executed so completion status is JNI_OK
       st->print_cr("%d", result);
-      result = JNI_OK;
     }
   }
   return result;
----------------------

At least, we can get error code if agent attach is failed.
If it is acceptable, I remove the change for HotSpotVirtualMacine.java .


This would be nice.
Could you, please, send a full webrev for this last suggestion?

We still cannot know the reason of attach failure. If jvmtiExport.cppwill be changed in above, I will file it as other issue on JBS.


Agreed, it is better to separate this issue.


Thanks,
Serguei



Thanks,

Yasumasa


On 2016/09/08 21:37, Dmitry Samersoff wrote:

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

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 JVMTIagent

cannot be attached.
I think this problem should be fixed in 9 GA.

The users who want.to <http://want.to> attach JVMTI agentwant 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, itimplies

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

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

Reply via email to