Hi Thomas,
I'd be fine with leaving it out. It only provides confirmation of the
library call,
nothing algorithmic or complex and there is no bug to verify.
So yes, drop it and save the test time and maintenance.
Thanks, Roger
On 02/09/2019 10:24 AM, Thomas Stüfe wrote:
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
<mailto: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
<http://cr.openjdk.java.net/%7Estuefe/webrevs/8213192--%28process%29-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 <mailto: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
<http://cr.openjdk.java.net/%7Estuefe/webrevs/8213192--%28process%29-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 <mailto: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
<http://cr.openjdk.java.net/%7Estuefe/webrevs/8213192--%28process%29-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
