Thank you Roger! On Mon, Feb 11, 2019 at 7:55 PM Roger Riggs <roger.ri...@oracle.com> wrote:
> Great! Looks fine. > > Thanks, Roger > > On 02/11/2019 01:50 PM, Thomas Stüfe wrote: > > Hi Roger, Martin, > > hopefully final version: > > > http://cr.openjdk.java.net/~stuefe/webrevs/8213192--(process)-change-the-process-launch-mechanism-default-on-linux-to-be-posix_spawn/webrev.03/webrev/ > > I removed the test and the changes in the test library made for the test. > Test is just too brittle with too little nourishing value. Everything else > is unchanged from webrev.02. > > Thank you, Thomas > > > > On Fri, Feb 8, 2019 at 10:38 AM Thomas Stüfe <thomas.stu...@gmail.com> > 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 >>>> >>>> >