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

[Hohensee, Paul]

A more generic approach that doesn’t check for a specific platform in ClhsdbLauncher.java would be nice, e.g., add a checkForPrivileges() method to Platform, but if OSX is the only platform that needs the check, I'm ok with this patch.

Thanks,

Paul

On 1/10/19, 8:16 PM, "serviceability-dev on behalf of Jini George" <serviceability-dev-bounces at openjdk.java.net on behalf of jini.george at oracle.com> wrote:

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