On 18/01/2019 11:30 pm, Lindenmaier, Goetz wrote:
Hi Jini,
we see test failing after this change arrived deeper in our infrastructure.
On a linuxx86_64 docker container it does not work.
This is because you now throw Error() if shouldSAAttach() returns false.
I think you need this:
--- a/test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java Thu Jan 03
17:30:03 2019 +0100
+++ b/test/hotspot/jtreg/serviceability/sa/ClhsdbLauncher.java Fri Jan 18
14:25:05 2019 +0100
@@ -190,8 +190,9 @@
needPrivileges = true;
}
} else {
- System.out.println("SA attach not expected to work. Insufficient
privileges.");
- throw new Error("Cannot attach.");
+ // Silently skip the test if we don't have enough permissions
to attach
+ System.out.println("SA attach not expected to work - test
skipped.");
+ return null;
We should be able to use SkippedException for this case.
David
}
}
This is because canPtraceAttachLinux() in Platform.java returns false.
What do you think?
Best regards,
Goetz.
-----Original Message-----
From: serviceability-dev <serviceability-dev-boun...@openjdk.java.net> On
Behalf Of Jini George
Sent: Montag, 14. Januar 2019 06:08
To: Sharath Ballal <sharath.bal...@oracle.com>; JC Beyler
<jcbey...@google.com>; serviceability-dev@openjdk.java.net
Subject: Re: RFR: JDK-8215544: SA: Modify ClhsdbLauncher to add sudo
privileges to enable MacOS tests on Mach5
Thank you very much, Sharath, for the review. My comments inline.
On 1/12/2019 3:38 PM, Sharath Ballal wrote:
Hi Jini,
ClhsdbLauncher.java
1. Is the platform check not required in case of core files ?
No, it is not needed. The permission issues don't exist.
- if (!Platform.shouldSAAttach()) {
- // Silently skip the test if we don't have enough permissions to
attach
- System.out.println("SA attach not expected to work - test
skipped.");
- return null;
- }
-
2. Nit: extra line at 135
Done.
Thanks,
Jini.
Thanks,
Sharath
-----Original Message-----
From: Jini George
Sent: Friday, January 11, 2019 9:46 AM
To: JC Beyler; serviceability-dev@openjdk.java.net
Subject: Re: RFR: JDK-8215544: SA: Modify ClhsdbLauncher to add sudo
privileges to enable MacOS tests on Mach5
Thanks again, JC! :-)
Folks, Could I please get one more review on this ?
-Jini.
On 1/11/2019 9:38 AM, JC Beyler wrote:
Hi Jini,
Looks good to me now :) Thanks for handling the comments :) Jc
On Tue, Jan 8, 2019 at 8:11 PM Jini George <jini.geo...@oracle.com
<mailto:jini.geo...@oracle.com>> wrote:
Thank you very much for the review, JC. My comments inline:
On 1/8/2019 12:22 AM, JC Beyler wrote:
>
>
http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/hotspot/jtreg/s
erviceability/sa/ClhsdbLauncher.java.udiff.html
> + // by appending 'sudo' to it. Check now if sudo
> passes in this
> + // environment with a simple 'echo' command.
> -> Really shouldn't provide implementation details about
what is
> being done by that method; if we change that, this comment will
rot :-)
> canAddPrivileges is explicit enough in my mind
Done. I have removed the comment.
>
>
http://cr.openjdk.java.net/~jgeorge/8215544/webrev.03/test/lib/jdk/tes
t/lib/SA/SATestUtils.java.html
>
> -> You put some System.out.println; are they intended to
still be
> there or were they for debug?
I guess you meant this one:
System.out.println("Adding 'sudo -E' to the command.");
This one is meant to be there.
>
>
> 77 for (String val: cmdStringList) {
> 78 outStringList.add(val);
> 79 }
>
>
> Couldn't we use addAll ?
Done.
The modified webrev is at:
http://cr.openjdk.java.net/~jgeorge/8215544/webrev.04/index.html
Thanks,
Jini.
--
Thanks,
Jc
