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

[David Holmes]

On 18/01/2019 11:30 pm, Lindenmaier, Goetz wrote:
> 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;

We should be able to use SkippedException for this case.

David

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