From: Josselin Poiret via "Bug reports for GUILE, GNU's Ubiquitous Extension Language" <bug-guile@gnu.org>
To: "Ludovic Courtès" <ludo@gnu.org>
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: Sun, 11 Dec 2022 21:16:34 +0100 [thread overview]
Message-ID: <87sfhl8zml.fsf@jpoiret.xyz> (raw)
In-Reply-To: <875yex4x9a.fsf_-_@gnu.org>
Hi Ludo,
Ludovic Courtès <ludo@gnu.org> 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
next prev parent reply other threads:[~2022-12-11 20:16 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 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Ludovic Courtès
2022-12-11 20:16 ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language [this message]
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=87sfhl8zml.fsf@jpoiret.xyz \
--to=bug-guile@gnu.org \
--cc=52835@debbugs.gnu.org \
--cc=dev@jpoiret.xyz \
--cc=ludo@gnu.org \
--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).