Hi Ludo, Ludovic Courtès writes: > For top-level functions, please add a comment above explaining what it > does. Completely forgot about that. I will do it at some point, however we will need to settle on how to resolve the issue at the bottom first. > I would call this one ‘primitive-spawn’ rather than ‘spawn*’ and keep it > private to (ice-9 popen). Is there any reason we would want this to not be accessible to the user? There are already a bunch of functions that manipulate raw fdes, and people might want to directly use this to not have to care about ports. > Please add a docstring. It may also be worth documenting it in the > manual given that it’s public. > >> + (let* ((in (port-with-defaults in "r")) >> + (out (port-with-defaults out "w")) >> + (err (port-with-defaults err "w")) > > I’d make it “r0” and “w0” since we’re doing to throw the ports away > right after. Sure. > We could even avoid allocating a port when we’re going to use /dev/null > (and thus go with ‘open-fdes’ directly), but that’s a micro-optimization > we can keep for later. Right. I chose to keep the code simple for now, it's too much trouble having to choose between using ports and fdes. Note that I did a small benchmark and system* with PATCH v5 is 3x faster than on 3.0.8. vfork is working wonders. >> +++ b/test-suite/tests/posix.test >> @@ -236,24 +236,24 @@ >> >> (with-test-prefix "system*" >> >> - (pass-if "http://bugs.gnu.org/13166" >> - ;; With Guile up to 2.0.7 included, the child process launched by >> - ;; `system*' would remain alive after an `execvp' failure. >> - (let ((me (getpid))) >> - (and (not (zero? (system* "something-that-does-not-exist"))) >> - (= me (getpid))))) > > I’d keep this one, I guess it doesn’t hurt? As is, it doesn't work since system* would throw a system exception because spawn is able to catch that error. Previously, the child would fail its execvp and die with exit code 127, which system* would return. >> - (pass-if-equal "exit code for nonexistent file" >> - 127 ;aka. EX_NOTFOUND >> - (status:exit-val (system* "something-that-does-not-exist"))) > > It’s good that we have better error reporting thanks to ‘posix_spawn’. > > However, I don’t think we can change that in 3.0. What about, for 3.0, > adding a layer around ‘spawn’ so that ‘system*’ still returns 127 when > ‘spawn’ throws to ‘system-error’? So I've been working on something that would do this, but I noticed that we have no way to be strictly backwards-compatible: if there's an error like ENOFILE, we can't get a pid from posix_spawn, and so piped-process won't have anything to return, whereas before it would return the pid of the failing child. I'm not sure there's a way to emulate that, unless we just fork a child that instantly returns 127. Doesn't seem great though. WDYT? -- Josselin Poiret