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; } } 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