<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">HI Chris,<div class=""><br class=""></div><div class="">overall looks good to me, a few comments though:</div><div class="">1. since you removed vm.hasSAandCanAttach from VMProps, you also need to remove it from all TEST.ROOT files which mention it (test/jdk/TEST.ROOT and test/hotspot/jtreg/TEST.ROOT) so people won't be confused by undefined property and jtreg will be able to properly report invalid usages of it if any.</div><div class=""><br class=""></div><div class="">2. in SATestUtils::canAddPrivileges, could you please add some meaningful message to the RuntimeException at L#102?</div><div class=""><br class=""></div><div class="">3. SATestUtils::checkAttachOk method name is somewhat misleading (hence you had to put comment every time you used it), I'd recommend you to rename to smth like skipIfCannotAttach().</div><div class=""><br class=""></div><div class="">4. in SATestUtils::checkAttachOk's javadoc, it would be better to use @throws tag like:</div><div class=""><blockquote type="cite" class="">+ /**<br class="">+ * Checks if SA Attach is expected to work.<br class=""></blockquote><blockquote type="cite" class="">+. * @throws SkippedException ifSA Attach is not expected to work.</blockquote><blockquote type="cite" class="">+ */</blockquote></div><div class=""><br class=""></div><div class="">5. it also might make sense to catch IOException within SATestUtils::checkAttachOk and throw it as Error or RuntimeException.</div><div class=""><br class=""></div><div class="">I've briefly looked at all the changed tests and they look good.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">-- Igor </div><div class=""><br class=""></div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Mar 12, 2020, at 11:06 PM, Chris Plummer <<a href="mailto:chris.plummer@oracle.com" class="">chris.plummer@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" class="">
<div text="#000000" bgcolor="#FFFFFF" class="">
<div class="moz-cite-prefix">Hi Serguei,<br class="">
<br class="">
Thanks for the review!<br class="">
<br class="">
Can I get one more reviewer please?<br class="">
<br class="">
thanks,<br class="">
<br class="">
Chris<br class="">
<br class="">
On 3/12/20 12:06 AM, <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:33ffd0e7-d017-c1bf-feb5-4d2f07e753f2@oracle.com" class="">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" class="">
<div class="moz-cite-prefix">Hi Chris,<br class="">
<br class="">
<br class="">
On 3/12/20 00:03, Chris Plummer wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:227652ff-ffb0-a1f0-531d-03b75ecec921@oracle.com" class="">
<div class="moz-cite-prefix">Hi Serguei,<br class="">
<br class="">
That check used to be in Platform.shouldSAAttach(), which
essentially was moved to SATestUtils.checkAttachOk() and
reworked some. It was necessary in Platform.shouldSAAttach()
since that was used to evaluation vm.hasSAandCanAttach (which
is now gone). When I moved everything to
SATestUtils.checkAttachOk(), I recall thinking it wasn't
really necessary since all tests that call it should have
@require vm.hasSA, but left it in anyway just to be extra
safe. I'm still inclined to just leave it in, but would not be
opposed to removing it.<br class="">
</div>
</blockquote>
<br class="">
I agree, it is more safe to keep it, at list for now.<br class="">
<br class="">
<br class="">
Thanks,<br class="">
Serguei<br class="">
<br class="">
<blockquote type="cite" cite="mid:227652ff-ffb0-a1f0-531d-03b75ecec921@oracle.com" class="">
<div class="moz-cite-prefix"> thanks,<br class="">
<br class="">
Chris<br class="">
<br class="">
On 3/11/20 11:20 PM, <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com" moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:150f87f8-5b54-deae-ced1-1227bb1ed17a@oracle.com" class="">
<div class="moz-cite-prefix">Hi Chris,<br class="">
<br class="">
I've made another pass today.<br class="">
It looks good to me.<br class="">
<br class="">
I have just one minor questions.<br class="">
<br class="">
There is some overlap between the <span class="new">requires
vm.hasSA check and checkAttachOk:</span><br class="">
<pre class=""><span class="new">+ public static void checkAttachOk() throws IOException {</span>
<span class="new">+ if (!Platform.hasSA()) {</span>
<span class="new">+ throw new SkippedException("SA not supported.");</span>
<span class="new">+ }</span></pre>
<span class="new"></span>In the former case, the test is not
run but in the latter the SkippedException is thrown.<br class="">
As I see, all tests with the <span class="new">checkAttachOk
call use </span><span class="new">requires vm.hasSA as
well.<br class="">
It can be that the first check</span><span class="new">
"if (!Platform.hasSA())"</span><span class="new"></span> <span class="new">in the checkAttachOk is redundant.<br class="">
</span><span class="new"><span class="new">It is okay and
more safe in general but generates little confusion.<br class="">
I'm okay if you don't do anything with this but wanted
to know your view.</span></span><br class="">
<br class="">
Thanks,<br class="">
Serguei<br class="">
<br class="">
<br class="">
On 3/10/20 18:57, Chris Plummer wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:c26a5f69-3127-bd0a-0e25-8b7afe4464aa@oracle.com" class="">
<div class="moz-cite-prefix">On 3/10/20 6:07 PM, <a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com" moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:1b344718-9485-eb5c-1b85-9e358851c369@oracle.com" class="">
<div class="moz-cite-prefix">Hi Chris,<br class="">
<br class="">
Overall, this looks as a right direction to me while it
is not easy to verify all the details.<br class="">
</div>
</blockquote>
Yes, there are a lot of tests with quite a few different
types of changes. I did a lot of testing and verified that
when the tests pass, they pass for the right reasons (really
ran the test, skipped due to lack of privileges, or skipped
due to running signed on OSX 10.14 or later). I also
verified locally running as root, running with a cached
sudo, and running without sudo.<br class="">
<blockquote type="cite" cite="mid:1b344718-9485-eb5c-1b85-9e358851c369@oracle.com" class="">
<div class="moz-cite-prefix"> I'll make another pass
tomorrow. <br class="">
</div>
</blockquote>
Thanks!<br class="">
<blockquote type="cite" cite="mid:1b344718-9485-eb5c-1b85-9e358851c369@oracle.com" class="">
<div class="moz-cite-prefix"> <br class="">
A couple of quick nits so far:<br class="">
<br class="">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java.udiff.html" moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java.udiff.html</a><br class="">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java.udiff.html" moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java.udiff.html</a><br class="">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForInvokeDynamic.java.udiff.html" moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForInvokeDynamic.java.udiff.html</a><br class="">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java.udiff.html" moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java.udiff.html</a><br class="">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackMixed.java.udiff.html" moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackMixed.java.udiff.html</a><br class="">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestRevPtrsForInvokeDynamic.java.udiff.html" moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestRevPtrsForInvokeDynamic.java.udiff.html</a><br class="">
<pre class=""> import jdk.test.lib.Utils;
<span class="removed">-import jdk.test.lib.Asserts;</span>
<span class="new">+import jdk.test.lib.SA.SATestUtils;</span><span class="changed"></span></pre>
Need to swap these exports.<br class="">
<br class="">
<br class="">
</div>
</blockquote>
Ok<br class="">
<blockquote type="cite" cite="mid:1b344718-9485-eb5c-1b85-9e358851c369@oracle.com" class="">
<div class="moz-cite-prefix"> <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/lib/jdk/test/lib/SA/SATestUtils.java.frames.html" moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/lib/jdk/test/lib/SA/SATestUtils.java.frames.html</a>
<pre class=""><span class="new"> 48 if (SATestUtils.needsPrivileges()) {</span>
<span class="new"> 49 cmdStringList = SATestUtils.addPrivileges(cmdStringList);</span>
</pre>
The method calls are local, so the class name can be
omitted in the method names:<br class="">
<span class="new">SATestUtils.needsPrivileges and </span><span class="new">SATestUtils.addPrivileges.</span></div>
</blockquote>
Ok<br class="">
<blockquote type="cite" cite="mid:1b344718-9485-eb5c-1b85-9e358851c369@oracle.com" class="">
<div class="moz-cite-prefix"> <br class="">
<br class="">
<pre class=""><span class="new"> 94 try {</span>
95 if (echoProcess.waitFor(60, TimeUnit.SECONDS) == false) {
<span class="changed"> 96 // Due to using the "-n" option, sudo should complete almost immediately. 60 seconds</span>
<span class="changed"> 97 // is more than generous. If it didn't complete in that time, something went very wrong.</span>
98 echoProcess.destroyForcibly();
<span class="changed"> 99 throw new RuntimeException("Timed out waiting for sudo to execute.");</span>
<span class="changed"> 100 }</span>
<span class="changed"> 101 } catch (InterruptedException e) {</span>
<span class="changed"> 102 throw new RuntimeException(e);</span>
103 }
</pre>
The lines 101/103 are misaligned.<br class="">
</div>
</blockquote>
Ok.<br class="">
<blockquote type="cite" cite="mid:1b344718-9485-eb5c-1b85-9e358851c369@oracle.com" class="">
<div class="moz-cite-prefix"> <br class="">
<br class="">
Thanks,<br class="">
Serguei<br class="">
<br class="">
</div>
</blockquote>
Thanks,<br class="">
<br class="">
Chris<br class="">
<blockquote type="cite" cite="mid:1b344718-9485-eb5c-1b85-9e358851c369@oracle.com" class="">
<div class="moz-cite-prefix"> <br class="">
<br class="">
On 3/9/20 19:29, Chris Plummer wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:769c20ac-52e1-bbdf-dadb-370a92f9615a@oracle.com" class="">Hi,
<br class="">
<br class="">
Please help review the following: <br class="">
<br class="">
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8238268" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8238268</a>
<br class="">
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/" moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/</a>
<br class="">
<br class="">
I'll try to give enough background first to make it
easier to understand the changes. On OSX you must run SA
tests that attach to a live process as root or using
sudo. For example: <br class="">
<br class="">
sudo make run-test
TEST=serviceability/sa/ClhsdbJstackXcompStress.java <br class="">
<br class="">
Whether running as root or under sudo, the check to
allow the test to run is done with: <br class="">
<br class="">
private static boolean canAttachOSX() { <br class="">
return userName.equals("root"); <br class="">
} <br class="">
<br class="">
Any test using "@requires vm.hasSAandCanAttach" must
pass this check via Platform.shouldSAAttach(), which for
OSX returns: <br class="">
<br class="">
return canAttachOSX() &&
!isSignedOSX(); <br class="">
<br class="">
So if running as root the "@requires
vm.hasSAandCanAttach" passes, otherwise it does not.
However, using a root login to run tests is not a very
desirable, nor is issuing a "sudo make run-test" (any
created file ends up with root ownership). Because of
this support was previously added for just running the
attaching process using sudo, not the entire test. This
was only done for the 20 or so tests that use
ClhsdbLauncher. These tests use "@requires vm.hasSA",
and then while running the test will do a "sudo" check
if canAttachOSX() returns false: <br class="">
<br class="">
if (!Platform.shouldSAAttach()) { <br class="">
if (Platform.isOSX()) { <br class="">
if (Platform.isSignedOSX()) { <br class="">
throw new SkippedException("SA
attach not expected to work. JDK is signed."); <br class="">
} else if
(SATestUtils.canAddPrivileges()) { <br class="">
needPrivileges = true; <br class="">
} <br class="">
} <br class="">
if (!needPrivileges) { <br class="">
// Skip the test if we don't have enough
permissions to attach <br class="">
// and cannot add privileges. <br class="">
throw new SkippedException( <br class="">
"SA attach not expected to work.
Insufficient privileges."); <br class="">
} <br class="">
} <br class="">
<br class="">
So basically it does a runtime check of
vm.hasSAandCanAttach, and if it fails then checks if
running with sudo will work. This allows for either a
passwordless sudo to be used when running clhsdb, or for
the user to be prompted for the sudo password (note I've
remove support for the latter with my changes). <br class="">
<br class="">
That brings us to the CR that is being fixed.
ClhsdbLauncher tests support sudo and will therefore run
with our CI testing on OSX, but the 25 or so tests that
use "@requires vm.hasSAandCanAttach" do not, and
therefore are never run with our CI OSX testing. The
changes in this webrev fix that. <br class="">
<br class="">
There are two possible approaches to the fix. One is
having the check for sudo be done as part of the
vm.hasSAandCanAttach evaluation. The other approach is
to do the check in the test at runtime similar to how
ClhsdbLauncher currently does. This would mean just
using "@requires vm.hasSA" for all the tests instead of
"@requires vm.hasSAandCanAttach". I chose the later
because there is an advantage to throwing
SkippedException rather than just silently skipping the
test using @requires. The advantage is that mdash tells
you how many tests were skipped, and when you hover over
the reason you can see the SkippedException message,
which will differentiate between reasons like the JDK
was signed or there are insufficient privileges. If all
the checking was done by the vm.hasSAandCanAttach
evaluation, you would not know why the test wasn't run.
<br class="">
<br class="">
The "support" related changes made are all in the
following 3 files. The rest of the changes are in the
tests: <br class="">
<br class="">
test/jtreg-ext/requires/VMProps.java <br class="">
test/lib/jdk/test/lib/Platform.java <br class="">
test/lib/jdk/test/lib/SA/SATestUtils.java <br class="">
<br class="">
You'll noticed that one change I made to the sudo
support in SATestUtils.canAddPrivileges() is to make
sudo non-interactive, which means no password prompt. So
that means either the user does not require a password,
or the credentials have been cached. Otherwise the sudo
check will fail. On most platforms if you execute a sudo
command, the credentials are cached for 5 minutes. So if
your user is not setup for passwordless sudo, then a
sudo command can be issued before running the tests, and
will likely remain cached until the test is run. The
reason for using passwordless is because prompting in
the middle of running tests can be confusing (you
usually walk way once launching the tests and miss the
prompt anyway), and avoids unnecessary delays in
automated testing due to waiting for the password prompt
to timeout (it used to wait 1 minute). <br class="">
<br class="">
There are essentially 3 types of tests that SA Attach to
a process, each needing a slightly different fix: <br class="">
<br class="">
1. Tests that directly launch a jdk.hotspot.agent class,
such as TestClassDump.java. They need to call
SATestUtils.checkAttachOk() to verify that attaching
will be possible, and then
SATestUtils.addPrivilegesIfNeeded(pb) to get the sudo
command added if needed.They also need to switch from
using hasSAandCanAttach to using hasSA. <br class="">
<br class="">
2. Tests that launch command line tools such has jhsdb.
They need to call SATestUtils.checkAttachOk() to verify
that attaching will be possible, and then
SATestUtils.createProcessBuilder() to create a process
that will be launched using sudo if necessary.They also
need to switch from using hasSAandCanAttach to using
hasSA. <br class="">
<br class="">
3. Tests that use ClhsdbLauncher. They already use hasSA
instead of hasSAandCanAttach, and rely on ClhsdbLauncher
to do check at runtime if attaching will work, so for
the most part all the these tests are unchanged.
ClhsdbLauncher was modified to take advantage of the new
SATestUtils.createProcessBuilder() and
SATestUtils.checkAttachOk() APIs. <br class="">
<br class="">
Some tests required special handling: <br class="">
<br class="">
test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java <br class="">
test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java <br class="">
<br class="">
- These two tests SA Attach to a core file, not to a
process, so only need hasSA, <br class="">
not hasSAandCanAttach. No other changes were needed. <br class="">
<br class="">
test/hotspot/jtreg/serviceability/sa/ClhsdbFindPC.java <br class="">
<br class="">
- The output should never be null. If the test was
skipped due to lack of privileges, you <br class="">
would never get to this section of the test. <br class="">
<br class="">
test/hotspot/jtreg/serviceability/sa/TestClhsdbJstackLock.java <br class="">
test/hotspot/jtreg/serviceability/sa/TestIntConstant.java <br class="">
test/hotspot/jtreg/serviceability/sa/TestPrintMdo.java <br class="">
test/hotspot/jtreg/serviceability/sa/TestType.java <br class="">
test/hotspot/jtreg/serviceability/sa/TestUniverse.java <br class="">
<br class="">
- These are ClhsdbLauncher tests, so they should have
been using hasSA instead of <br class="">
hasSAandCanAttachin the first place. No other changes
were needed. <br class="">
<br class="">
test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java <br class="">
test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java <br class="">
test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java <br class="">
<br class="">
- These tests used to "@require mac" but seem run fine
on OSX, so I removed this requirement. <br class="">
<br class="">
test/jdk/sun/tools/jhsdb/BasicLauncherTest.java <br class="">
<br class="">
- This test had a runtime check to not run on OSX due to
not having core file stack <br class="">
walking support. However, this tests always attaches
to a process, not a core file, <br class="">
and seems to run just fine on OSX. <br class="">
<br class="">
test/jdk/sun/tools/jstack/DeadlockDetectionTest.java <br class="">
<br class="">
- I changed the test to throw a SkippedException if it
gets the unexpected error code <br class="">
rather than just println. <br class="">
<br class="">
And a few other miscellaneous changes not already
covered: <br class="">
<br class="">
test/lib/jdk/test/lib/Platform.java <br class="">
- Made canPtraceAttachLinux() public so it can be called
from SATestUtils. <br class="">
- vm.hasSAandCanAttach is now gone. <br class="">
<br class="">
thanks, <br class="">
<br class="">
Chris <br class="">
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</div>
</div></blockquote></div><br class=""></div></body></html>