Hi Chris, does canPtraceAttachLinux have to be public?
otherwise, looks good to me. -- Igor > On Mar 16, 2020, at 5:11 PM, Chris Plummer <[email protected]> wrote: > > Hi Serguei and Igor, > > New webrev: > > http://cr.openjdk.java.net/~cjplummer/8238268/webrev.02/index.html > <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.02/index.html> > > Only files changed were Platform.java and SATestUtils.java. > > -Moved canPtraceAttachLinux() from Platform.java to SATestUtils.java > -Changed Platform.canPtraceAttachLinux() reference in SATestUtils.java to > just be canPtraceAttachLinux(). > -Had to change userName.equals("root") reference in canPtraceAttachLinux() to > Platform.isRoot(). Probably should have been that way in the first place. > -Made some adjustments to the imports > > thanks, > > Chris > > On 3/16/20 12:13 PM, Igor Ignatev wrote: >> >> >>> On Mar 16, 2020, at 11:43 AM, "[email protected]" >>> <mailto:[email protected]> <[email protected]> >>> <mailto:[email protected]> wrote: >>> >>> >>> On 3/16/20 11:26, Chris Plummer wrote: >>>> I had to make another change. TestMutuallyExclusivePlatformPredicates.java >>>> failed when I ran tier 3. I had fixed it a long while back due to >>>> Platform.shouldSAAttach() being removed, but there were more changes to >>>> Platform.java after that that I didn't account for. isRoot() was added and >>>> canPtraceAttrachLinux() was made public. So this is what the diff looks >>>> like now: <> >>>> >>>> --- >>>> a/test/hotspot/jtreg/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java >>>> +++ >>>> b/test/hotspot/jtreg/testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java >>>> @@ -51,9 +51,9 @@ >>>> VM_TYPE("isClient", "isServer", "isMinimal", "isZero", >>>> "isEmbedded"), >>>> MODE("isInt", "isMixed", "isComp"), >>>> IGNORED("isEmulatedClient", "isDebugBuild", "isFastDebugBuild", >>>> - "isSlowDebugBuild", "hasSA", "shouldSAAttach", >>>> "isTieredSupported", >>>> + "isSlowDebugBuild", "hasSA", "canPtraceAttachLinux", >>>> "isTieredSupported", >>>> "areCustomLoadersSupportedForCDS", >>>> "isDefaultCDSArchiveSupported", >>>> - "isSignedOSX"); >>>> + "isSignedOSX", "isRoot"); >>>> >>>> However, I'm thinking maybe I should just move canPtraceAttachLinux() to >>>> SATestUtils.java since that's the only user, and it is an SA specific API. >>>> What do you think? >>> >>> The approach to localize canPtraceAttachLinux() in SATestUtils.java sounds >>> right to me if it is an SA specific API. >>> >> +1 >> — Igor >> >>> Thanks, >>> Serguei >>> >>>> >>>> Chris >>>> >>>> On 3/15/20 10:22 PM, [email protected] >>>> <mailto:[email protected]> wrote: >>>>> Hi Chris, >>>>> >>>>> Looks good. >>>>> Thank you for update! >>>>> >>>>> Thanks, >>>>> Serguei >>>>> >>>>> >>>>> On 3/15/20 17:47, Chris Plummer wrote: >>>>>> I changed them all to "SA Attach" and grepped to make sure there are no >>>>>> other occurrences of "SA attach". >>>>>> >>>>>> thanks, >>>>>> >>>>>> Chris >>>>>> >>>>>> On 3/15/20 4:49 PM, Igor Ignatyev wrote: >>>>>>> Hi Chris, >>>>>>> >>>>>>> looks good, thanks! >>>>>>> >>>>>>> one minor nit, in SATestUtils::skipIfCannotAttach, you have two >>>>>>> exception messages which start with 'SA attach not expected to work.', >>>>>>> and one w/ 'SA Attach not expected to work.' (w/ Attach instead of >>>>>>> attach), it'd be nicer to have them uniform. >>>>>>> >>>>>>> Cheers, >>>>>>> -- Igor >>>>>>> >>>>>>>> On Mar 15, 2020, at 4:35 PM, Chris Plummer <[email protected] >>>>>>>> <mailto:[email protected]>> wrote: >>>>>>>> >>>>>>>> Hi Igor, >>>>>>>> >>>>>>>> Thanks for the review. Here's and updated webrev with all of the >>>>>>>> suggestions from you and Serguei: >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.01/index.html >>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.01/index.html> >>>>>>>> >>>>>>>> Also some comments inline below. >>>>>>>> >>>>>>>> On 3/13/20 9:26 AM, Igor Ignatyev wrote: >>>>>>>>> HI Chris, >>>>>>>>> >>>>>>>>> overall looks good to me, a few comments though: >>>>>>>>> 1. since you removed vm.hasSAandCanAttach from VMProps, you also need >>>>>>>>> to remove it from all TEST.ROOT files which mention it >>>>>>>>> (test/jdk/TEST.ROOT and test/hotspot/jtreg/TEST.ROOT) so people won't >>>>>>>>> be confused by undefined property and jtreg will be able to properly >>>>>>>>> report invalid usages of it if any. >>>>>>>> Ok, but it's unclear to me what requires.properties is even for, and >>>>>>>> what is the impact of extra or missing properties. What kind of test >>>>>>>> would catch these errors? >>>>>>> jtreg uses 'requires.properties' as a list of extra variables for >>>>>>> @require expressions, if a test uses a name which isn't in >>>>>>> 'requires.properties' (or known to jtreg), jtreg won't execute such >>>>>>> test and will set its status to Error w/ 'invalid name ...' message. >>>>>>> >>>>>>>>> >>>>>>>>> 2. in SATestUtils::canAddPrivileges, could you please add some >>>>>>>>> meaningful message to the RuntimeException at L#102? >>>>>>>>> >>>>>>>> Ok. >>>>>>>> >>>>>>>> throw new RuntimeException("sudo process interrupted", e); >>>>>>>> >>>>>>>>> 3. SATestUtils::checkAttachOk method name is somewhat misleading >>>>>>>>> (hence you had to put comment every time you used it), I'd recommend >>>>>>>>> you to rename to smth like skipIfCannotAttach(). >>>>>>>> Ok, but I still left the comment in place. >>>>>>>>> >>>>>>>>> 4. in SATestUtils::checkAttachOk's javadoc, it would be better to use >>>>>>>>> @throws tag like: >>>>>>>>>> + /** >>>>>>>>>> + * Checks if SA Attach is expected to work. >>>>>>>>>> +. * @throws SkippedException ifSA Attach is not expected to work. >>>>>>>>>> + */ >>>>>>>>> >>>>>>>>> >>>>>>>> Ok. >>>>>>>>> 5. it also might make sense to catch IOException within >>>>>>>>> SATestUtils::checkAttachOk and throw it as Error or RuntimeException. >>>>>>>>> >>>>>>>> Ok. >>>>>>>>> I've briefly looked at all the changed tests and they look good. >>>>>>>> >>>>>>>> Thanks! >>>>>>>> >>>>>>>> Chris >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> -- Igor >>>>>>>>> >>>>>>>>> >>>>>>>>>> On Mar 12, 2020, at 11:06 PM, Chris Plummer >>>>>>>>>> <[email protected] <mailto:[email protected]>> wrote: >>>>>>>>>> >>>>>>>>>> Hi Serguei, >>>>>>>>>> >>>>>>>>>> Thanks for the review! >>>>>>>>>> >>>>>>>>>> Can I get one more reviewer please? >>>>>>>>>> >>>>>>>>>> thanks, >>>>>>>>>> >>>>>>>>>> Chris >>>>>>>>>> >>>>>>>>>> On 3/12/20 12:06 AM, [email protected] >>>>>>>>>> <mailto:[email protected]> wrote: >>>>>>>>>>> Hi Chris, >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On 3/12/20 00:03, Chris Plummer wrote: >>>>>>>>>>>> Hi Serguei, >>>>>>>>>>>> >>>>>>>>>>>> That check used to be in Platform.shouldSAAttach(), which >>>>>>>>>>>> essentially was moved to SATestUtils.checkAttachOk() and reworked >>>>>>>>>>>> some. It was necessary in Platform.shouldSAAttach() since that was >>>>>>>>>>>> used to evaluation vm.hasSAandCanAttach (which is now gone). When >>>>>>>>>>>> I moved everything to SATestUtils.checkAttachOk(), I recall >>>>>>>>>>>> thinking it wasn't really necessary since all tests that call it >>>>>>>>>>>> should have @require vm.hasSA, but left it in anyway just to be >>>>>>>>>>>> extra safe. I'm still inclined to just leave it in, but would not >>>>>>>>>>>> be opposed to removing it. >>>>>>>>>>> >>>>>>>>>>> I agree, it is more safe to keep it, at list for now. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Serguei >>>>>>>>>>> >>>>>>>>>>>> thanks, >>>>>>>>>>>> >>>>>>>>>>>> Chris >>>>>>>>>>>> >>>>>>>>>>>> On 3/11/20 11:20 PM, [email protected] >>>>>>>>>>>> <mailto:[email protected]> wrote: >>>>>>>>>>>>> Hi Chris, >>>>>>>>>>>>> >>>>>>>>>>>>> I've made another pass today. >>>>>>>>>>>>> It looks good to me. >>>>>>>>>>>>> >>>>>>>>>>>>> I have just one minor questions. >>>>>>>>>>>>> >>>>>>>>>>>>> There is some overlap between the requires vm.hasSA check and >>>>>>>>>>>>> checkAttachOk: >>>>>>>>>>>>> + public static void checkAttachOk() throws IOException { >>>>>>>>>>>>> + if (!Platform.hasSA()) { >>>>>>>>>>>>> + throw new SkippedException("SA not supported."); >>>>>>>>>>>>> + } >>>>>>>>>>>>> In the former case, the test is not run but in the latter the >>>>>>>>>>>>> SkippedException is thrown. >>>>>>>>>>>>> As I see, all tests with the checkAttachOk call use requires >>>>>>>>>>>>> vm.hasSA as well. >>>>>>>>>>>>> It can be that the first check "if (!Platform.hasSA())" in the >>>>>>>>>>>>> checkAttachOk is redundant. >>>>>>>>>>>>> It is okay and more safe in general but generates little >>>>>>>>>>>>> confusion. >>>>>>>>>>>>> I'm okay if you don't do anything with this but wanted to know >>>>>>>>>>>>> your view. >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> Serguei >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On 3/10/20 18:57, Chris Plummer wrote: >>>>>>>>>>>>>> On 3/10/20 6:07 PM, [email protected] >>>>>>>>>>>>>> <mailto:[email protected]> wrote: >>>>>>>>>>>>>>> Hi Chris, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Overall, this looks as a right direction to me while it is not >>>>>>>>>>>>>>> easy to verify all the details. >>>>>>>>>>>>>> Yes, there are a lot of tests with quite a few different types >>>>>>>>>>>>>> of changes. I did a lot of testing and verified that when the >>>>>>>>>>>>>> tests pass, they pass for the right reasons (really ran the >>>>>>>>>>>>>> test, skipped due to lack of privileges, or skipped due to >>>>>>>>>>>>>> running signed on OSX 10.14 or later). I also verified locally >>>>>>>>>>>>>> running as root, running with a cached sudo, and running without >>>>>>>>>>>>>> sudo. >>>>>>>>>>>>>>> I'll make another pass tomorrow. >>>>>>>>>>>>>> Thanks! >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> A couple of quick nits so far: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java.udiff.html >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java.udiff.html> >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java.udiff.html >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java.udiff.html> >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForInvokeDynamic.java.udiff.html >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForInvokeDynamic.java.udiff.html> >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java.udiff.html >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java.udiff.html> >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackMixed.java.udiff.html >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackMixed.java.udiff.html> >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestRevPtrsForInvokeDynamic.java.udiff.html >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestRevPtrsForInvokeDynamic.java.udiff.html> >>>>>>>>>>>>>>> import jdk.test.lib.Utils; >>>>>>>>>>>>>>> -import jdk.test.lib.Asserts; >>>>>>>>>>>>>>> +import jdk.test.lib.SA.SATestUtils; >>>>>>>>>>>>>>> Need to swap these exports. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> Ok >>>>>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/lib/jdk/test/lib/SA/SATestUtils.java.frames.html >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/lib/jdk/test/lib/SA/SATestUtils.java.frames.html> >>>>>>>>>>>>>>> 48 if (SATestUtils.needsPrivileges()) { >>>>>>>>>>>>>>> 49 cmdStringList = >>>>>>>>>>>>>>> SATestUtils.addPrivileges(cmdStringList); >>>>>>>>>>>>>>> The method calls are local, so the class name can be omitted in >>>>>>>>>>>>>>> the method names: >>>>>>>>>>>>>>> SATestUtils.needsPrivileges and SATestUtils.addPrivileges. >>>>>>>>>>>>>> Ok >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 94 try { >>>>>>>>>>>>>>> 95 if (echoProcess.waitFor(60, TimeUnit.SECONDS) >>>>>>>>>>>>>>> == false) { >>>>>>>>>>>>>>> 96 // Due to using the "-n" option, sudo >>>>>>>>>>>>>>> should complete almost immediately. 60 seconds >>>>>>>>>>>>>>> 97 // is more than generous. If it didn't >>>>>>>>>>>>>>> complete in that time, something went very wrong. >>>>>>>>>>>>>>> 98 echoProcess.destroyForcibly(); >>>>>>>>>>>>>>> 99 throw new RuntimeException("Timed out >>>>>>>>>>>>>>> waiting for sudo to execute."); >>>>>>>>>>>>>>> 100 } >>>>>>>>>>>>>>> 101 } catch (InterruptedException e) { >>>>>>>>>>>>>>> 102 throw new RuntimeException(e); >>>>>>>>>>>>>>> 103 } >>>>>>>>>>>>>>> The lines 101/103 are misaligned. >>>>>>>>>>>>>> Ok. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>> Serguei >>>>>>>>>>>>>>> >>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>> >>>>>>>>>>>>>> Chris >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 3/9/20 19:29, Chris Plummer wrote: >>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Please help review the following: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8238268 >>>>>>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8238268> >>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/ >>>>>>>>>>>>>>>> <http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I'll try to give enough background first to make it easier to >>>>>>>>>>>>>>>> understand the changes. On OSX you must run SA tests that >>>>>>>>>>>>>>>> attach to a live process as root or using sudo. For example: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> sudo make run-test >>>>>>>>>>>>>>>> TEST=serviceability/sa/ClhsdbJstackXcompStress.java >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Whether running as root or under sudo, the check to allow the >>>>>>>>>>>>>>>> test to run is done with: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> private static boolean canAttachOSX() { >>>>>>>>>>>>>>>> return userName.equals("root"); >>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Any test using "@requires vm.hasSAandCanAttach" must pass this >>>>>>>>>>>>>>>> check via Platform.shouldSAAttach(), which for OSX returns: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> return canAttachOSX() && !isSignedOSX(); >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> So if running as root the "@requires vm.hasSAandCanAttach" >>>>>>>>>>>>>>>> passes, otherwise it does not. However, using a root login to >>>>>>>>>>>>>>>> run tests is not a very desirable, nor is issuing a "sudo make >>>>>>>>>>>>>>>> run-test" (any created file ends up with root ownership). >>>>>>>>>>>>>>>> Because of this support was previously added for just running >>>>>>>>>>>>>>>> the attaching process using sudo, not the entire test. This >>>>>>>>>>>>>>>> was only done for the 20 or so tests that use ClhsdbLauncher. >>>>>>>>>>>>>>>> These tests use "@requires vm.hasSA", and then while running >>>>>>>>>>>>>>>> the test will do a "sudo" check if canAttachOSX() returns >>>>>>>>>>>>>>>> false: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> if (!Platform.shouldSAAttach()) { >>>>>>>>>>>>>>>> if (Platform.isOSX()) { >>>>>>>>>>>>>>>> if (Platform.isSignedOSX()) { >>>>>>>>>>>>>>>> throw new SkippedException("SA attach not >>>>>>>>>>>>>>>> expected to work. JDK is signed."); >>>>>>>>>>>>>>>> } else if (SATestUtils.canAddPrivileges()) { >>>>>>>>>>>>>>>> needPrivileges = true; >>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>> if (!needPrivileges) { >>>>>>>>>>>>>>>> // Skip the test if we don't have enough >>>>>>>>>>>>>>>> permissions to attach >>>>>>>>>>>>>>>> // and cannot add privileges. >>>>>>>>>>>>>>>> throw new SkippedException( >>>>>>>>>>>>>>>> "SA attach not expected to work. >>>>>>>>>>>>>>>> Insufficient privileges."); >>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>> } >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> So basically it does a runtime check of vm.hasSAandCanAttach, >>>>>>>>>>>>>>>> and if it fails then checks if running with sudo will work. >>>>>>>>>>>>>>>> This allows for either a passwordless sudo to be used when >>>>>>>>>>>>>>>> running clhsdb, or for the user to be prompted for the sudo >>>>>>>>>>>>>>>> password (note I've remove support for the latter with my >>>>>>>>>>>>>>>> changes). >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> That brings us to the CR that is being fixed. ClhsdbLauncher >>>>>>>>>>>>>>>> tests support sudo and will therefore run with our CI testing >>>>>>>>>>>>>>>> on OSX, but the 25 or so tests that use "@requires >>>>>>>>>>>>>>>> vm.hasSAandCanAttach" do not, and therefore are never run with >>>>>>>>>>>>>>>> our CI OSX testing. The changes in this webrev fix that. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> There are two possible approaches to the fix. One is having >>>>>>>>>>>>>>>> the check for sudo be done as part of the vm.hasSAandCanAttach >>>>>>>>>>>>>>>> evaluation. The other approach is to do the check in the test >>>>>>>>>>>>>>>> at runtime similar to how ClhsdbLauncher currently does. This >>>>>>>>>>>>>>>> would mean just using "@requires vm.hasSA" for all the tests >>>>>>>>>>>>>>>> instead of "@requires vm.hasSAandCanAttach". I chose the later >>>>>>>>>>>>>>>> because there is an advantage to throwing SkippedException >>>>>>>>>>>>>>>> rather than just silently skipping the test using @requires. >>>>>>>>>>>>>>>> The advantage is that mdash tells you how many tests were >>>>>>>>>>>>>>>> skipped, and when you hover over the reason you can see the >>>>>>>>>>>>>>>> SkippedException message, which will differentiate between >>>>>>>>>>>>>>>> reasons like the JDK was signed or there are insufficient >>>>>>>>>>>>>>>> privileges. If all the checking was done by the >>>>>>>>>>>>>>>> vm.hasSAandCanAttach evaluation, you would not know why the >>>>>>>>>>>>>>>> test wasn't run. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> The "support" related changes made are all in the following 3 >>>>>>>>>>>>>>>> files. The rest of the changes are in the tests: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> test/jtreg-ext/requires/VMProps.java >>>>>>>>>>>>>>>> test/lib/jdk/test/lib/Platform.java >>>>>>>>>>>>>>>> test/lib/jdk/test/lib/SA/SATestUtils.java >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> You'll noticed that one change I made to the sudo support in >>>>>>>>>>>>>>>> SATestUtils.canAddPrivileges() is to make sudo >>>>>>>>>>>>>>>> non-interactive, which means no password prompt. So that means >>>>>>>>>>>>>>>> either the user does not require a password, or the >>>>>>>>>>>>>>>> credentials have been cached. Otherwise the sudo check will >>>>>>>>>>>>>>>> fail. On most platforms if you execute a sudo command, the >>>>>>>>>>>>>>>> credentials are cached for 5 minutes. So if your user is not >>>>>>>>>>>>>>>> setup for passwordless sudo, then a sudo command can be issued >>>>>>>>>>>>>>>> before running the tests, and will likely remain cached until >>>>>>>>>>>>>>>> the test is run. The reason for using passwordless is because >>>>>>>>>>>>>>>> prompting in the middle of running tests can be confusing (you >>>>>>>>>>>>>>>> usually walk way once launching the tests and miss the prompt >>>>>>>>>>>>>>>> anyway), and avoids unnecessary delays in automated testing >>>>>>>>>>>>>>>> due to waiting for the password prompt to timeout (it used to >>>>>>>>>>>>>>>> wait 1 minute). >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> There are essentially 3 types of tests that SA Attach to a >>>>>>>>>>>>>>>> process, each needing a slightly different fix: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 1. Tests that directly launch a jdk.hotspot.agent class, such >>>>>>>>>>>>>>>> as TestClassDump.java. They need to call >>>>>>>>>>>>>>>> SATestUtils.checkAttachOk() to verify that attaching will be >>>>>>>>>>>>>>>> possible, and then SATestUtils.addPrivilegesIfNeeded(pb) to >>>>>>>>>>>>>>>> get the sudo command added if needed.They also need to switch >>>>>>>>>>>>>>>> from using hasSAandCanAttach to using hasSA. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 2. Tests that launch command line tools such has jhsdb. They >>>>>>>>>>>>>>>> need to call SATestUtils.checkAttachOk() to verify that >>>>>>>>>>>>>>>> attaching will be possible, and then >>>>>>>>>>>>>>>> SATestUtils.createProcessBuilder() to create a process that >>>>>>>>>>>>>>>> will be launched using sudo if necessary.They also need to >>>>>>>>>>>>>>>> switch from using hasSAandCanAttach to using hasSA. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 3. Tests that use ClhsdbLauncher. They already use hasSA >>>>>>>>>>>>>>>> instead of hasSAandCanAttach, and rely on ClhsdbLauncher to do >>>>>>>>>>>>>>>> check at runtime if attaching will work, so for the most part >>>>>>>>>>>>>>>> all the these tests are unchanged. ClhsdbLauncher was modified >>>>>>>>>>>>>>>> to take advantage of the new >>>>>>>>>>>>>>>> SATestUtils.createProcessBuilder() and >>>>>>>>>>>>>>>> SATestUtils.checkAttachOk() APIs. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Some tests required special handling: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> test/hotspot/jtreg/compiler/ciReplay/TestSAClient.java >>>>>>>>>>>>>>>> test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> - These two tests SA Attach to a core file, not to a process, >>>>>>>>>>>>>>>> so only need hasSA, >>>>>>>>>>>>>>>> not hasSAandCanAttach. No other changes were needed. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/ClhsdbFindPC.java >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> - The output should never be null. If the test was skipped due >>>>>>>>>>>>>>>> to lack of privileges, you >>>>>>>>>>>>>>>> would never get to this section of the test. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/TestClhsdbJstackLock.java >>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/TestIntConstant.java >>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/TestPrintMdo.java >>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/TestType.java >>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/TestUniverse.java >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> - These are ClhsdbLauncher tests, so they should have been >>>>>>>>>>>>>>>> using hasSA instead of >>>>>>>>>>>>>>>> hasSAandCanAttachin the first place. No other changes were >>>>>>>>>>>>>>>> needed. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java >>>>>>>>>>>>>>>> test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> - These tests used to "@require mac" but seem run fine on OSX, >>>>>>>>>>>>>>>> so I removed this requirement. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> test/jdk/sun/tools/jhsdb/BasicLauncherTest.java >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> - This test had a runtime check to not run on OSX due to not >>>>>>>>>>>>>>>> having core file stack >>>>>>>>>>>>>>>> walking support. However, this tests always attaches to a >>>>>>>>>>>>>>>> process, not a core file, >>>>>>>>>>>>>>>> and seems to run just fine on OSX. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> test/jdk/sun/tools/jstack/DeadlockDetectionTest.java >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> - I changed the test to throw a SkippedException if it gets >>>>>>>>>>>>>>>> the unexpected error code >>>>>>>>>>>>>>>> rather than just println. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> And a few other miscellaneous changes not already covered: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> test/lib/jdk/test/lib/Platform.java >>>>>>>>>>>>>>>> - Made canPtraceAttachLinux() public so it can be called from >>>>>>>>>>>>>>>> SATestUtils. >>>>>>>>>>>>>>>> - vm.hasSAandCanAttach is now gone. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> thanks, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Chris >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >
