RFR: JDK-8215544: SA: Modify ClhsdbLauncher to add sudo privileges to enable MacOS tests on Mach5

[Lindenmaier, Goetz]

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-bounces at openjdk.java.net> On
> Behalf Of Jini George
> Sent: Montag, 14. Januar 2019 06:08
> To: Sharath Ballal <sharath.ballal at oracle.com>; JC Beyler
> <jcbeyler at google.com>; serviceability-dev at 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 at 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.george at oracle.com
> >> <mailto:jini.george at 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