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

[Jini George]

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