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

Jini George jini.george at oracle.com
Wed Jan 9 04:11:11 UTC 2019


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.





More information about the serviceability-dev mailing list