bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

Hi Ludo, 

Ludovic Courtès <[email protected]> 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