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

Hi Josselin,

Sorry for taking so long to come back to you.  I think this is great
work!  I pushed it as ‘wip-posix-spawn’ so others can give it a try.

Josselin Poiret <[email protected]> skribis:

> * libguile/posix.c (renumber_file_descriptor, start_child,
> scm_piped_process): Remove functions.
> (scm_port_to_fd_with_default): New helper function.
> (scm_system_star): Rewrite using scm_spawn_process.
> (scm_init_popen): Remove the definition of piped-process.
> (scm_init_posix): Now make popen available unconditionally.
>
> * module/ice-9/popen.scm (port-with-defaults): New helper procedure.
> (spawn): New procedure.
> (open-process): Rewrite using spawn.
> (pipeline): Rewrite using spawn*.
>
> * test-suite/tests/popen.test ("piped-process", "piped-process:
> with-output"): Removed tests.
> ("spawn", "spawn: with output"): Added tests.
> * test-suite/tests/posix.test ("http://bugs.gnu.org/13166", "exit code
> for nonexistent file", "https://bugs.gnu.org/55596"): Remove obsolete
> tests.
> ("exception for nonexistent file"): Add test.
> ---
>  libguile/posix.c            | 218 +++---------------------------------
>  module/ice-9/popen.scm      |  83 ++++++++++----
>  test-suite/tests/popen.test |  14 +--
>  test-suite/tests/posix.test |  36 +++---
>  4 files changed, 102 insertions(+), 249 deletions(-)

More deletions than insertions. 👍

That scary-looking Gnulib update seems to have worked well.

I have mostly cosmetic/polishing comments and one issue with ‘system*’.
I can actually do that on your behalf if you’re unavailable these days;
let me know what you prefer.

>  static SCM
>  scm_spawn_process (SCM prog, SCM args, SCM scm_in, SCM scm_out, SCM scm_err)
>  #define FUNC_NAME "spawn*"

For top-level functions, please add a comment above explaining what it
does.

I would call this one ‘primitive-spawn’ rather than ‘spawn*’ and keep it
private to (ice-9 popen).

> +(define* (spawn exec-file argv #:key
> +                             (in (current-input-port))
> +                             (out (current-output-port))
> +                             (err (current-error-port)))

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.

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.

> +++ 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?

> -  (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’?

> -  (pass-if-equal "https://bugs.gnu.org/55596"
> -      127
> -    ;; The parameterization below used to cause 'start_child' to close
> -    ;; fd 2 in the child process, which in turn would cause it to
> -    ;; segfault, leading to a wrong exit code.
> -    (parameterize ((current-output-port (current-error-port)))
> -      (status:exit-val (system* "something-that-does-not-exist")))))

Likewise we should keep this one.

> +  (pass-if-equal "exception for nonexistent file"
> +      2 ; ENOENT
> +      (call-with-prompt 'escape
> +        (lambda ()
> +          (with-exception-handler
> +              (lambda (exn)
> +                (let* ((kind (exception-kind exn))
> +                       (errno (and (eq? kind 'system-error)
> +                                   (car (car
> +                                         (cdr (cdr (cdr (exception-args
> +                                                        exn)))))))))
> +                  (abort-to-prompt 'escape errno)))
> +            (lambda ()
> +              (status:exit-val (system*
> +                                "something-that-does-not-exist")))
> +            #:unwind? #t))
> +        (lambda (k arg)
> +          arg))))

We’ll have to leave this change for the next major series of Guile.

Thanks!

Ludo’.