[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(L) 8238268: Many SA tests are not running on OSX because they do not attempt to use sudo whe
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2020-03-17 1:16:29
Message-ID: 7f615584-81e2-e49a-0bfb-563adc1f5834 () oracle ! com
[Download RAW message or body]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">No it doesn't. I'll change that.<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 3/16/20 5:14 PM, Igor Ignatyev wrote:<br>
</div>
<blockquote type="cite"
cite="mid:556F1CD7-46ED-46D9-BAB9-DD099111D981@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
Hi Chris,
<div class=""><br class="">
</div>
<div class="">does canPtraceAttachLinux have to be public?</div>
<div class=""><br class="">
</div>
<div class="">otherwise, looks good to me.</div>
<div class=""><br class="">
</div>
<div class="">-- Igor<br class="">
<div><br class="">
<blockquote type="cite" class="">
<div class="">On Mar 16, 2020, at 5:11 PM, Chris Plummer
<<a href="mailto:chris.plummer@oracle.com" class=""
moz-do-not-send="true">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 class="">
<div class="moz-cite-prefix">Hi Serguei and Igor,<br
class="">
<br class="">
New webrev:<br class="">
<br class="">
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.02/index.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.02/index.html</a><br
class="">
<br class="">
Only files changed were Platform.java and
SATestUtils.java.<br class="">
<br class="">
-Moved canPtraceAttachLinux() from Platform.java to
SATestUtils.java<br class="">
-Changed Platform.canPtraceAttachLinux() reference in
SATestUtils.java to just be canPtraceAttachLinux().<br
class="">
-Had to change userName.equals("root") reference in
canPtraceAttachLinux() to Platform.isRoot(). Probably
should have been that way in the first place.<br
class="">
-Made some adjustments to the imports<br class="">
<br class="">
thanks,<br class="">
<br class="">
Chris<br class="">
<br class="">
On 3/16/20 12:13 PM, Igor Ignatev wrote:<br class="">
</div>
<blockquote type="cite"
cite="mid:247B7EA6-7BB5-4F6C-84C4-C110BAF8F063@oracle.com"
class="">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8" class="">
<br class="">
<div dir="ltr" class=""><br class="">
<blockquote type="cite" class="">On Mar 16, 2020, at
11:43 AM, <a class="moz-txt-link-rfc2396E"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">"serguei.spitsyn@oracle.com"</a>
<a class="moz-txt-link-rfc2396E"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true"><serguei.spitsyn@oracle.com></a>
wrote:<br class="">
<br class="">
</blockquote>
</div>
<blockquote type="cite" class="">
<div dir="ltr" class="">
<meta http-equiv="Content-Type"
content="text/html; charset=UTF-8" class="">
<div class="moz-cite-prefix">On 3/16/20 11:26,
Chris Plummer wrote:<br class="">
</div>
<blockquote type="cite"
cite="mid:af2d05b8-01b2-61e3-a729-c29d2479599c@oracle.com"
class="">
<div class="moz-cite-prefix">I had to make
another change. <a role="listbox"
class="dropdown basic ui compact"
tabindex="0" moz-do-not-send="true"><span
class="">TestMutuallyExclusivePlatformPredicates.java
failed when I ran tier 3. I had fixed it a
long while back due to Platform.</span>shouldSAAttach()
being removed, but there were more changes
to Platform.java after that that I didn't
account for. isRoot() was added and
canPtraceAttrachLinux() was made public. So
this is what the diff looks like now:</a><br
class="">
<br class="">
---
a/test/hotspot/jtreg/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java<br
class="">
+++
b/test/hotspot/jtreg/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java<br
class="">
@@ -51,9 +51,9 @@<br class="">
VM_TYPE("isClient", "isServer",
"isMinimal", "isZero", "isEmbedded"),<br
class="">
MODE("isInt", "isMixed", "isComp"),<br
class="">
IGNORED("isEmulatedClient",
"isDebugBuild", "isFastDebugBuild",<br
class="">
- "isSlowDebugBuild", \
"hasSA", "shouldSAAttach", "isTieredSupported",<br
class="">
+ "isSlowDebugBuild", \
"hasSA", "canPtraceAttachLinux", "isTieredSupported",<br
class="">
"areCustomLoadersSupportedForCDS",
"isDefaultCDSArchiveSupported",<br class="">
- "isSignedOSX");<br \
class="">
+ "isSignedOSX", \
"isRoot");<br class="">
<br class="">
However, I'm thinking maybe I should just move
canPtraceAttachLinux() to SATestUtils.java
since that's the only user, and it is an SA
specific API. What do you think?<br class="">
</div>
</blockquote>
<br class="">
The approach to localize canPtraceAttachLinux() in
SATestUtils.java sounds right to me if it is an SA
specific API.<br class="">
<br class="">
</div>
</blockquote>
+1
<div class="">— Igor</div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div dir="ltr" class=""> Thanks,<br class="">
Serguei<br class="">
<br class="">
<blockquote type="cite"
cite="mid:af2d05b8-01b2-61e3-a729-c29d2479599c@oracle.com"
class="">
<div class="moz-cite-prefix"> <br class="">
Chris<br class="">
<br class="">
On 3/15/20 10:22 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:c15a019e-1dad-16de-c8b7-0ca9e97ada97@oracle.com" class="">
<div class="moz-cite-prefix">Hi Chris,<br
class="">
<br class="">
Looks good.<br class="">
Thank you for update!<br class="">
<br class="">
Thanks,<br class="">
Serguei<br class="">
<br class="">
<br class="">
On 3/15/20 17:47, Chris Plummer wrote:<br
class="">
</div>
<blockquote type="cite"
\
cite="mid:a2af1551-2450-4efb-00a0-ab921dc68fa7@oracle.com" class="">
<div class="moz-cite-prefix">I changed
them all to "SA Attach" and grepped to
make sure there are no other occurrences
of "SA attach".<br class="">
<br class="">
thanks,<br class="">
<br class="">
Chris<br class="">
<br class="">
On 3/15/20 4:49 PM, Igor Ignatyev wrote:<br
class="">
</div>
<blockquote type="cite"
\
cite="mid:78834ED4-055B-4E43-86A8-01BB49BC3C73@oracle.com" class=""> Hi Chris,
<div class=""><br class="">
</div>
<div class="">looks good, thanks!</div>
<div class=""><br class="">
</div>
<div class="">one minor nit,
in SATestUtils::skipIfCannotAttach,
you have two exception messages which
start with 'SA attach not expected to
work.', and one w/ 'SA Attach not
expected to work.' (w/ Attach instead
of attach), it'd be nicer to have them
uniform. </div>
<div class=""><br class="">
</div>
<div class="">Cheers,</div>
<div class="">-- Igor<br class="">
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On Mar 15, 2020, at
4:35 PM, Chris Plummer <<a
href="mailto:chris.plummer@oracle.com"
class=""
\
moz-do-not-send="true">chris.plummer@oracle.com</a>> wrote:</div>
<br
class="Apple-interchange-newline">
<div class="">
<div class="">
<div class="moz-cite-prefix">Hi
Igor,<br class="">
<br class="">
Thanks for the review.
Here's and updated webrev
with all of the suggestions
from you and Serguei:<br
class="">
<br class="">
<a
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~cjplummer/8238268/webrev.01/index.html"
\
moz-do-not-send="true">http://cr.openjdk.java.net/~cjplummer/8238268/webrev.01/index.html</a><br
class="">
<br class="">
Also some comments inline
below.<br class="">
<br class="">
On 3/13/20 9:26 AM, Igor
Ignatyev wrote:<br class="">
</div>
<blockquote type="cite"
\
cite="mid:BAAB2AF7-B0C0-4C01-890A-F2F0FC360C42@oracle.com" 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>
</blockquote>
Ok, but it's unclear to me
what requires.properties is
even for, and what is the
impact of extra or missing
properties. What kind of test
would catch these errors?<br
class="">
</div>
</div>
</blockquote>
jtreg uses 'requires.properties' as
a list of extra variables for
@require expressions, if a test uses
a name which isn't
in 'requires.properties' (or known
to jtreg), jtreg won't execute such
test and will set its status to
Error w/ 'invalid name ...' message.</div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">
<div class="">
<blockquote type="cite"
\
cite="mid:BAAB2AF7-B0C0-4C01-890A-F2F0FC360C42@oracle.com" class="">
<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>
</blockquote>
Ok.<br class="">
<br class="">
throw new
RuntimeException("sudo process
interrupted", e);<br class="">
<br class="">
<blockquote type="cite"
\
cite="mid:BAAB2AF7-B0C0-4C01-890A-F2F0FC360C42@oracle.com" class="">
<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>
</blockquote>
Ok, but I still left the
comment in place.<br class="">
<blockquote type="cite"
\
cite="mid:BAAB2AF7-B0C0-4C01-890A-F2F0FC360C42@oracle.com" class="">
<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>
</blockquote>
Ok.<br class="">
<blockquote type="cite"
\
cite="mid:BAAB2AF7-B0C0-4C01-890A-F2F0FC360C42@oracle.com" class="">
<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>
</blockquote>
Ok.<br class="">
<blockquote type="cite"
\
cite="mid:BAAB2AF7-B0C0-4C01-890A-F2F0FC360C42@oracle.com" class="">
<div class="">I've briefly
looked at all the changed
tests and they look good.</div>
</blockquote>
<br class="">
Thanks!<br class="">
<br class="">
Chris<br class="">
<blockquote type="cite"
\
cite="mid:BAAB2AF7-B0C0-4C01-890A-F2F0FC360C42@oracle.com" class="">
<div class=""><br class="">
</div>
<div class="">Thanks,</div>
<div class="">-- Igor </div>
<div class=""><br class="">
</div>
<div class=""><br class="">
<div class="">
<blockquote type="cite"
class="">
<div class="">On Mar
12, 2020, at 11:06
PM, Chris Plummer
<<a
\
href="mailto:chris.plummer@oracle.com" class=""
\
moz-do-not-send="true">chris.plummer@oracle.com</a>> wrote:</div>
<br
\
class="Apple-interchange-newline"> <div class="">
<div 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" \
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br
class="">
</div>
<blockquote
type="cite"
\
cite="mid:33ffd0e7-d017-c1bf-feb5-4d2f07e753f2@oracle.com" 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/h \
otspot/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/h \
otspot/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/h \
otspot/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>
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</div>
</blockquote>
</div>
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]