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

[Sharath Ballal]

Hi Jini,

ClhsdbLauncher.java
1. Is the platform check not required in case of core files ?
-        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


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/serviceability/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