On 28/11/2017 5:35 PM, Yasumasa Suenaga wrote:
http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.06/
The update looks good to me.
Serguei, do we need CSR?
After some thinking, I'd say - Not.
Thanks Serguei!
David, can I list you as a reviewer?
I guess that latest webrev is resolved your concern about
Agent_OnAttach and testcase fix in StartManagementAgent.java.
Yes - Reviewed.
Thanks,
David
Yasumasa
2017-11-28 16:20 GMT+09:00 serguei.spit...@oracle.com
<serguei.spit...@oracle.com>:
Hi Yasumasa,
On 11/27/17 23:15, Yasumasa Suenaga wrote:
Hi,
That said perhaps your simple fix to the test suffices here as the code
is
simply trying to skip throwing the expected exception. Perhaps include
the
whole of 'NumberFormatException: For input string: "apa"' just to be sure
it
is the expected NumberFormatException.
Thanks David!
I updated it in new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.06/
The update looks good to me.
Serguei, do we need CSR?
After some thinking, I'd say - Not.
Thanks,
Serguei
I don't know about CSR well, so I want you to help about it if we need it.
I think we need to merge this change to jdk repo ASAP because jdk10
repo will be opened soon.
I will change fixVersion to 11 if it is difficult.
Of course, I want to merge it to jdk10. :-)
Yasumasa
2017-11-28 15:21 GMT+09:00 David Holmes <david.hol...@oracle.com>:
Sorry Yasumasa I was away for a few days.
On 20/11/2017 5:54 PM, Yasumasa Suenaga wrote:
Hi David,
My own feeling is that it is up to the OnAttach function to properly
deal
with pending exceptions: report and/or clear them. The VM side just has
to
clear any pending exception to avoid it causing problems for later
code.
I removed the change to print pending exceptions in new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.04/
test/jdk/com/sun/tools/attach/StartManagementAgent.java
The reporting of NumberFormatException may be accurate in terms of the
low-level exception, but "Invalid com.sun.management.jmxremote.port
number"
was much clearer. This makes me wonder about whether the code that
previously produced "Invalid com.sun.management.jmxremote.port number"
needs
updating if this change proceeds. (And alao makes me wonder about the
impact
of the change in general.)
I tested StartManagementAgent.java without this change, and I got
failure
as below:
--------------
JavaTest Message: Test threw exception:
com.sun.tools.attach.AttachOperationFailedE
xception: java.lang.RuntimeException:
jdk.internal.agent.AgentConfigurationError: j
ava.lang.NumberFormatException: For input string: "apa"
JavaTest Message: shutting down test
STATUS:Failed.`main' threw exception:
com.sun.tools.attach.AttachOperationFailedExc
eption: java.lang.RuntimeException:
jdk.internal.agent.AgentConfigurationError: jav
a.lang.NumberFormatException: For input string: "apa"
--------------
Should we change this testcase whatever this change is not accepted?
Obviously the test was not updated when the exception information
changed.
The code that generates the AgentConfigurationError is here:
public static synchronized JMXConnectorServer
startRemoteConnectorServer(String portStr, Properties props) {
// Get port number
final int port;
try {
port = Integer.parseInt(portStr);
} catch (NumberFormatException x) {
throw new AgentConfigurationError(INVALID_JMXREMOTE_PORT, x,
portStr);
}
though I still can't see exactly how the printed exception information
would
come about. It makes me think that the code that sends the ACE back to
the
originating VM was updated inappropriately ... which may mean it was one
of
the earlier fixes in this area that broke the test.
That said perhaps your simple fix to the test suffices here as the code
is
simply trying to skip throwing the expected exception. Perhaps include
the
whole of 'NumberFormatException: For input string: "apa"' just to be sure
it
is the expected NumberFormatException.
Thanks,
David
Thanks,
Yasumasa
On 2017/11/20 6:41, David Holmes wrote:
Hi Yasumasa,
I've been trying to leave these reviews to serviceability folk ...
I've gone back through the original RFR from September last year to see
what we did and what was left.
The current proposal raises some concern for me - and IIRC Dmitry was
also concerned about it last time: printing of the pending exception.
If we
print the pending exception we will report an error and throw
AgentLoadException, even if execution of the OnAttach function returned
JNI_OK. If that exception was not critical to the success of the
loading the
agent, and the agent was just sloppy about clearing it, then it will
now
fail to load - which would be a compatibility concern.
Further, if the exception indicates an error and the OnAttach function
returns JNI_ERR then we won't report that cleanly because the printing
of
the exception will prevent matching with "return code: -1".
My own feeling is that it is up to the OnAttach function to properly
deal
with pending exceptions: report and/or clear them. The VM side just has
to
clear any pending exception to avoid it causing problems for later
code.
Some specific comments:
HotSpotVirtualMachine.java
The regex code seems overkill for the basic parsing you are doing. You
just need to see if the strings starts with "return code: " and then
parse
the next bit as an integer to get the return code.
---
test/jdk/com/sun/tools/attach/StartManagementAgent.java
The reporting of NumberFormatException may be accurate in terms of the
low-level exception, but "Invalid com.sun.management.jmxremote.port
number"
was much clearer. This makes me wonder about whether the code that
previously produced "Invalid com.sun.management.jmxremote.port number"
needs
updating if this change proceeds. (And alao makes me wonder about the
impact
of the change in general.)
---
Sorry - not the quick second review you were looking for.
David
-----
On 19/11/2017 11:38 PM, Yasumasa Suenaga wrote:
PING:
Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.03/
I want to merge this change to jdk 10. So I need a second reviewer.
Yasumasa
On 2017/11/16 21:09, Yasumasa Suenaga wrote:
Hi David, Serguei,
The test logic is adding it in AttachFailedTestBase.java:
45 return
Paths.get(System.getProperty("test.nativepath"),
"lib", libname)
46 .toAbsolutePath()
47 .toString();
Thanks!
I've fixed it in new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.03/
I've tested it as below. It works fine:
$ $JT_HOME/bin/jtreg -ignore:quiet -nativepath:$NATIVE_PATH
hotspot/jtreg/serviceability/dcmd/jvmti/AttachFailed
$ echo $NATIVE_PATH
/<Path to configuration>/images/test/hotspot/jtreg/native
Thanks,
Yasumasa
On 2017/11/16 16:49, serguei.spit...@oracle.com wrote:
On 11/15/17 23:29, David Holmes wrote:
On 16/11/2017 4:43 PM, serguei.spit...@oracle.com wrote:
On 11/15/17 18:11, David Holmes wrote:
Hi Serguei,
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so'
There should not be any "/lib/" in that path
Right, it should not be.
The test logic is adding it in AttachFailedTestBase.java:
45 return
Paths.get(System.getProperty("test.nativepath"),
"lib", libname)
46 .toAbsolutePath()
47 .toString();
but it shouldn't.
Nice catch!
I looked right to these lines and overlooked it. :)
Thanks,
Serguei
David
-----
This is the script I'm using to run the tests:
#!/bin/sh
REPO=/var/tmp/sspitsyn/jdk.attach
IMAGES=${REPO}/build/linux-x86_64-normal-server-release/images
export JAVA_HOME=${IMAGES}/jdk
export NATIVE_PATH=${IMAGES}/../support/test/hotspot/jtreg/native
export NATIVE_PATH=${IMAGES}/test/hotspot/jtreg/native
echo "JAVA_HOME = $JAVA_HOME"
/java/re/jtreg/4.2/nightly/binaries/jtreg/bin/jtreg
-nativepath:${NATIVE_PATH} \
$REPO/open/test/hotspot/jtreg/serviceability/dcmd/jvmti/AttachFailed
This is a part of log with the reported error from the
AttachException.jtr:
[TestNG] Running:
serviceability/dcmd/jvmti/AttachFailed/AttachException.java
Running DCMD 'JVMTI.agent_load
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so'
through 'PidJcmdExecutor'
Executing command
'[/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/jdk/bin/jcmd,
8689, JVMTI.agent_load
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so]'
Command returned with exit code 0
---------------- stdout ----------------
8689:
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native*/lib*/libException.so
was not loaded.
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native*/lib*/libException.so:
cannot open shared object file: No such file or directory
These are the locations of the libException.so:
build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/libException.so
build/linux-x86_64-normal-server-release/support/test/hotspot/jtreg/native/lib/libException.so
The tests fail with the
"NATIVE_PATH=${IMAGES}/test/hotspot/jtreg/native"
but pass with the "export
NATIVE_PATH=${IMAGES}/../support/test/hotspot/jtreg/native".
When the "export
NATIVE_PATH=${IMAGES}/../support/test/hotspot/jtreg/native" is
used
the log has this line:
Running DCMD 'JVMTI.agent_load
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/../support/test/hotspot/jtreg/native*/lib*/libException.so'
through 'JMXExecutor'
Apparently, the sub-directory name "/lib" is added to the path.
Thanks,
Serguei
David
On 16/11/2017 4:34 AM, serguei.spit...@oracle.com wrote:
Hi Yasumasa and David,
On 11/15/17 04:56, David Holmes wrote:
On 15/11/2017 10:15 PM, Yasumasa Suenaga wrote:
Hi Serguei,
Do the new tests pass in your runs?
Of course.
It seems not to exist jtreg native libraries.
I've tested as below:
$ make build-test-hotspot-jtreg-native
$ cd test
$ $JT_HOME/bin/jtreg -ignore:quiet
-nativepath:<builddir>/<confdir>/support/test/hotspot/jtreg/native/lib
hotspot/jtreg/serviceability/attach
hotspot/jtreg/serviceability/dcmd/jvmti
jdk/com/sun/tools/attach
Thanks.
I missed to add the -nativepath flag, sorry.
Please check that:
make test-image
followed by jtreg
-nativepath:<build-dir>/images/test/hotspot/jtreg/native
also works.
It fails with the error:
63 Running DCMD 'JVMTI.agent_load
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so'
through 'PidJcmdExecutor'
64 Executing command
'[/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/jdk/bin/jcmd,
28407, JVMTI.agent_load
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg
/native/lib/libException.so]'
65 Command returned with exit code 0
66 ---------------- stdout ----------------
67 28407:
68
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so
was not loaded.
69
/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/test/hotspot/jtreg/native/lib/libException.so:
cannot open shared object file: No such file or directory
70
It seems, the '/lib' folder is added to the nativepath.
Yasumasa, could you, double check it please?
I'm using the jtreg:
/java/re/jtreg/4.2/promoted/latest/binaries/jtreg/bin/jtreg
which is:
% ls -l /java/re/jtreg/4.2/promoted/latest
lrwxrwxrwx 1 uucp 143 7 Nov 6 21:49
/java/re/jtreg/4.2/promoted/latest -> fcs/b10/
Thanks,
Serguei
Thanks,
David
Good news is that the attach-related tests from closed
repository are passed.
Thanks!
Yasumasa
On 2017/11/15 16:38, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
Do the new tests pass in your runs?
In my runs 3 of 4 tests are failed with the errors like this:
109 Running DCMD 'JVMTI.agent_load
/var/tmp/sspitsyn/tst/jdk.attach/JTwork/scratch/null/lib/libException.so'
through 'PidJcmdExecutor'
110 Executing command
'[/var/tmp/sspitsyn/jdk.attach/build/linux-x86_64-normal-server-release/images/jdk/bin/jcmd,
21951, JVMTI.agent_load
/var/tmp/sspitsyn/tst/jdk.attach/JTwork/scratch/null/lib/libException.so]'
111 Command returned with exit code 0
112 ---------------- stdout ----------------
113 21951:
114
/var/tmp/sspitsyn/tst/jdk.attach/JTwork/scratch/null/lib/libException.so was
not loaded.
115
/var/tmp/sspitsyn/tst/jdk.attach/JTwork/scratch/null/lib/libException.so:
cannot open shared object file: No such file or directory
116
Good news is that the attach-related tests from closed
repository are passed.
Thanks,
Serguei
On 11/14/17 16:40, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
It looks good to me.
Thanks,
Serguei
On 11/7/17 22:38, Yasumasa Suenaga wrote:
Hi Serguei,
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8165736/webrev.02/
I'd expect a check for some exception name, not for
details like: For
input string: "apa".
Should we remove this comparison?
I don't understand. Why do remove?
Would it better to check for the exception name instead?
I've changed to check exception name
(NumberFormatException)
in
StartManagementAgent.java.
I will sponsor this fix and run these tests before the
push.
Thanks!
I'm waiting for second reviewer.
Yasumasa
2017-11-08 11:55 GMT+09:00 serguei.spit...@oracle.com
<serguei.spit...@oracle.com>:
On 11/6/17 04:31, Yasumasa Suenaga wrote:
Hi Serguei,
On 2017/11/06 20:06, serguei.spit...@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?
I don't understand. Why do remove?
Would it better to check for the exception name instead?
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?
Ok.
I will sponsor this fix and run these tests before the
push.
It seems, another update and one more review is needed.
Thanks,
Serguei
Thanks,
Yasumasa
Thanks,
Serguei
Thanks,
Yasumasa
On 2017/11/03 16:31, serguei.spit...@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
