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

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’.