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
