[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR: JDK-8215544: SA: Modify ClhsdbLauncher to add sudo privileges to enable MacOS tests on Mach
From: "Lindenmaier, Goetz" <goetz.lindenmaier () sap ! com>
Date: 2019-01-21 10:25:17
Message-ID: 99cb1e48ee444f0bbb0c5f5741a75752 () sap ! com
[Download RAW message or body]
Sure, I'll try to test it.
Best regards,
Goetz.
> -----Original Message-----
> From: Jini George <jini.george@oracle.com>
> Sent: Sonntag, 20. Januar 2019 17:12
> To: David Holmes <david.holmes@oracle.com>; Lindenmaier, Goetz
> <goetz.lindenmaier@sap.com>; serviceability-dev@openjdk.java.net
> Subject: Re: RFR: JDK-8215544: SA: Modify ClhsdbLauncher to add sudo
> privileges to enable MacOS tests on Mach5
>
> Thanks, Goetz for letting me know of the failure and thanks, David for
> the suggestion wrt SkippedException. Goetz, I will send you a patch
> off-list, and it would be great if you could test it again for me.
>
> Thanks,
> Jini.
>
> On 1/20/2019 4:18 AM, David Holmes wrote:
> > 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@openjdk.java.net> On
> >>> Behalf Of Jini George
> >>> Sent: Montag, 14. Januar 2019 06:08
> >>> To: Sharath Ballal <sharath.ballal@oracle.com>; JC Beyler
> >>> <jcbeyler@google.com>; serviceability-dev@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@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@oracle.com
> >>>>> <mailto:jini.george@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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic