Hi Roger, I start getting doubts about the whole test, honestly. Do you think it is worth the maintenance effort to test that we indeed call posix_spawn as we intended to? I wonder whether I should just scratch it.
..Thomas On Fri, Feb 8, 2019 at 10:00 PM Roger Riggs <roger.ri...@oracle.com> wrote: > Hi Thomas, > > The new OutputAnalyzer methods are new: > > 496: now that we have Immutable Lists, can the filterByKeyword argument be > a List<String>? > > List<String> interesting = List.of( "fork", "clone", "vfork", "exec", > "/bin/true", "jspawnhelper" ); > > 497: Instead of passing an int, pass the String itself > 480, 485: that is returned from getStdout() or getStderr(). > > The body of the method could probably be nicely written using the Stream > API. > (except for the line numbers). I'm not sure the line numbers are useful > in this case > based since the strace output is not shown anywhere or seen in the full. > > <string>.lines().filter()... > > Add the javadoc to the methods so its clear how to use them since this is > a public test framework. > > Regards, Roger > > > On 02/08/2019 04:38 AM, Thomas Stüfe wrote: > > Hi Roger, Martin, > > next version: > > > http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.02/webrev > > - did massage the comment in ProcessImpl.c > - made the test more resilient by scanning for the strace tool and by > silently ignoring all problems stemming from strace or the payload binary > not being there. The test now only fails if the forks were successully > executed but it does not seem to use posix-spawn. > - added output to the test by printing the "interesting" lines of the > strace output. Note that this filtering is not really sophisticated and > will show all thread related clone() calls as well: > > 614 [pid 12447] <... clone resumed> child_stack=0x7fe00c4baff0, > flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, > parent_tidptr=0x7fe00c4bb9d0, tls=0x7fe00c4bb700, > child_tidptr=0x7fe00c4bb9d0) = 12452 > 646 [pid 12447] clone(/usr/bin/strace: Process 12453 attached > 649 [pid 12447] <... clone resumed> child_stack=0x7fe00c3b9ff0, > flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, > parent_tidptr=0x7fe00c3ba9d0, tls=0x7fe00c3ba700, > child_tidptr=0x7fe00c3ba9d0) = 12453 > .... > > I am sure this could be made more intelligent but lets keep it simple for > now. > > - I removed the helperPath() methods Roger mentioned, they are not needed > anymore. > > @Martin: I like the -e signal=none -e trace=process idea but I'm not sure > if all versions of strace support these options. I think the strace output > is small enough for this small use case, some kB only. > > Cheers, Thomas > > > > > > On Thu, Feb 7, 2019 at 6:01 PM Thomas Stüfe <thomas.stu...@gmail.com> > wrote: > >> Hi all, >> >> second version, including the updated comment in ProcessImpl.c Martin >> requested: >> >> >> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.01/webrev/index.html >> >> @Roger: thanks for feeding this into your tests. I still try to get it to >> run thru jdk-submit, but that seems to be stuck again.. >> >> Cheers, Thomas >> >> >> >> >> >> On Wed, Feb 6, 2019 at 10:29 AM Thomas Stüfe <thomas.stu...@gmail.com> >> wrote: >> >>> Hi all >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213192 >>> webrev: >>> http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.00/webrev/index.html >>> >>> (@Roger: I hope you do not mind? The bug is assigned to you but since I >>> happened to play around with posix_spawn I prepared this webrev. If you >>> rather do this change, that is fine and I will leave it to you.) >>> >>> When we added the possibility to use posix_spawn as underlying >>> implementation for Runtime.exec() on Linux with >>> https://bugs.openjdk.java.net/browse/JDK-8212828, we agreed to keep >>> VFORK as default until work on 13 starts. So now would be a good time to >>> switch the default to posix_spawn to get a good testing window. Note that >>> at SAP we run our VMs internally with posix_spawn as default since some >>> months and have not seen problems. >>> >>> As for the fix, I added a test which tests that the default is indeed >>> posix_spawn - not sure whether this is overdoing it though. Also, I use >>> strace for the test, and /bin/true, and while strace is usually available >>> and reachable by path resolution, I am afraid on some test machines it may >>> not. What do you think, should I leave the test out? >>> >>> The fix ran through all java/lang/ProcessBuilder jtreg tests ok. >>> >>> Thanks, Thomas >>> >>> >
