Hello, Josselin Poiret <[email protected]> skribis:
>> 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. Yeah, on second thought I think you’re right: it be can be useful to have it exposed to users. In fact, I think we should provide interfaces that make ‘primitive-fork’ unnecessary for most use cases. Exposing that procedure is a step in that direction. >> 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. Nice! >>> +++ 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. Right now ‘system*’ does: pid = scm_spawn_process (prog, args, in, out, err); SCM_SYSCALL (wait_result = waitpid (scm_to_int (pid), &status, 0)); if (wait_result == -1) SCM_SYSERROR; How about introducing decomposing ‘scm_spawn_process’ so that we have a lower-level function we could use (‘spawn_process’ below), roughly like so: ret = spawn_process (proc, args, in, out, err, &pid); if (ret != 0) { if (ret == ENOMEM) { errno = ENOMEM; SCM_SYSERROR; } else /* Emulate old-style failure. TODO: In 3.2, turn that into an exception */ status = 127 << 8; } else SCM_SYSCALL (wait_result = waitpid (scm_to_int (pid), &status, 0)); Does that make sense? It’s a bit of work to emulate that suboptimal behavior, but I think it’s important not to change that in 3.0. Thanks for your feedback! Ludo’.
