From: "Ludovic Courtès" <ludo@gnu.org>
To: Josselin Poiret <dev@jpoiret.xyz>
Cc: 52835@debbugs.gnu.org, Timothy Sample <samplet@ngyro.com>
Subject: bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
Date: Tue, 29 Nov 2022 16:05:05 +0100 [thread overview]
Message-ID: <875yex4x9a.fsf_-_@gnu.org> (raw)
In-Reply-To: <1d64d8e0e292fc3a89bcd491dd8f10171cb7c804.1662358976.git.dev@jpoiret.xyz> (Josselin Poiret's message of "Mon, 5 Sep 2022 08:48:15 +0200")
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 <dev@jpoiret.xyz> 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’.
next prev parent reply other threads:[~2022-11-29 15:05 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-27 21:25 bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2021-12-27 21:35 ` bug#52835: [PATCH 1/2] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2021-12-27 21:35 ` bug#52835: [PATCH 2/2] Remove unused renumber_file_descriptor Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2021-12-27 21:49 ` bug#52835: [PATCH v2 " Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2021-12-28 15:40 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Timothy Sample
2021-12-28 17:25 ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-02-07 16:55 ` bug#52835: [PATCH v3] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46 ` bug#52835: [PATCH v4 0/4] Improve safety of start_child and piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46 ` bug#52835: [PATCH v4 1/4] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46 ` bug#52835: [PATCH v4 2/4] Avoid double closes in piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46 ` bug#52835: [PATCH v4 3/4] Remove useless closing code in start_child Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46 ` bug#52835: [PATCH v4 4/4] Make start_child propagate the child errno to the parent Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-09-05 6:48 ` bug#52835: [PATCH v5 0/3] Move spawning procedures to posix_spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-09-05 6:48 ` bug#52835: [PATCH v5 1/3] Update gnulib to 0.1.5414-8204d and add posix_spawn, posix_spawnp Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-09-05 6:48 ` bug#52835: [PATCH v5 2/3] Add spawn* Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-09-05 6:48 ` bug#52835: [PATCH v5 3/3] Move popen and posix procedures to spawn* Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-11-29 15:05 ` Ludovic Courtès [this message]
2022-12-11 20:16 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-12 23:49 ` Ludovic Courtès
2022-12-22 12:49 ` bug#52835: [PATCH v6 0/3] Move spawning procedures to posix_spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-22 12:49 ` bug#52835: [PATCH v6 1/3] Add spawn* Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-22 12:49 ` bug#52835: [PATCH v6 2/3] Make system* and piped-process internally use spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-22 12:49 ` bug#52835: [PATCH v6 3/3] Move popen and posix procedures to spawn* Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-23 10:53 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Ludovic Courtès
2022-12-23 17:15 ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-23 17:17 ` bug#52835: [PATCH v7 1/2] Add spawn* and spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-23 17:17 ` bug#52835: [PATCH v7 2/2] Make system* and piped-process internally use spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-25 17:04 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Ludovic Courtès
2022-12-25 17:03 ` Ludovic Courtès
2022-12-25 16:58 ` Ludovic Courtès
2023-01-07 16:07 ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2023-01-07 16:07 ` bug#52835: [PATCH v8 1/2] Add spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2023-01-07 16:07 ` bug#52835: [PATCH v8 2/2] Make system* and piped-process internally use spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2023-01-12 22:02 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Ludovic Courtès
2023-01-13 1:11 ` Andrew Whatson via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2023-01-13 15:20 ` Ludovic Courtès
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/guile/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875yex4x9a.fsf_-_@gnu.org \
--to=ludo@gnu.org \
--cc=52835@debbugs.gnu.org \
--cc=dev@jpoiret.xyz \
--cc=samplet@ngyro.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).