Hi Jini Thanks for reviewing again, I fixed the typo's!
Best regards, Goetz. > -----Original Message----- > From: Jini George <jini.geo...@oracle.com> > Sent: Monday, June 25, 2018 6:20 PM > To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; 'Chris Plummer' > <chris.plum...@oracle.com>; serviceability-dev (serviceability- > d...@openjdk.java.net) <serviceability-dev@openjdk.java.net> > Subject: Re: RFR(S/M): 8205419: [testbug] TestJmapCore failing without SA: > introduce @requires vm.hasSA > > Hi Goetz, > > Your changes look good, though I feel that even for the tests that use > ClhsdbLauncher and which need an attach, an "@requires > vm.hasSAandCanAttach" could be used, and the Platform.shouldSAAttach() > calls can be removed from ClhsdbLauncher. > > Some nits: > > 1. In VMProps.java, > > servicability ==> serviceability (an extra 'e') > > 2. In Platform.java > > reqires ==> requires > > Thanks, > Jini (Not a (R)eviewer). > > On 6/22/2018 1:50 PM, Lindenmaier, Goetz wrote: > > Hi, > > > > Ok, so I now added > > vm.hasSA and > > vm.hasSAandCanAttach > > I use the first in the JMapCore tests, and in the tests that don't check > shouldSAAttach() > > right at the beginning. > > Wherever the test is immediately ended after checking shouldSAAttach(), I > > removed shouldSAAttach(), and added vm.hasSAandCanAttach. The > others > > depend on ClhsdbLauncher. Just checking vm.hasSA is on the safe side > > if runOnCore() is changed. > > http://cr.openjdk.java.net/~goetz/wr18/8205419-requiresHasSA/02/ > > > > This also makes the implementation of VMProps and VMPlatform more > clear > > as requested by Chris. > > > > Best regards, > > Goetz. > > > > > >> -----Original Message----- > >> From: Jini George <jini.geo...@oracle.com> > >> Sent: Thursday, June 21, 2018 12:18 PM > >> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; 'Chris Plummer' > >> <chris.plum...@oracle.com>; serviceability-dev (serviceability- > >> d...@openjdk.java.net) <serviceability-dev@openjdk.java.net> > >> Subject: Re: RFR(S/M): 8205419: [testbug] TestJmapCore failing without > SA: > >> introduce @requires vm.hasSA > >> > >> Hi Goetz, > >> > >> Tests like TestJmapCore which involve only corefile debugging by SA (and > >> not attaching to a live process) would only need to check for the > >> presence of SA, and would not need to check for ptrace related attach > >> permissions like what is done for Linux and OSX -- since super user > >> privileges are not required to debug corefiles. So a single @requires > >> catering to both these might not be desirable. > >> > >> Having said that, I realized that runOnCore() of ClhsdbLauncher > >> incorrectly checks for the ptrace related attach permissions. (Will file > >> an issue to correct this). > >> > >> An @requires instead of a shouldSAAttach() might help in avoiding fake > >> passes like those on OSX when not run as "root". > >> (https://bugs.openjdk.java.net/browse/JDK-8199700) > >> > >> Thanks, > >> Jini. > >> > >> > >> > >> > >> On 6/21/2018 1:52 AM, Lindenmaier, Goetz wrote: > >>> Hi Chris, > >>> > >>> Thanks for looking at this change. > >>> > >>>> Can't VMProps.vmHasSA() just return Platform.shouldSAAttach()? It > >> seems > >>>> you are unnecessarily replicating Platform.shouldSAAttach() logic. > >>> I think there are two things to distinguish: > >>> - platforms that don't implement SA > >>> - systems/setups where SA does not work. > >>> If both can be recognized when VMProps is evaluated, shouldSAAttach() > >>> is the one that is superfluous in the long run?! If not, shouldSAAttach > >>> should only return those where it is necessary to check in the test. > >>> But this is probably too much brains in this case 😊 > >>> > >>>> If you are adding "@requires vm.hasSAandCanAttach" to a test, > shouldn't > >>>> you remove the test's Platform.shouldSAAttach() check? > >>> Well I asked in my mail whether I should do that. So I take this as a yes. > >>> But it depends on whether it must be evaluated in the test. > >>> > >>>> Is there a reason not to take the more direct and established approach > >>>> of just adding the Platform.shouldSAAttach() check to TestJmapCore? > It > >>>> would be a lot less disruptive. I know the vm.hasSAandCanAttach > >>>> approach has advantages in not even attempting to compile and run > the > >>>> test, but I'm not so sure those advantages outweigh the simplicity of > >>>> just adding a Platform.shouldSAAttach() check to one test. I can go > >>>> either way here. Just wondering if there are additional reasons I might > >>>> not be seeing. > >>> I like the @requires much more. It tells right where the test is described > >>> that it does not run with SA. The other is quite hidden in the test, > >>> e.g., in > >>> helper class ClhsdbLauncher.java. > >>> Also, it's more easy to remember (The implementor of > >>> TestJmapCore forgot it...). And, last but not least, it seems best > >>> practice > >>> nowadays. There are lots of excludes for mac, they are also done by > >> @requires > >>> and not as a check using Platform at runtime. > >>> > >>>> Sorry, I don't have an answer to your VMProps evaluation question. > You > >>>> might need to ask a wider audience than just svc. > >>> Whom should I ask? Hotspot-dev? > >>> > >>> Best regards, > >>> Goetz. > >>> > >>> > >>>> > >>>> thanks, > >>>> > >>>> Chris > >>>> > >>>> On 6/20/18 6:49 AM, Lindenmaier, Goetz wrote: > >>>>> Hi, > >>>>> > >>>>> TestJmapCore is failing on aix because there jhsdb is not implemented. > >>>>> So far, such tests check at runtime Platform.shouldSAAttach() which is > >>>> missing > >>>>> in TestJmapCore. > >>>>> > >>>>> Instead, I implement @requires vm.hasSAandCanAttach. > >>>>> This makes jtreg skip the tests without compiling and starting them. > >>>>> > >>>>> I added the new property to all tests that check shouldSAAttach in jdk > >> and > >>>> hostpot test > >>>>> directories. > >>>>> This is the implementation of the new property, the rest is just > >> repetitive > >>>> checking it: > >>>>> http://cr.openjdk.java.net/~goetz/wr18/8205419- > >>>> requiresHasSA/01/test/jtreg-ext/requires/VMProps.java.udiff.html > >>>>> webrev: > >>>>> http://cr.openjdk.java.net/~goetz/wr18/8205419-requiresHasSA/01/ > >>>>> > >>>>> Is it correct to assume the VMProps is evaluated on the same machine > >> and > >>>> with the same > >>>>> user as used for executing the test? canPtraceAttachLinux directly > >>>> accesses the system. > >>>>> > >>>>> (Should I remove the checks for shouldSAAttach() from the changed > >> tests?) > >>>>> > >>>>> Best regards, > >>>>> Goetz. > >>>> > >>>> > >>>
