[PATCH 0/2] Fix spawning a child not setting standard fds properly

  • Done
  • quality assurance status badge
Details
4 participants
  • Josselin Poiret
  • Ludovic Courtès
  • Timothy Sample
  • Andrew Whatson
Owner
unassigned
Submitted by
Josselin Poiret
Severity
normal
J
J
Josselin Poiret wrote on 27 Dec 2021 22:25
(address . bug-guile@gnu.org)(name . Josselin Poiret)(address . dev@jpoiret.xyz)
cover.1640638819.git.dev@jpoiret.xyz
Hello,

While working on the Guix installer (that needs to use system* quite a
lot), I've noticed that output/error redirection doesn't behave as
intended when combined with system*. Here's a test you can try
at home:
Toggle snippet (6 lines)
(call-with-output-file "/tmp/test.log"
(lambda (port) (with-output-to-port port
(lambda () (with-error-to-port port
(lambda () (system* "bash" "-c" "echo bong >&2")))))))

With current Guix, you will notice that /tmp/test.log is empty,
instead of the expected "bong".

Worse even, when testing with
Toggle snippet (2 lines)
(with-error-to-port (current-output-port) (lambda () (system* "bash" "-c" "echo $$; sleep 10")))
you can actually inspect `/proc/<PID>/fd/` and see that the stderr fd,
2, is actually closed. This means that the next opened fd will take
its place, to which writes to stderr may end up.

The logic behind the stdin/out/err redirection for child processes
lies in `start_child`, in libguile/posix.c, and doesn't take into
account cases like:
* in/out/err having common values, as the common fd will be closed
before it has been dup2'd to all the std fds (which happens in the
first example);
* in/out/err having values between 0 and 2 which aren't their
corresponding std fd number, as they will not be dup2'd to
the stdin/out/err (which happens in the second example).

The first patch addresses this by:
* moving in/out/err closing logic after they've all been dup2'd;
* removing the check that in/out/err are > the corresponding
stdin/out/err;
* replacing renumber_file_descriptor by simply dup, as the former
closes fds that might be shared. The closing logic of the first point
is enough here.

The second patch removes renumber_file_descriptor, as it is no longer
used.

Best,
Josselin

Josselin Poiret (2):
Fix child spawning closing standard fds prematurely
Remove unused renumber_file_descriptor

libguile/posix.c | 72 ++++++++++++++----------------------------------
1 file changed, 20 insertions(+), 52 deletions(-)

--
2.34.0
J
J
Josselin Poiret wrote on 27 Dec 2021 22:35
[PATCH 1/2] Fix child spawning closing standard fds prematurely
(name . Josselin Poiret)(address . dev@jpoiret.xyz)(address . 52835@debbugs.gnu.org)
299d40c7e9fc0c30c3aebfa06c69506b981bd1f8.1640638819.git.dev@jpoiret.xyz
* libguile/posix.c (start_child): Close standard file descriptors only
after all of them have been dup2'd.
---
libguile/posix.c | 40 +++++++++++++++++++---------------------
1 file changed, 19 insertions(+), 21 deletions(-)

Toggle diff (53 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 3ab12b99e..148ebeb3d 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1357,27 +1357,25 @@ start_child (const char *exec_file, char **exec_argv,
if (err == -1)
err = open ("/dev/null", O_WRONLY);
- if (in > 0)
- {
- if (out == 0)
- out = renumber_file_descriptor (out, err);
- if (err == 0)
- err = renumber_file_descriptor (err, err);
- do dup2 (in, 0); while (errno == EINTR);
- close (in);
- }
- if (out > 1)
- {
- if (err == 1)
- err = renumber_file_descriptor (err, err);
- do dup2 (out, 1); while (errno == EINTR);
- close (out);
- }
- if (err > 2)
- {
- do dup2 (err, 2); while (errno == EINTR);
- close (err);
- }
+ /* Dup each non-yet-dup2'd fd that's in the way to the next available fd,
+ so that we can safely dup2 to 0/1/2 without potentially overwriting
+ in/out/err. Note that dup2 doesn't do anything if its arguments are
+ equal. */
+ if (out == 0)
+ do out = dup (out); while (errno == EINTR);
+ if (err == 0)
+ do err = dup (err); while (errno == EINTR);
+ do dup2 (in, 0); while (errno == EINTR);
+
+ if (err == 1)
+ do err = dup (err); while (errno == EINTR);
+ do dup2 (out, 1); while (errno == EINTR);
+
+ do dup2 (err, 2); while (errno == EINTR);
+
+ if (in > 2) close (in);
+ if (out > 2) close (out);
+ if (err > 2) close (err);
execvp (exec_file, exec_argv);
--
2.34.0
J
J
Josselin Poiret wrote on 27 Dec 2021 22:35
[PATCH 2/2] Remove unused renumber_file_descriptor
(name . Josselin Poiret)(address . dev@jpoiret.xyz)(address . 52835@debbugs.gnu.org)
552de70f16142c8a42cde0a449549eac42e48485.1640638819.git.dev@jpoiret.xyz
* libguile/posix.c (renumber_file_descriptor): Remove it.
---
libguile/posix.c | 32 +-------------------------------
1 file changed, 1 insertion(+), 31 deletions(-)

Toggle diff (45 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 148ebeb3d..2624d07c2 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1277,37 +1277,7 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
return scm_from_int (pid);
}
#undef FUNC_NAME
-#endif /* HAVE_FORK */
-
-#ifdef HAVE_FORK
-/* 'renumber_file_descriptor' is a helper function for 'start_child'
- below, and is specialized for that particular environment where it
- doesn't make sense to report errors via exceptions. It uses dup(2)
- to duplicate the file descriptor FD, closes the original FD, and
- returns the new descriptor. If dup(2) fails, print an error message
- to ERR and abort. */
-static int
-renumber_file_descriptor (int fd, int err)
-{
- int new_fd;
-
- do
- new_fd = dup (fd);
- while (new_fd == -1 && errno == EINTR);
-
- if (new_fd == -1)
- {
- /* At this point we are in the child process before exec. We
- cannot safely raise an exception in this environment. */
- const char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
- _exit (127); /* Use exit status 127, as with other exec errors. */
- }
-
- close (fd);
- return new_fd;
-}
-#endif /* HAVE_FORK */
+#endif /* HAVE_FORK *
#ifdef HAVE_FORK
#define HAVE_START_CHILD 1
--
2.34.0
J
J
Josselin Poiret wrote on 27 Dec 2021 22:49
[PATCH v2 2/2] Remove unused renumber_file_descriptor
(name . Josselin Poiret)(address . dev@jpoiret.xyz)(address . 52835@debbugs.gnu.org)
20211227214959.15324-1-dev@jpoiret.xyz
* libguile/posix.c (renumber_file_descriptor): Remove it.
---
Sorry for the noise, but I just saw that this patch omitted a closing
*/, here is a fixed version.

libguile/posix.c | 30 ------------------------------
1 file changed, 30 deletions(-)

Toggle diff (43 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 148ebeb3d..1d9996565 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1279,36 +1279,6 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#undef FUNC_NAME
#endif /* HAVE_FORK */
-#ifdef HAVE_FORK
-/* 'renumber_file_descriptor' is a helper function for 'start_child'
- below, and is specialized for that particular environment where it
- doesn't make sense to report errors via exceptions. It uses dup(2)
- to duplicate the file descriptor FD, closes the original FD, and
- returns the new descriptor. If dup(2) fails, print an error message
- to ERR and abort. */
-static int
-renumber_file_descriptor (int fd, int err)
-{
- int new_fd;
-
- do
- new_fd = dup (fd);
- while (new_fd == -1 && errno == EINTR);
-
- if (new_fd == -1)
- {
- /* At this point we are in the child process before exec. We
- cannot safely raise an exception in this environment. */
- const char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
- _exit (127); /* Use exit status 127, as with other exec errors. */
- }
-
- close (fd);
- return new_fd;
-}
-#endif /* HAVE_FORK */
-
#ifdef HAVE_FORK
#define HAVE_START_CHILD 1
/* Since Guile uses threads, we have to be very careful to avoid calling
--
2.34.0
T
T
Timothy Sample wrote on 28 Dec 2021 16:40
Re: bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
(name . Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language)(address . bug-guile@gnu.org)
8735mcu4a5.fsf@ngyro.com
Hey Josselin,

Thanks for finding this bug! I have one concern about your patches:

Josselin Poiret writes:

Toggle quote (3 lines)
> The second patch removes renumber_file_descriptor, as it is no longer
> used.

One thing that ‘renumber_file_descriptor’ does that we seem to be losing
here is error checking. To my eye, the old code will try and warn the
user if they run out of file descriptors, but the new code will not.

The other thing that I like is how ‘renumber_file_descriptor’ checks the
return value of ‘dup’ in addition to checking ‘errno’. (I realize that
the old code skips that check for ‘dup2’ – I’m kinda just stating a
preference here.) A quick check of POSIX turns up the following: “the
value of ‘errno’ should only be examined when it is indicated to be
valid by a function’s return value” [1].


-- Tim

J
J
Josselin Poiret wrote on 28 Dec 2021 18:25
(name . Timothy Sample)(address . samplet@ngyro.com)(address . 52835@debbugs.gnu.org)
87lf04fxqo.fsf@jpoiret.xyz
Hello Timothy,

Timothy Sample <samplet@ngyro.com> writes:

Toggle quote (4 lines)
> One thing that ‘renumber_file_descriptor’ does that we seem to be losing
> here is error checking. To my eye, the old code will try and warn the
> user if they run out of file descriptors, but the new code will not.

I may have been too hasty on that front, you're right that we should
check for errors when dup'ing. Note though that we should not run out
of fds, as we close all of them except for in/out/err right before using
it, but still, here are the interesting error codes:
* EINTR, although OpenBSD and Linux will never set that error from my
light research;
* EBUSY, not POSIX though, only Linux, so should we?
* EMFILE, if we run out of fds;
* EBADF if either of the fds are invalid.

As for error reporting, should we keep the parent's stderr fd open while
we do all of this, just to be able to report an error in case it
happens? The old code used to try reporting the errors to the new `err`
fd instead, but it might be clearer to use stderr.

WDYT?

--
Josselin Poiret
J
J
Josselin Poiret wrote on 7 Feb 2022 17:55
[PATCH v3] Fix child spawning closing standard fds prematurely
(address . 52835@debbugs.gnu.org)
20220207165543.12723-1-dev@jpoiret.xyz
* libguile/posix.c (renumber_file_descriptor): Refactor it as
dup_handle_error.
(dup_handle_error, dup2_handle_error): New functions that wrap around
dup and dup2 by retrying on EINTR or EBUSY, as well as erroring out on
other errors.
(start_child): Close standard file descriptors only
after all of them have been dup2'd.
---
Hello,

This is a new version of the fix that should now handle possible
dup/dup2 errors properly.

Best,
Josselin
libguile/posix.c | 82 +++++++++++++++++++++++++++++++-----------------
1 file changed, 53 insertions(+), 29 deletions(-)

Toggle diff (110 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 3ab12b99e..dc3080b3c 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1280,14 +1280,14 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#endif /* HAVE_FORK */
#ifdef HAVE_FORK
-/* 'renumber_file_descriptor' is a helper function for 'start_child'
- below, and is specialized for that particular environment where it
- doesn't make sense to report errors via exceptions. It uses dup(2)
- to duplicate the file descriptor FD, closes the original FD, and
- returns the new descriptor. If dup(2) fails, print an error message
- to ERR and abort. */
+/* 'dup_handle_error' is a helper function for 'start_child' below, and
+ is specialized for that particular environment where it doesn't make
+ sense to report errors via exceptions. It uses dup(2) to duplicate
+ the file descriptor FD, does *not* close the original FD, and returns
+ the new descriptor. If dup(2) fails, print an error message to ERR
+ and abort. */
static int
-renumber_file_descriptor (int fd, int err)
+dup_handle_error (int fd, int err)
{
int new_fd;
@@ -1304,7 +1304,33 @@ renumber_file_descriptor (int fd, int err)
_exit (127); /* Use exit status 127, as with other exec errors. */
}
- close (fd);
+ return new_fd;
+}
+
+/* 'dup2_handle_error' is a helper function for 'start_child' below, and
+ is specialized for that particular environment where it doesn't make
+ sense to report errors via exceptions. It uses dup2(2) to duplicate
+ the file descriptor FD, does *not* close the original FD, and returns
+ the new descriptor. If dup2(2) fails, print an error message to ERR
+ and abort. */
+static int
+dup2_handle_error (int fd, int to, int err)
+{
+ int new_fd;
+
+ do
+ new_fd = dup2 (fd, to);
+ while (new_fd == -1 && (errno == EINTR || errno == EBUSY));
+
+ if (new_fd == -1)
+ {
+ /* At this point we are in the child process before exec. We
+ cannot safely raise an exception in this environment. */
+ const char *msg = strerror (errno);
+ fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
+ _exit (127); /* Use exit status 127, as with other exec errors. */
+ }
+
return new_fd;
}
#endif /* HAVE_FORK */
@@ -1357,27 +1383,25 @@ start_child (const char *exec_file, char **exec_argv,
if (err == -1)
err = open ("/dev/null", O_WRONLY);
- if (in > 0)
- {
- if (out == 0)
- out = renumber_file_descriptor (out, err);
- if (err == 0)
- err = renumber_file_descriptor (err, err);
- do dup2 (in, 0); while (errno == EINTR);
- close (in);
- }
- if (out > 1)
- {
- if (err == 1)
- err = renumber_file_descriptor (err, err);
- do dup2 (out, 1); while (errno == EINTR);
- close (out);
- }
- if (err > 2)
- {
- do dup2 (err, 2); while (errno == EINTR);
- close (err);
- }
+ /* Dup each non-yet-dup2'd fd that's in the way to the next available fd,
+ so that we can safely dup2 to 0/1/2 without potentially overwriting
+ in/out/err. Note that dup2 doesn't do anything if its arguments are
+ equal. */
+ if (out == 0)
+ out = dup_handle_error (out, err);
+ if (err == 0)
+ err = dup_handle_error (err, err);
+ dup2_handle_error (in, 0, err);
+
+ if (err == 1)
+ err = dup_handle_error (err, err);
+ dup2_handle_error (out, 1, err);
+
+ dup2_handle_error (err, 2, err);
+
+ if (in > 2) close (in);
+ if (out > 2) close (out);
+ if (err > 2) close (err);
execvp (exec_file, exec_argv);
--
2.34.0
J
J
Josselin Poiret wrote on 28 May 2022 14:46
[PATCH v4 1/4] Fix child spawning closing standard fds prematurely.
(address . 52835@debbugs.gnu.org)
20220528124634.17353-2-dev@jpoiret.xyz
* libguile/posix.c (renumber_file_descriptor): Refactor it as
dup_handle_error.
(dup_handle_error, dup2_handle_error): New functions that wrap around
dup and dup2 by retrying on EINTR or EBUSY, as well as erroring out on
other errors.
(start_child): Close standard file descriptors only after all of them
have been dup2'd.
---
libguile/posix.c | 84 +++++++++++++++++++++++++++++++-----------------
1 file changed, 54 insertions(+), 30 deletions(-)

Toggle diff (118 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 3ab12b99e..e9f49fa27 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1280,14 +1280,14 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#endif /* HAVE_FORK */
#ifdef HAVE_FORK
-/* 'renumber_file_descriptor' is a helper function for 'start_child'
- below, and is specialized for that particular environment where it
- doesn't make sense to report errors via exceptions. It uses dup(2)
- to duplicate the file descriptor FD, closes the original FD, and
- returns the new descriptor. If dup(2) fails, print an error message
- to ERR and abort. */
+/* 'dup_handle_error' is a helper function for 'start_child' below, and
+ is specialized for that particular environment where it doesn't make
+ sense to report errors via exceptions. It uses dup(2) to duplicate
+ the file descriptor FD, does *not* close the original FD, and returns
+ the new descriptor. If dup(2) fails, print an error message to ERR
+ and abort. */
static int
-renumber_file_descriptor (int fd, int err)
+dup_handle_error (int fd, int err)
{
int new_fd;
@@ -1304,7 +1304,33 @@ renumber_file_descriptor (int fd, int err)
_exit (127); /* Use exit status 127, as with other exec errors. */
}
- close (fd);
+ return new_fd;
+}
+
+/* 'dup2_handle_error' is a helper function for 'start_child' below, and
+ is specialized for that particular environment where it doesn't make
+ sense to report errors via exceptions. It uses dup2(2) to duplicate
+ the file descriptor FD, does *not* close the original FD, and returns
+ the new descriptor. If dup2(2) fails, print an error message to ERR
+ and abort. */
+static int
+dup2_handle_error (int fd, int to, int err)
+{
+ int new_fd;
+
+ do
+ new_fd = dup2 (fd, to);
+ while (new_fd == -1 && (errno == EINTR || errno == EBUSY));
+
+ if (new_fd == -1)
+ {
+ /* At this point we are in the child process before exec. We
+ cannot safely raise an exception in this environment. */
+ const char *msg = strerror (errno);
+ fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
+ _exit (127); /* Use exit status 127, as with other exec errors. */
+ }
+
return new_fd;
}
#endif /* HAVE_FORK */
@@ -1357,34 +1383,32 @@ start_child (const char *exec_file, char **exec_argv,
if (err == -1)
err = open ("/dev/null", O_WRONLY);
- if (in > 0)
- {
- if (out == 0)
- out = renumber_file_descriptor (out, err);
- if (err == 0)
- err = renumber_file_descriptor (err, err);
- do dup2 (in, 0); while (errno == EINTR);
- close (in);
- }
- if (out > 1)
- {
- if (err == 1)
- err = renumber_file_descriptor (err, err);
- do dup2 (out, 1); while (errno == EINTR);
- close (out);
- }
- if (err > 2)
- {
- do dup2 (err, 2); while (errno == EINTR);
- close (err);
- }
+ /* Dup each non-yet-dup2'd fd that's in the way to the next available fd,
+ so that we can safely dup2 to 0/1/2 without potentially overwriting
+ in/out/err. Note that dup2 doesn't do anything if its arguments are
+ equal. */
+ if (out == 0)
+ out = dup_handle_error (out, err);
+ if (err == 0)
+ err = dup_handle_error (err, err);
+ dup2_handle_error (in, 0, err);
+
+ if (err == 1)
+ err = dup_handle_error (err, err);
+ dup2_handle_error (out, 1, err);
+
+ dup2_handle_error (err, 2, err);
+
+ if (in > 2) close (in);
+ if (out > 2) close (out);
+ if (err > 2) close (err);
execvp (exec_file, exec_argv);
/* The exec failed! There is nothing sensible to do. */
{
const char *msg = strerror (errno);
- fprintf (fdopen (2, "a"), "In execvp of %s: %s\n",
+ dprintf (2, "In execvp of %s: %s\n",
exec_file, msg);
}
--
2.36.0
J
J
Josselin Poiret wrote on 28 May 2022 14:46
[PATCH v4 2/4] Avoid double closes in piped-process.
(address . 52835@debbugs.gnu.org)
20220528124634.17353-3-dev@jpoiret.xyz
* libguile/posix.c (scm_piped_process): Avoid double closes.
---
libguile/posix.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)

Toggle diff (37 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index e9f49fa27..155ad09b7 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1475,12 +1475,18 @@ scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
if (reading)
{
close (c2p[0]);
- close (c2p[1]);
+ if (c2p[1] != c2p[0])
+ close (c2p[1]);
}
if (writing)
{
- close (p2c[0]);
- close (p2c[1]);
+ if (!(reading && (c2p[0] == p2c[0] ||
+ c2p[1] == p2c[0])))
+ close (p2c[0]);
+ if (p2c[0] != p2c[1] &&
+ !(reading && (c2p[0] == p2c[1] ||
+ c2p[1] == p2c[1])))
+ close (p2c[1]);
}
errno = errno_save;
SCM_SYSERROR;
@@ -1488,7 +1494,7 @@ scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
if (reading)
close (c2p[1]);
- if (writing)
+ if (writing && !(reading && c2p[1] == p2c[0]))
close (p2c[0]);
return scm_from_int (pid);
--
2.36.0
J
J
Josselin Poiret wrote on 28 May 2022 14:46
[PATCH v4 3/4] Remove useless closing code in start_child.
(address . 52835@debbugs.gnu.org)
20220528124634.17353-4-dev@jpoiret.xyz
* libguile/posix.c (start_child): We're closing all fds anyway, no need
to try to close some specific ones beforehand.
---
libguile/posix.c | 6 ------
1 file changed, 6 deletions(-)

Toggle diff (19 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 155ad09b7..94a043cca 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1363,12 +1363,6 @@ start_child (const char *exec_file, char **exec_argv,
child. Return directly in either case. */
return pid;
- /* The child. */
- if (reading)
- close (c2p[0]);
- if (writing)
- close (p2c[1]);
-
/* Close all file descriptors in ports inherited from the parent
except for in, out, and err. Heavy-handed, but robust. */
while (max_fd--)
--
2.36.0
J
J
Josselin Poiret wrote on 28 May 2022 14:46
[PATCH v4 0/4] Improve safety of start_child and piped-process.
(address . 52835@debbugs.gnu.org)
20220528124634.17353-1-dev@jpoiret.xyz
retitle 52835 Improve safety of start_child and piped-process.
thanks

Hello everyone,

This time, it's another Guix bug [1] that prompted me to have a closer
look at piped-process and start_child, which don't seem to be very
multi-thread safe. I've ended up with a couple of improvements that
IMO would make all procedures relying on them more robust. Here's
roughly what I did:

* Fix the fd closing code that was bogus for unusual values for in,
out, err for start_child.
* Check for double closes and avoid them, so that we don't
accidentally close an fd that another thread could have opened.
* Remove some closing code in the child, since we're already
generically closing all fds.
* Add a pipe from the child to the parent that the former uses to
report its errno to the latter. This avoids the use of strerror and
printf in the child after forking, since they are not async-signal
safe. As a side effect, this lets piped-error raise the proper
system exception for the child errno, instead of returning the PID
of a process that hasn't exec'd successfully.


Best,
Josselin Poiret (4):
Fix child spawning closing standard fds prematurely.
Avoid double closes in piped-process.
Remove useless closing code in start_child.
Make start_child propagate the child errno to the parent.

configure.ac | 3 +-
libguile/posix.c | 187 ++++++++++++++++++++++++++++++++++-------------
2 files changed, 138 insertions(+), 52 deletions(-)

--
2.36.0
J
J
Josselin Poiret wrote on 28 May 2022 14:46
[PATCH v4 4/4] Make start_child propagate the child errno to the parent.
(address . 52835@debbugs.gnu.org)
20220528124634.17353-5-dev@jpoiret.xyz
* configure.ac: Add AC_CHECK_FUNCS for pipe2.
* libguile/posix.c (start_child): Use a pipe to transmit the child's
errno to the parent, which can then use it to signal an error instead of
writing to its error file descriptor. This also avoids the use of
async-signal unsafe functions inside the child before exec. Use pipe2
when available.
(dup_handle_error,dup2_handle_error): Ditto.
---
configure.ac | 3 +-
libguile/posix.c | 113 ++++++++++++++++++++++++++++++++++++-----------
2 files changed, 89 insertions(+), 27 deletions(-)

Toggle diff (206 lines)
diff --git a/configure.ac b/configure.ac
index 827e1c09d..19441a52e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -525,6 +525,7 @@ AC_CHECK_HEADERS([assert.h crt_externs.h])
# fork - unavailable on Windows
# sched_getaffinity, sched_setaffinity - GNU extensions (glibc)
# sendfile - non-POSIX, found in glibc
+# pipe2 - non-POSIX, found on Linux
#
AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid \
fesetround ftime ftruncate fchown fchmod getcwd geteuid getsid \
@@ -536,7 +537,7 @@ AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid \
getgrent kill getppid getpgrp fork setitimer getitimer strchr strcmp \
index bcopy memcpy rindex truncate isblank _NSGetEnviron \
strcoll strcoll_l strtod_l strtol_l newlocale uselocale utimensat \
- sched_getaffinity sched_setaffinity sendfile])
+ sched_getaffinity sched_setaffinity sendfile pipe2])
# The newlib C library uses _NL_ prefixed locale langinfo constants.
AC_CHECK_DECLS([_NL_NUMERIC_GROUPING], [], [], [[#include <langinfo.h>]])
diff --git a/libguile/posix.c b/libguile/posix.c
index 94a043cca..79f097756 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1284,8 +1284,7 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
is specialized for that particular environment where it doesn't make
sense to report errors via exceptions. It uses dup(2) to duplicate
the file descriptor FD, does *not* close the original FD, and returns
- the new descriptor. If dup(2) fails, print an error message to ERR
- and abort. */
+ the new descriptor. If dup(2) fails, send errno to ERR and abort. */
static int
dup_handle_error (int fd, int err)
{
@@ -1299,9 +1298,12 @@ dup_handle_error (int fd, int err)
{
/* At this point we are in the child process before exec. We
cannot safely raise an exception in this environment. */
- const char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
- _exit (127); /* Use exit status 127, as with other exec errors. */
+ int errno_save = errno;
+ while (write (err, &errno_save, sizeof (errno)) == -1)
+ if (errno != EINTR)
+ break;
+
+ _exit (127);
}
return new_fd;
@@ -1311,8 +1313,8 @@ dup_handle_error (int fd, int err)
is specialized for that particular environment where it doesn't make
sense to report errors via exceptions. It uses dup2(2) to duplicate
the file descriptor FD, does *not* close the original FD, and returns
- the new descriptor. If dup2(2) fails, print an error message to ERR
- and abort. */
+ the new descriptor. If dup2(2) fails, send errno to ERR and
+ abort. */
static int
dup2_handle_error (int fd, int to, int err)
{
@@ -1326,9 +1328,11 @@ dup2_handle_error (int fd, int to, int err)
{
/* At this point we are in the child process before exec. We
cannot safely raise an exception in this environment. */
- const char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
- _exit (127); /* Use exit status 127, as with other exec errors. */
+ int errno_save = errno;
+ while (write (err, &errno_save, sizeof (errno)) == -1)
+ if (errno != EINTR)
+ break;
+ _exit (127);
}
return new_fd;
@@ -1347,6 +1351,8 @@ start_child (const char *exec_file, char **exec_argv,
{
int pid;
int max_fd = 1024;
+ int errno_save;
+ int errpipefds[2];
#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
{
@@ -1356,17 +1362,73 @@ start_child (const char *exec_file, char **exec_argv,
}
#endif
+/* Setup a pipe to receive the errno of the child, which can't call
+ async-signal unsafe functions such as strerror or the printf family. */
+#ifdef HAVE_PIPE2
+ if (pipe2 (errpipefds, O_CLOEXEC))
+ return -1;
+#else
+ if (pipe (errpipefds))
+ return -1;
+ /* Set the writer end as close-on-exec, so that it automatically
+ closes on successful exec, and so that other threads that fork+exec
+ will not block it.
+
+ XXX: There can potentially be a race issue between the pipe and
+ fcntl calls, such that another thread that forks in between
+ inherits the fd without CLOEXEC. There is no POSIXy way to make
+ the combination atomic, so just hope that any other fork would
+ release resources it doesn't need, like we do. */
+ if (fcntl (errpipefds[1], F_SETFD, fcntl (errpipefds[1], F_GETFD) | FD_CLOEXEC))
+ {
+ errno_save = errno;
+ close (errpipefds[0]);
+ close (errpipefds[1]);
+ errno = errno_save;
+ return -1;
+ }
+#endif
+
pid = fork ();
if (pid != 0)
- /* The parent, with either and error (pid == -1), or the PID of the
- child. Return directly in either case. */
- return pid;
+ {
+ /* We're in the parent process. */
+ int read_count;
+ close (errpipefds[1]);
+ if (pid == -1)
+ {
+ /* Fork failed. */
+ errno_save = errno;
+ close (errpipefds[0]);
+ errno = errno_save;
+ return -1;
+ }
+
+ /* Fork successful, try to read a potential child errno from the pipe. */
+ while ((read_count = read(errpipefds[0], &errno_save, sizeof (errno))) == -1)
+ if (errno != EAGAIN && errno != EINTR)
+ break;
+ if (read_count == -1)
+ errno_save = errno;
+ close (errpipefds[0]);
+ if (read_count != 0)
+ {
+ /* Either the read failed (-1) or the child sent us an errno (> 0) */
+ errno = errno_save;
+ return -1;
+ }
+ return pid;
+ }
+
+ /* From now on, we are single threaded because of fork, so double
+ closes should be a no-op. */
/* Close all file descriptors in ports inherited from the parent
- except for in, out, and err. Heavy-handed, but robust. */
+ except for in, out, err and the error pipe back to the parent.
+ Heavy-handed, but robust. */
while (max_fd--)
- if (max_fd != in && max_fd != out && max_fd != err)
+ if (max_fd != in && max_fd != out && max_fd != err && max_fd != errpipefds[1])
close (max_fd);
/* Ignore errors on these open() calls. */
@@ -1382,16 +1444,16 @@ start_child (const char *exec_file, char **exec_argv,
in/out/err. Note that dup2 doesn't do anything if its arguments are
equal. */
if (out == 0)
- out = dup_handle_error (out, err);
+ out = dup_handle_error (out, errpipefds[1]);
if (err == 0)
- err = dup_handle_error (err, err);
- dup2_handle_error (in, 0, err);
+ err = dup_handle_error (err, errpipefds[1]);
+ dup2_handle_error (in, 0, errpipefds[1]);
if (err == 1)
- err = dup_handle_error (err, err);
- dup2_handle_error (out, 1, err);
+ err = dup_handle_error (err, errpipefds[1]);
+ dup2_handle_error (out, 1, errpipefds[1]);
- dup2_handle_error (err, 2, err);
+ dup2_handle_error (err, 2, errpipefds[1]);
if (in > 2) close (in);
if (out > 2) close (out);
@@ -1400,11 +1462,10 @@ start_child (const char *exec_file, char **exec_argv,
execvp (exec_file, exec_argv);
/* The exec failed! There is nothing sensible to do. */
- {
- const char *msg = strerror (errno);
- dprintf (2, "In execvp of %s: %s\n",
- exec_file, msg);
- }
+ errno_save = errno;
+ while (write (errpipefds[1], &errno_save, sizeof (errno)) == -1)
+ if (errno != EINTR)
+ break;
/* Use exit status 127, like shells in this case, as per POSIX
<http://pubs.opengroup.org/onlinepubs/007904875/utilities/xcu_chap02.html#tag_02_09_01_01>. */
--
2.36.0
J
J
Josselin Poiret wrote on 5 Sep 2022 08:48
[PATCH v5 0/3] Move spawning procedures to posix_spawn.
(address . 52835@debbugs.gnu.org)
cover.1662358976.git.dev@jpoiret.xyz
Hi everyone,

As was discussed on IRC, if we're going to rewrite a non-negligible part of
posix.c, let's at least do it right and use posix_spawn to handle the process
spawning side of things. This is quite complex to get right in general
(highlighted by this very bug) and so people have already done the hard work for
us. Additionally, we use Gnulib's posix_spawn, so that it is available on all
supported systems. I then adjusted all the procedures to use posix_spawn
instead of scm_piped_process and removed the latter, and the tests in
popen.test, posix.test.

There are two inderminates here:

* I don't have anything other than a Linux system to test. This would need some
feedback for at least Mach and win32.

* This changes the interfaces (for the better, in my opinion): whenever
possible, posix_spawn reports child starting failures as a parent errno, meaning
that for eg. non- existing binaries, system* now throws an exception instead of
returning a pid that will have an exit status code of 127. This means that
existing code that relies on that behavior will need to be changed, the first
example being the test suite which I adapted to actually check for exceptions
instead. Some tests were removed because they no longer make sense: in
posix.test, https://bugs.gnu.org/13166,exit code for nonexistent file and
https://bugs.gnu.org/55596are superseded by "exception for nonexistent file".

Also, I have no experience in using Gnulib so I'm not 100% sure I committed
exactly the right files, I'd love it if someone could check this is ok.

What do you all think about this approach?

Josselin Poiret (3):
Update gnulib to 0.1.5414-8204d and add posix_spawn, posix_spawnp.
Add spawn*.
Move popen and posix procedures to spawn*.

GNUmakefile | 2 +-
build-aux/announce-gen | 69 +-
build-aux/gendocs.sh | 50 +-
build-aux/git-version-gen | 13 +-
build-aux/gitlog-to-changelog | 4 +-
build-aux/gnu-web-doc-update | 4 +-
build-aux/gnupload | 4 +-
build-aux/useless-if-before-free | 6 +-
build-aux/vc-list-files | 2 +-
doc/gendocs_template | 4 +-
doc/gendocs_template_min | 2 +-
gnulib-local/m4/clock_time.m4.diff | 12 +-
lib/Makefile.am | 1252 +++++++++-------
lib/_Noreturn.h | 2 +-
lib/accept.c | 2 +-
lib/accept4.c | 4 +-
lib/access.c | 31 +
lib/alignof.h | 2 +-
lib/alloca.c | 35 -
lib/alloca.in.h | 2 +-
lib/arg-nonnull.h | 2 +-
lib/arpa_inet.in.h | 2 +-
lib/asnprintf.c | 2 +-
lib/assure.h | 2 +-
lib/attribute.h | 10 +-
lib/basename-lgpl.c | 2 +-
lib/basename-lgpl.h | 2 +-
lib/binary-io.c | 2 +-
lib/binary-io.h | 4 +-
lib/bind.c | 2 +-
lib/btowc.c | 2 +-
lib/byteswap.in.h | 2 +-
lib/c++defs.h | 2 +-
lib/c-ctype.c | 2 +-
lib/c-ctype.h | 2 +-
lib/c-strcase.h | 2 +-
lib/c-strcasecmp.c | 2 +-
lib/c-strcaseeq.h | 2 +-
lib/c-strncasecmp.c | 2 +-
lib/canonicalize-lgpl.c | 2 +-
lib/cdefs.h | 76 +-
lib/ceil.c | 4 +-
lib/cloexec.c | 2 +-
lib/cloexec.h | 2 +-
lib/close.c | 2 +-
lib/concat-filename.c | 73 +
lib/concat-filename.h | 46 +
lib/connect.c | 2 +-
lib/copysign.c | 4 +-
lib/dirent.in.h | 24 +-
lib/dirfd.c | 2 +-
lib/dirname-lgpl.c | 2 +-
lib/dirname.h | 2 +-
lib/dup2.c | 2 +-
lib/duplocale.c | 4 +-
lib/dynarray.h | 2 +-
lib/eloop-threshold.h | 2 +-
lib/errno.in.h | 2 +-
lib/fcntl.c | 2 +-
lib/fcntl.in.h | 6 +-
lib/fd-hook.c | 2 +-
lib/fd-hook.h | 2 +-
lib/filename.h | 2 +-
lib/findprog-in.c | 399 ++++++
lib/findprog.h | 77 +
lib/flexmember.h | 2 +-
lib/float+.h | 2 +-
lib/float.c | 2 +-
lib/float.in.h | 2 +-
lib/flock.c | 2 +-
lib/floor.c | 4 +-
lib/free.c | 2 +-
lib/frexp.c | 2 +-
lib/fstat.c | 2 +-
lib/fsync.c | 2 +-
lib/full-read.c | 2 +-
lib/full-read.h | 2 +-
lib/full-write.c | 2 +-
lib/full-write.h | 2 +-
lib/gai_strerror.c | 2 +-
lib/getaddrinfo.c | 2 +-
lib/getdtablesize.c | 2 +-
lib/getlogin.c | 2 +-
lib/getpeername.c | 2 +-
lib/getrandom.c | 2 +-
lib/getsockname.c | 2 +-
lib/getsockopt.c | 2 +-
lib/gettext.h | 15 +-
lib/hard-locale.c | 2 +-
lib/hard-locale.h | 2 +-
lib/iconv.c | 2 +-
lib/iconv.in.h | 2 +-
lib/iconv_close.c | 2 +-
lib/iconv_open-aix.gperf | 2 +-
lib/iconv_open-hpux.gperf | 2 +-
lib/iconv_open-irix.gperf | 2 +-
lib/iconv_open-osf.gperf | 2 +-
lib/iconv_open-solaris.gperf | 2 +-
lib/iconv_open-zos.gperf | 2 +-
lib/iconv_open-zos.h | 329 +++++
lib/iconv_open.c | 2 +-
lib/iconveh.h | 7 +-
lib/idx.h | 22 +-
lib/inet_ntop.c | 2 +-
lib/inet_pton.c | 2 +-
lib/intprops-internal.h | 392 +++++
lib/intprops.h | 359 +----
lib/inttypes.h | 1509 ++++++++++++++++++++
lib/inttypes.in.h | 2 +-
lib/isfinite.c | 4 +-
lib/isinf.c | 4 +-
lib/isnan.c | 2 +-
lib/isnand-nolibm.h | 2 +-
lib/isnand.c | 2 +-
lib/isnanf-nolibm.h | 2 +-
lib/isnanf.c | 2 +-
lib/isnanl-nolibm.h | 2 +-
lib/isnanl.c | 2 +-
lib/itold.c | 2 +-
lib/langinfo.in.h | 2 +-
lib/lc-charset-dispatch.c | 2 +-
lib/lc-charset-dispatch.h | 2 +-
lib/libc-config.h | 13 +-
lib/libunistring.valgrind | 4 +-
lib/limits.in.h | 2 +-
lib/link.c | 2 +-
lib/listen.c | 2 +-
lib/localcharset.c | 2 +-
lib/localcharset.h | 2 +-
lib/locale.in.h | 2 +-
lib/localeconv.c | 2 +-
lib/log.c | 4 +-
lib/log1p.c | 4 +-
lib/lstat.c | 2 +-
lib/malloc.c | 2 +-
lib/malloc/.dirstamp | 0
lib/malloc/dynarray-skeleton.c | 2 +-
lib/malloc/dynarray-skeleton.gl.h | 529 +++++++
lib/malloc/dynarray.gl.h | 174 +++
lib/malloc/dynarray.h | 2 +-
lib/malloc/dynarray_at_failure.c | 2 +-
lib/malloc/dynarray_emplace_enlarge.c | 2 +-
lib/malloc/dynarray_finalize.c | 2 +-
lib/malloc/dynarray_resize.c | 2 +-
lib/malloc/dynarray_resize_clear.c | 2 +-
lib/malloc/scratch_buffer.h | 2 +-
lib/malloc/scratch_buffer_dupfree.c | 2 +-
lib/malloc/scratch_buffer_grow.c | 2 +-
lib/malloc/scratch_buffer_grow_preserve.c | 2 +-
lib/malloc/scratch_buffer_set_array_size.c | 2 +-
lib/malloca.c | 12 +-
lib/malloca.h | 2 +-
lib/math.c | 2 +-
lib/math.in.h | 16 +-
lib/mbrtowc-impl-utf8.h | 2 +-
lib/mbrtowc-impl.h | 2 +-
lib/mbrtowc.c | 2 +-
lib/mbsinit.c | 2 +-
lib/mbtowc-impl.h | 2 +-
lib/mbtowc-lock.c | 2 +-
lib/mbtowc-lock.h | 2 +-
lib/mbtowc.c | 2 +-
lib/memchr.c | 2 +-
lib/memchr.valgrind | 2 +-
lib/mempcpy.c | 2 +-
lib/minmax.h | 2 +-
lib/mkdir.c | 4 +-
lib/mkostemp.c | 2 +-
lib/mktime-internal.h | 2 +-
lib/mktime.c | 30 +-
lib/msvc-inval.c | 2 +-
lib/msvc-inval.h | 2 +-
lib/msvc-nothrow.c | 2 +-
lib/msvc-nothrow.h | 2 +-
lib/netdb.in.h | 2 +-
lib/netinet_in.in.h | 2 +-
lib/nl_langinfo-lock.c | 2 +-
lib/nl_langinfo.c | 2 +-
lib/nproc.c | 24 +-
lib/nproc.h | 2 +-
lib/nstrftime.c | 28 +-
lib/open.c | 2 +-
lib/pathmax.h | 2 +-
lib/pipe.c | 2 +-
lib/pipe2.c | 2 +-
lib/poll.c | 2 +-
lib/poll.in.h | 2 +-
lib/printf-args.c | 2 +-
lib/printf-args.h | 2 +-
lib/printf-parse.c | 2 +-
lib/printf-parse.h | 2 +-
lib/putenv.c | 4 +-
lib/raise.c | 2 +-
lib/rawmemchr.c | 78 +-
lib/rawmemchr.valgrind | 2 +-
lib/read.c | 2 +-
lib/readlink.c | 6 +-
lib/realloc.c | 2 +-
lib/recv.c | 2 +-
lib/recvfrom.c | 2 +-
lib/regcomp.c | 831 +++++------
lib/regex.c | 3 +-
lib/regex.h | 52 +-
lib/regex_internal.c | 64 +-
lib/regex_internal.h | 51 +-
lib/regexec.c | 95 +-
lib/rename.c | 4 +-
lib/rmdir.c | 2 +-
lib/round.c | 4 +-
lib/safe-read.c | 2 +-
lib/safe-read.h | 2 +-
lib/safe-write.c | 2 +-
lib/safe-write.h | 2 +-
lib/same-inode.h | 2 +-
lib/sched.h | 580 ++++++++
lib/sched.in.h | 99 ++
lib/scratch_buffer.h | 2 +-
lib/select.c | 2 +-
lib/send.c | 2 +-
lib/sendto.c | 2 +-
lib/setenv.c | 2 +-
lib/setlocale-lock.c | 2 +-
lib/setlocale_null.c | 2 +-
lib/setlocale_null.h | 2 +-
lib/setsockopt.c | 2 +-
lib/shutdown.c | 2 +-
lib/signal.in.h | 2 +-
lib/signbitd.c | 2 +-
lib/signbitf.c | 2 +-
lib/signbitl.c | 2 +-
lib/size_max.h | 2 +-
lib/snprintf.c | 2 +-
lib/socket.c | 2 +-
lib/sockets.c | 4 +-
lib/sockets.h | 2 +-
lib/spawn.c | 34 +
lib/spawn.h | 1499 +++++++++++++++++++
lib/spawn.in.h | 992 +++++++++++++
lib/spawn_int.h | 72 +
lib/spawni.c | 965 +++++++++++++
lib/spawnp.c | 34 +
lib/stat-time.c | 2 +-
lib/stat-time.h | 8 +-
lib/stat-w32.c | 2 +-
lib/stat-w32.h | 2 +-
lib/stat.c | 2 +-
lib/stdalign.in.h | 2 +-
lib/stdbool.h | 116 ++
lib/stdbool.in.h | 27 +-
lib/stdckdint.h | 38 +
lib/stdckdint.in.h | 37 +
lib/stddef.in.h | 2 +-
lib/stdint.in.h | 2 +-
lib/stdio-read.c | 168 +++
lib/stdio-write.c | 206 +++
lib/stdio.in.h | 86 +-
lib/stdlib.in.h | 66 +-
lib/stpcpy.c | 49 +
lib/strchrnul.c | 142 ++
lib/strchrnul.valgrind | 28 +
lib/strdup.c | 2 +-
lib/streq.h | 2 +-
lib/strftime.h | 4 +-
lib/striconveh.c | 106 +-
lib/striconveh.h | 2 +-
lib/string.in.h | 175 ++-
lib/stripslash.c | 2 +-
lib/sys-limits.h | 2 +-
lib/sys_file.in.h | 2 +-
lib/sys_random.in.h | 8 +-
lib/sys_select.in.h | 15 +-
lib/sys_socket.c | 2 +-
lib/sys_socket.in.h | 2 +-
lib/sys_stat.in.h | 30 +-
lib/sys_time.in.h | 2 +-
lib/sys_times.in.h | 2 +-
lib/sys_types.in.h | 2 +-
lib/sys_uio.in.h | 2 +-
lib/tempname.c | 176 +--
lib/tempname.h | 4 +-
lib/time-internal.h | 4 +-
lib/time.in.h | 13 +-
lib/time_r.c | 2 +-
lib/time_rz.c | 4 +-
lib/timegm.c | 2 +-
lib/times.c | 2 +-
lib/trunc.c | 4 +-
lib/tzset.c | 4 +-
lib/unistd.c | 2 +-
lib/unistd.in.h | 21 +-
lib/unsetenv.c | 2 +-
lib/vasnprintf.c | 241 +---
lib/vasnprintf.h | 2 +-
lib/verify.h | 14 +-
lib/vsnprintf.c | 2 +-
lib/w32sock.h | 2 +-
lib/warn-on-use.h | 8 +-
lib/wchar.in.h | 73 +-
lib/wcrtomb.c | 2 +-
lib/wctype-h.c | 2 +-
lib/wctype.in.h | 2 +-
lib/windows-initguard.h | 2 +-
lib/windows-spawn.c | 727 ++++++++++
lib/windows-spawn.h | 157 ++
lib/write.c | 2 +-
lib/xalloc-oversized.h | 2 +-
lib/xsize.c | 2 +-
lib/xsize.h | 2 +-
libguile/posix.c | 221 +--
m4/00gnulib.m4 | 2 +-
m4/__inline.m4 | 2 +-
m4/absolute-header.m4 | 2 +-
m4/accept4.m4 | 2 +-
m4/access.m4 | 16 +
m4/alloca.m4 | 12 +-
m4/arpa_inet_h.m4 | 2 +-
m4/autobuild.m4 | 2 +-
m4/btowc.m4 | 2 +-
m4/builtin-expect.m4 | 2 +-
m4/byteswap.m4 | 10 +-
m4/canonicalize.m4 | 2 +-
m4/ceil.m4 | 2 +-
m4/check-math-lib.m4 | 2 +-
m4/clock_time.m4 | 20 +-
m4/close.m4 | 2 +-
m4/codeset.m4 | 2 +-
m4/copysign.m4 | 2 +-
m4/dirent_h.m4 | 2 +-
m4/dirfd.m4 | 2 +-
m4/double-slash-root.m4 | 2 +-
m4/dup2.m4 | 2 +-
m4/duplocale.m4 | 2 +-
m4/eaccess.m4 | 12 +
m4/eealloc.m4 | 2 +-
m4/environ.m4 | 2 +-
m4/errno_h.m4 | 1
This message was truncated. Download the full message here.
J
J
Josselin Poiret wrote on 5 Sep 2022 08:48
[PATCH v5 1/3] Update gnulib to 0.1.5414-8204d and add posix_spawn, posix_spawnp.
(address . 52835@debbugs.gnu.org)
ae50942e90f9a5035b5c3db3928c7e78235f0d72.1662358976.git.dev@jpoiret.xyz
---
GNUmakefile | 2 +-
build-aux/announce-gen | 69 +-
build-aux/gendocs.sh | 50 +-
build-aux/git-version-gen | 13 +-
build-aux/gitlog-to-changelog | 4 +-
build-aux/gnu-web-doc-update | 4 +-
build-aux/gnupload | 4 +-
build-aux/useless-if-before-free | 6 +-
build-aux/vc-list-files | 2 +-
doc/gendocs_template | 4 +-
doc/gendocs_template_min | 2 +-
gnulib-local/m4/clock_time.m4.diff | 12 +-
lib/Makefile.am | 1252 +++++++++-------
lib/_Noreturn.h | 2 +-
lib/accept.c | 2 +-
lib/accept4.c | 4 +-
lib/access.c | 31 +
lib/alignof.h | 2 +-
lib/alloca.c | 35 -
lib/alloca.in.h | 2 +-
lib/arg-nonnull.h | 2 +-
lib/arpa_inet.in.h | 2 +-
lib/asnprintf.c | 2 +-
lib/assure.h | 2 +-
lib/attribute.h | 10 +-
lib/basename-lgpl.c | 2 +-
lib/basename-lgpl.h | 2 +-
lib/binary-io.c | 2 +-
lib/binary-io.h | 4 +-
lib/bind.c | 2 +-
lib/btowc.c | 2 +-
lib/byteswap.in.h | 2 +-
lib/c++defs.h | 2 +-
lib/c-ctype.c | 2 +-
lib/c-ctype.h | 2 +-
lib/c-strcase.h | 2 +-
lib/c-strcasecmp.c | 2 +-
lib/c-strcaseeq.h | 2 +-
lib/c-strncasecmp.c | 2 +-
lib/canonicalize-lgpl.c | 2 +-
lib/cdefs.h | 76 +-
lib/ceil.c | 4 +-
lib/cloexec.c | 2 +-
lib/cloexec.h | 2 +-
lib/close.c | 2 +-
lib/concat-filename.c | 73 +
lib/concat-filename.h | 46 +
lib/connect.c | 2 +-
lib/copysign.c | 4 +-
lib/dirent.in.h | 24 +-
lib/dirfd.c | 2 +-
lib/dirname-lgpl.c | 2 +-
lib/dirname.h | 2 +-
lib/dup2.c | 2 +-
lib/duplocale.c | 4 +-
lib/dynarray.h | 2 +-
lib/eloop-threshold.h | 2 +-
lib/errno.in.h | 2 +-
lib/fcntl.c | 2 +-
lib/fcntl.in.h | 6 +-
lib/fd-hook.c | 2 +-
lib/fd-hook.h | 2 +-
lib/filename.h | 2 +-
lib/findprog-in.c | 399 ++++++
lib/findprog.h | 77 +
lib/flexmember.h | 2 +-
lib/float+.h | 2 +-
lib/float.c | 2 +-
lib/float.in.h | 2 +-
lib/flock.c | 2 +-
lib/floor.c | 4 +-
lib/free.c | 2 +-
lib/frexp.c | 2 +-
lib/fstat.c | 2 +-
lib/fsync.c | 2 +-
lib/full-read.c | 2 +-
lib/full-read.h | 2 +-
lib/full-write.c | 2 +-
lib/full-write.h | 2 +-
lib/gai_strerror.c | 2 +-
lib/getaddrinfo.c | 2 +-
lib/getdtablesize.c | 2 +-
lib/getlogin.c | 2 +-
lib/getpeername.c | 2 +-
lib/getrandom.c | 2 +-
lib/getsockname.c | 2 +-
lib/getsockopt.c | 2 +-
lib/gettext.h | 15 +-
lib/hard-locale.c | 2 +-
lib/hard-locale.h | 2 +-
lib/iconv.c | 2 +-
lib/iconv.in.h | 2 +-
lib/iconv_close.c | 2 +-
lib/iconv_open-aix.gperf | 2 +-
lib/iconv_open-hpux.gperf | 2 +-
lib/iconv_open-irix.gperf | 2 +-
lib/iconv_open-osf.gperf | 2 +-
lib/iconv_open-solaris.gperf | 2 +-
lib/iconv_open-zos.gperf | 2 +-
lib/iconv_open-zos.h | 329 +++++
lib/iconv_open.c | 2 +-
lib/iconveh.h | 7 +-
lib/idx.h | 22 +-
lib/inet_ntop.c | 2 +-
lib/inet_pton.c | 2 +-
lib/intprops-internal.h | 392 +++++
lib/intprops.h | 359 +----
lib/inttypes.h | 1509 ++++++++++++++++++++
lib/inttypes.in.h | 2 +-
lib/isfinite.c | 4 +-
lib/isinf.c | 4 +-
lib/isnan.c | 2 +-
lib/isnand-nolibm.h | 2 +-
lib/isnand.c | 2 +-
lib/isnanf-nolibm.h | 2 +-
lib/isnanf.c | 2 +-
lib/isnanl-nolibm.h | 2 +-
lib/isnanl.c | 2 +-
lib/itold.c | 2 +-
lib/langinfo.in.h | 2 +-
lib/lc-charset-dispatch.c | 2 +-
lib/lc-charset-dispatch.h | 2 +-
lib/libc-config.h | 13 +-
lib/libunistring.valgrind | 4 +-
lib/limits.in.h | 2 +-
lib/link.c | 2 +-
lib/listen.c | 2 +-
lib/localcharset.c | 2 +-
lib/localcharset.h | 2 +-
lib/locale.in.h | 2 +-
lib/localeconv.c | 2 +-
lib/log.c | 4 +-
lib/log1p.c | 4 +-
lib/lstat.c | 2 +-
lib/malloc.c | 2 +-
lib/malloc/.dirstamp | 0
lib/malloc/dynarray-skeleton.c | 2 +-
lib/malloc/dynarray-skeleton.gl.h | 529 +++++++
lib/malloc/dynarray.gl.h | 174 +++
lib/malloc/dynarray.h | 2 +-
lib/malloc/dynarray_at_failure.c | 2 +-
lib/malloc/dynarray_emplace_enlarge.c | 2 +-
lib/malloc/dynarray_finalize.c | 2 +-
lib/malloc/dynarray_resize.c | 2 +-
lib/malloc/dynarray_resize_clear.c | 2 +-
lib/malloc/scratch_buffer.h | 2 +-
lib/malloc/scratch_buffer_dupfree.c | 2 +-
lib/malloc/scratch_buffer_grow.c | 2 +-
lib/malloc/scratch_buffer_grow_preserve.c | 2 +-
lib/malloc/scratch_buffer_set_array_size.c | 2 +-
lib/malloca.c | 12 +-
lib/malloca.h | 2 +-
lib/math.c | 2 +-
lib/math.in.h | 16 +-
lib/mbrtowc-impl-utf8.h | 2 +-
lib/mbrtowc-impl.h | 2 +-
lib/mbrtowc.c | 2 +-
lib/mbsinit.c | 2 +-
lib/mbtowc-impl.h | 2 +-
lib/mbtowc-lock.c | 2 +-
lib/mbtowc-lock.h | 2 +-
lib/mbtowc.c | 2 +-
lib/memchr.c | 2 +-
lib/memchr.valgrind | 2 +-
lib/mempcpy.c | 2 +-
lib/minmax.h | 2 +-
lib/mkdir.c | 4 +-
lib/mkostemp.c | 2 +-
lib/mktime-internal.h | 2 +-
lib/mktime.c | 30 +-
lib/msvc-inval.c | 2 +-
lib/msvc-inval.h | 2 +-
lib/msvc-nothrow.c | 2 +-
lib/msvc-nothrow.h | 2 +-
lib/netdb.in.h | 2 +-
lib/netinet_in.in.h | 2 +-
lib/nl_langinfo-lock.c | 2 +-
lib/nl_langinfo.c | 2 +-
lib/nproc.c | 24 +-
lib/nproc.h | 2 +-
lib/nstrftime.c | 28 +-
lib/open.c | 2 +-
lib/pathmax.h | 2 +-
lib/pipe.c | 2 +-
lib/pipe2.c | 2 +-
lib/poll.c | 2 +-
lib/poll.in.h | 2 +-
lib/printf-args.c | 2 +-
lib/printf-args.h | 2 +-
lib/printf-parse.c | 2 +-
lib/printf-parse.h | 2 +-
lib/putenv.c | 4 +-
lib/raise.c | 2 +-
lib/rawmemchr.c | 78 +-
lib/rawmemchr.valgrind | 2 +-
lib/read.c | 2 +-
lib/readlink.c | 6 +-
lib/realloc.c | 2 +-
lib/recv.c | 2 +-
lib/recvfrom.c | 2 +-
lib/regcomp.c | 831 +++++------
lib/regex.c | 3 +-
lib/regex.h | 52 +-
lib/regex_internal.c | 64 +-
lib/regex_internal.h | 51 +-
lib/regexec.c | 95 +-
lib/rename.c | 4 +-
lib/rmdir.c | 2 +-
lib/round.c | 4 +-
lib/safe-read.c | 2 +-
lib/safe-read.h | 2 +-
lib/safe-write.c | 2 +-
lib/safe-write.h | 2 +-
lib/same-inode.h | 2 +-
lib/sched.h | 580 ++++++++
lib/sched.in.h | 99 ++
lib/scratch_buffer.h | 2 +-
lib/select.c | 2 +-
lib/send.c | 2 +-
lib/sendto.c | 2 +-
lib/setenv.c | 2 +-
lib/setlocale-lock.c | 2 +-
lib/setlocale_null.c | 2 +-
lib/setlocale_null.h | 2 +-
lib/setsockopt.c | 2 +-
lib/shutdown.c | 2 +-
lib/signal.in.h | 2 +-
lib/signbitd.c | 2 +-
lib/signbitf.c | 2 +-
lib/signbitl.c | 2 +-
lib/size_max.h | 2 +-
lib/snprintf.c | 2 +-
lib/socket.c | 2 +-
lib/sockets.c | 4 +-
lib/sockets.h | 2 +-
lib/spawn.c | 34 +
lib/spawn.h | 1499 +++++++++++++++++++
lib/spawn.in.h | 992 +++++++++++++
lib/spawn_int.h | 72 +
lib/spawni.c | 965 +++++++++++++
lib/spawnp.c | 34 +
lib/stat-time.c | 2 +-
lib/stat-time.h | 8 +-
lib/stat-w32.c | 2 +-
lib/stat-w32.h | 2 +-
lib/stat.c | 2 +-
lib/stdalign.in.h | 2 +-
lib/stdbool.h | 116 ++
lib/stdbool.in.h | 27 +-
lib/stdckdint.h | 38 +
lib/stdckdint.in.h | 37 +
lib/stddef.in.h | 2 +-
lib/stdint.in.h | 2 +-
lib/stdio-read.c | 168 +++
lib/stdio-write.c | 206 +++
lib/stdio.in.h | 86 +-
lib/stdlib.in.h | 66 +-
lib/stpcpy.c | 49 +
lib/strchrnul.c | 142 ++
lib/strchrnul.valgrind | 28 +
lib/strdup.c | 2 +-
lib/streq.h | 2 +-
lib/strftime.h | 4 +-
lib/striconveh.c | 106 +-
lib/striconveh.h | 2 +-
lib/string.in.h | 175 ++-
lib/stripslash.c | 2 +-
lib/sys-limits.h | 2 +-
lib/sys_file.in.h | 2 +-
lib/sys_random.in.h | 8 +-
lib/sys_select.in.h | 15 +-
lib/sys_socket.c | 2 +-
lib/sys_socket.in.h | 2 +-
lib/sys_stat.in.h | 30 +-
lib/sys_time.in.h | 2 +-
lib/sys_times.in.h | 2 +-
lib/sys_types.in.h | 2 +-
lib/sys_uio.in.h | 2 +-
lib/tempname.c | 176 +--
lib/tempname.h | 4 +-
lib/time-internal.h | 4 +-
lib/time.in.h | 13 +-
lib/time_r.c | 2 +-
lib/time_rz.c | 4 +-
lib/timegm.c | 2 +-
lib/times.c | 2 +-
lib/trunc.c | 4 +-
lib/tzset.c | 4 +-
lib/unistd.c | 2 +-
lib/unistd.in.h | 21 +-
lib/unsetenv.c | 2 +-
lib/vasnprintf.c | 241 +---
lib/vasnprintf.h | 2 +-
lib/verify.h | 14 +-
lib/vsnprintf.c | 2 +-
lib/w32sock.h | 2 +-
lib/warn-on-use.h | 8 +-
lib/wchar.in.h | 73 +-
lib/wcrtomb.c | 2 +-
lib/wctype-h.c | 2 +-
lib/wctype.in.h | 2 +-
lib/windows-initguard.h | 2 +-
lib/windows-spawn.c | 727 ++++++++++
lib/windows-spawn.h | 157 ++
lib/write.c | 2 +-
lib/xalloc-oversized.h | 2 +-
lib/xsize.c | 2 +-
lib/xsize.h | 2 +-
m4/00gnulib.m4 | 2 +-
m4/__inline.m4 | 2 +-
m4/absolute-header.m4 | 2 +-
m4/accept4.m4 | 2 +-
m4/access.m4 | 16 +
m4/alloca.m4 | 12 +-
m4/arpa_inet_h.m4 | 2 +-
m4/autobuild.m4 | 2 +-
m4/btowc.m4 | 2 +-
m4/builtin-expect.m4 | 2 +-
m4/byteswap.m4 | 10 +-
m4/canonicalize.m4 | 2 +-
m4/ceil.m4 | 2 +-
m4/check-math-lib.m4 | 2 +-
m4/clock_time.m4 | 20 +-
m4/close.m4 | 2 +-
m4/codeset.m4 | 2 +-
m4/copysign.m4 | 2 +-
m4/dirent_h.m4 | 2 +-
m4/dirfd.m4 | 2 +-
m4/double-slash-root.m4 | 2 +-
m4/dup2.m4 | 2 +-
m4/duplocale.m4 | 2 +-
m4/eaccess.m4 | 12 +
m4/eealloc.m4 | 2 +-
m4/environ.m4 | 2 +-
m4/errno_h.m4 | 12 +-
m4/exponentd.m4 | 2 +-
m4/exponentf.m4 | 2 +-
m4/exponentl.m4 | 2 +-
m4/extensions.m4 | 2 +-
m4/extern-inline.m4 | 28 +-
m4/fcntl-o.m4 | 2 +-
m4/fcntl.m4 | 2 +-
m4/fcntl_h.m4 | 2 +-
m4/findprog-in.m4 | 11 +
m4/flexmember.m4 | 2 +-
m4/float_h.m4 | 22 +-
m4/flock.m4 | 2 +-
m4/floor.m4 | 2 +-
m4/fpieee.m4 | 2 +-
m4/free.m4 | 2 +-
m4/frexp.m4 | 2 +-
m4/fstat.m4 | 2 +-
m4/fsync.m4 | 2 +-
m4/func.m4 | 2 +-
m4/getaddrinfo.m4 | 2 +-
m4/getdtablesize.m4 | 2 +-
m4/getlogin.m4 | 2 +-
m4/getrandom.m4 | 2 +-
m4/gettext.m4 | 383 +++++
m4/glibc2.m4 | 30 +
m4/glibc21.m4 | 30 +
m4/gnulib-cache.m4 | 8 +-
m4/gnulib-common.m4 | 277 +++-
m4/gnulib-comp.m4 | 743 ++++++----
m4/gnulib-tool.m4 | 10 +-
m4/host-cpu-c-abi.m4 | 7 +-
m4/hostent.m4 | 2 +-
m4/iconv.m4 | 26 +-
m4/iconv_h.
This message was truncated. Download the full message here.
J
J
Josselin Poiret wrote on 5 Sep 2022 08:48
[PATCH v5 2/3] Add spawn*.
(address . 52835@debbugs.gnu.org)
95dbb2e804221fc369203d38c75238a190d3ff6f.1662358976.git.dev@jpoiret.xyz
* libguile/posix.c: Include spawn.h from Gnulib.
(scm_spawn_process): New function.
(scm_init_popen): Define spawn*.
---
libguile/posix.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 71 insertions(+)

Toggle diff (98 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index f4ca72d3e..5d287ff2a 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -32,6 +32,7 @@
#include <sys/types.h>
#include <uniconv.h>
#include <unistd.h>
+#include <spawn.h>
#ifdef HAVE_SCHED_H
# include <sched.h>
@@ -1472,6 +1473,75 @@ scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
}
#undef FUNC_NAME
+static SCM
+scm_spawn_process (SCM prog, SCM args, SCM scm_in, SCM scm_out, SCM scm_err)
+#define FUNC_NAME "spawn*"
+{
+ int in, out, err;
+ int pid;
+ char *exec_file;
+ char **exec_argv;
+ char **exec_env = NULL;
+
+ posix_spawn_file_actions_t actions;
+ posix_spawnattr_t *attrp = NULL;
+
+ exec_file = scm_to_locale_string (prog);
+ exec_argv = scm_i_allocate_string_pointers (args);
+
+ in = scm_to_int (scm_in);
+ out = scm_to_int (scm_out);
+ err = scm_to_int (scm_err);
+
+ int max_fd = 1024;
+
+#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
+ {
+ struct rlimit lim = { 0, 0 };
+ if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
+ max_fd = lim.rlim_cur;
+ }
+#endif
+
+ posix_spawn_file_actions_init (&actions);
+
+ int free_fd_slots = 0;
+ int fd_slot[3];
+
+ for (int fdnum = 3;free_fd_slots < 3 && fdnum < max_fd;fdnum++)
+ {
+ if (fdnum != in && fdnum != out && fdnum != err)
+ {
+ fd_slot[free_fd_slots] = fdnum;
+ free_fd_slots++;
+ }
+ }
+
+ /* Move the fds out of the way, so that duplicate fds or fds equal
+ to 0, 1, 2 don't trample each other */
+
+ posix_spawn_file_actions_adddup2 (&actions, in, fd_slot[0]);
+ posix_spawn_file_actions_adddup2 (&actions, out, fd_slot[1]);
+ posix_spawn_file_actions_adddup2 (&actions, err, fd_slot[2]);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[0], 0);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[1], 1);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[2], 2);
+
+ while (--max_fd > 2)
+ posix_spawn_file_actions_addclose (&actions, max_fd);
+
+ if (posix_spawnp (&pid, exec_file, &actions, attrp, exec_argv, environ) != 0)
+ {
+ int errno_save = errno;
+ free (exec_file);
+ errno = errno_save;
+ SCM_SYSERROR;
+ }
+
+ return scm_from_int (pid);
+}
+#undef FUNC_NAME
+
static void
restore_sigaction (SCM pair)
{
@@ -2381,6 +2451,7 @@ static void
scm_init_popen (void)
{
scm_c_define_gsubr ("piped-process", 2, 2, 0, scm_piped_process);
+ scm_c_define_gsubr ("spawn*", 5, 0, 0, scm_spawn_process);
}
#endif /* HAVE_START_CHILD */
--
2.37.2
J
J
Josselin Poiret wrote on 5 Sep 2022 08:48
[PATCH v5 3/3] Move popen and posix procedures to spawn*.
(address . 52835@debbugs.gnu.org)
1d64d8e0e292fc3a89bcd491dd8f10171cb7c804.1662358976.git.dev@jpoiret.xyz
* 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(-)

Toggle diff (487 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 5d287ff2a..c35346f9f 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -73,6 +73,7 @@
#include "fports.h"
#include "gettext.h"
#include "gsubr.h"
+#include "ioext.h"
#include "list.h"
#include "modules.h"
#include "numbers.h"
@@ -1280,199 +1281,6 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#undef FUNC_NAME
#endif /* HAVE_FORK */
-#ifdef HAVE_FORK
-/* 'renumber_file_descriptor' is a helper function for 'start_child'
- below, and is specialized for that particular environment where it
- doesn't make sense to report errors via exceptions. It uses dup(2)
- to duplicate the file descriptor FD, closes the original FD, and
- returns the new descriptor. If dup(2) fails, print an error message
- to ERR and abort. */
-static int
-renumber_file_descriptor (int fd, int err)
-{
- int new_fd;
-
- do
- new_fd = dup (fd);
- while (new_fd == -1 && errno == EINTR);
-
- if (new_fd == -1)
- {
- /* At this point we are in the child process before exec. We
- cannot safely raise an exception in this environment. */
- const char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
- _exit (127); /* Use exit status 127, as with other exec errors. */
- }
-
- close (fd);
- return new_fd;
-}
-#endif /* HAVE_FORK */
-
-#ifdef HAVE_FORK
-#define HAVE_START_CHILD 1
-/* Since Guile uses threads, we have to be very careful to avoid calling
- functions that are not async-signal-safe in the child. That's why
- this function is implemented in C. */
-static pid_t
-start_child (const char *exec_file, char **exec_argv,
- int reading, int c2p[2], int writing, int p2c[2],
- int in, int out, int err)
-{
- int pid;
- int max_fd = 1024;
-
-#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
- {
- struct rlimit lim = { 0, 0 };
- if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
- max_fd = lim.rlim_cur;
- }
-#endif
-
- pid = fork ();
-
- if (pid != 0)
- /* The parent, with either and error (pid == -1), or the PID of the
- child. Return directly in either case. */
- return pid;
-
- /* The child. */
- if (reading)
- close (c2p[0]);
- if (writing)
- close (p2c[1]);
-
- /* Close all file descriptors in ports inherited from the parent
- except for in, out, and err. Heavy-handed, but robust. */
- while (max_fd--)
- if (max_fd != in && max_fd != out && max_fd != err)
- close (max_fd);
-
- /* Ignore errors on these open() calls. */
- if (in == -1)
- in = open ("/dev/null", O_RDONLY);
- if (out == -1)
- out = open ("/dev/null", O_WRONLY);
- if (err == -1)
- err = open ("/dev/null", O_WRONLY);
-
- if (in > 0)
- {
- if (out == 0)
- out = renumber_file_descriptor (out, err);
- if (err == 0)
- err = renumber_file_descriptor (err, err);
- do dup2 (in, 0); while (errno == EINTR);
- close (in);
- }
- if (out > 1)
- {
- if (err == 1)
- err = renumber_file_descriptor (err, err);
- do dup2 (out, 1); while (errno == EINTR);
- if (out > 2)
- close (out);
- }
- if (err > 2)
- {
- do dup2 (err, 2); while (errno == EINTR);
- close (err);
- }
-
- execvp (exec_file, exec_argv);
-
- /* The exec failed! There is nothing sensible to do. */
- {
- const char *msg = strerror (errno);
- fprintf (fdopen (2, "a"), "In execvp of %s: %s\n",
- exec_file, msg);
- }
-
- /* Use exit status 127, like shells in this case, as per POSIX
- <http://pubs.opengroup.org/onlinepubs/007904875/utilities/xcu_chap02.html#tag_02_09_01_01>. */
- _exit (127);
-
- /* Not reached. */
- return -1;
-}
-#endif
-
-#ifdef HAVE_START_CHILD
-static SCM
-scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
-#define FUNC_NAME "piped-process"
-{
- int reading, writing;
- int c2p[2]; /* Child to parent. */
- int p2c[2]; /* Parent to child. */
- int in = -1, out = -1, err = -1;
- int pid;
- char *exec_file;
- char **exec_argv;
-
- exec_file = scm_to_locale_string (prog);
- exec_argv = scm_i_allocate_string_pointers (scm_cons (prog, args));
-
- reading = scm_is_pair (from);
- writing = scm_is_pair (to);
-
- if (reading)
- {
- c2p[0] = scm_to_int (scm_car (from));
- c2p[1] = scm_to_int (scm_cdr (from));
- out = c2p[1];
- }
-
- if (writing)
- {
- p2c[0] = scm_to_int (scm_car (to));
- p2c[1] = scm_to_int (scm_cdr (to));
- in = p2c[0];
- }
-
- {
- SCM port;
-
- if (SCM_OPOUTFPORTP ((port = scm_current_error_port ())))
- err = SCM_FPORT_FDES (port);
- if (out == -1 && SCM_OPOUTFPORTP ((port = scm_current_output_port ())))
- out = SCM_FPORT_FDES (port);
- if (in == -1 && SCM_OPINFPORTP ((port = scm_current_input_port ())))
- in = SCM_FPORT_FDES (port);
- }
-
- pid = start_child (exec_file, exec_argv, reading, c2p, writing, p2c,
- in, out, err);
-
- if (pid == -1)
- {
- int errno_save = errno;
- free (exec_file);
- if (reading)
- {
- close (c2p[0]);
- close (c2p[1]);
- }
- if (writing)
- {
- close (p2c[0]);
- close (p2c[1]);
- }
- errno = errno_save;
- SCM_SYSERROR;
- }
-
- if (reading)
- close (c2p[1]);
- if (writing)
- close (p2c[0]);
-
- return scm_from_int (pid);
-}
-#undef FUNC_NAME
-
static SCM
scm_spawn_process (SCM prog, SCM args, SCM scm_in, SCM scm_out, SCM scm_err)
#define FUNC_NAME "spawn*"
@@ -1563,6 +1371,15 @@ scm_dynwind_sigaction (int sig, SCM handler, SCM flags)
SCM_F_WIND_EXPLICITLY);
}
+static SCM
+scm_port_to_fd_with_default (SCM port, int mode)
+{
+ if (!SCM_FPORTP (port))
+ return scm_from_int (open_or_open64 ("/dev/null", mode));
+ return scm_fileno (port);
+
+}
+
SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
(SCM args),
"Execute the command indicated by @var{args}. The first element must\n"
@@ -1589,7 +1406,6 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
if (scm_is_null (args))
SCM_WRONG_NUM_ARGS ();
prog = scm_car (args);
- args = scm_cdr (args);
scm_dynwind_begin (0);
/* Make sure the child can't kill us (as per normal system call). */
@@ -1602,7 +1418,13 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
SCM_UNDEFINED);
#endif
- pid = scm_piped_process (prog, args, SCM_UNDEFINED, SCM_UNDEFINED);
+ SCM in, out, err;
+
+ in = scm_port_to_fd_with_default (scm_current_input_port (), O_RDONLY);
+ out = scm_port_to_fd_with_default (scm_current_output_port (), O_WRONLY);
+ err = scm_port_to_fd_with_default (scm_current_error_port (), O_WRONLY);
+
+ 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;
@@ -1612,7 +1434,6 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
return scm_from_int (status);
}
#undef FUNC_NAME
-#endif /* HAVE_START_CHILD */
#ifdef HAVE_UNAME
SCM_DEFINE (scm_uname, "uname", 0, 0, 0,
@@ -2446,14 +2267,11 @@ SCM_DEFINE (scm_gethostname, "gethostname", 0, 0, 0,
#endif /* HAVE_GETHOSTNAME */
-#ifdef HAVE_START_CHILD
static void
scm_init_popen (void)
{
- scm_c_define_gsubr ("piped-process", 2, 2, 0, scm_piped_process);
scm_c_define_gsubr ("spawn*", 5, 0, 0, scm_spawn_process);
}
-#endif /* HAVE_START_CHILD */
void
scm_init_posix ()
@@ -2566,11 +2384,9 @@ scm_init_posix ()
#ifdef HAVE_FORK
scm_add_feature ("fork");
#endif /* HAVE_FORK */
-#ifdef HAVE_START_CHILD
scm_add_feature ("popen");
scm_c_register_extension ("libguile-" SCM_EFFECTIVE_VERSION,
"scm_init_popen",
(scm_t_extension_init_func) scm_init_popen,
NULL);
-#endif /* HAVE_START_CHILD */
}
diff --git a/module/ice-9/popen.scm b/module/ice-9/popen.scm
index e638726a4..533282f4d 100644
--- a/module/ice-9/popen.scm
+++ b/module/ice-9/popen.scm
@@ -25,12 +25,37 @@
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-9)
#:export (port/pid-table open-pipe* open-pipe close-pipe open-input-pipe
- open-output-pipe open-input-output-pipe pipeline))
+ open-output-pipe open-input-output-pipe pipeline
+ spawn* spawn))
(eval-when (expand load eval)
(load-extension (string-append "libguile-" (effective-version))
"scm_init_popen"))
+(define (port-with-defaults port default-mode)
+ (if (file-port? port)
+ port
+ (open-file "/dev/null" default-mode)))
+
+(define* (spawn exec-file argv #:key
+ (in (current-input-port))
+ (out (current-output-port))
+ (err (current-error-port)))
+ (let* ((in (port-with-defaults in "r"))
+ (out (port-with-defaults out "w"))
+ (err (port-with-defaults err "w"))
+ ;; Increment port revealed counts while to prevent ports GC'ing and
+ ;; closing the associated fds while we spawn the process.
+ (result (spawn* exec-file
+ argv
+ (port->fdes in)
+ (port->fdes out)
+ (port->fdes err))))
+ (release-port-handle in)
+ (release-port-handle out)
+ (release-port-handle err)
+ result))
+
(define-record-type <pipe-info>
(make-pipe-info pid)
pipe-info?
@@ -92,13 +117,13 @@
(define (open-process mode command . args)
"Backwards compatible implementation of the former procedure in
-libguile/posix.c (scm_open_process) replaced by
-scm_piped_process. Executes the program @var{command} with optional
-arguments @var{args} (all strings) in a subprocess. A port to the
-process (based on pipes) is created and returned. @var{mode} specifies
-whether an input, an output or an input-output port to the process is
-created: it should be the value of @code{OPEN_READ}, @code{OPEN_WRITE}
-or @code{OPEN_BOTH}."
+libguile/posix.c (scm_open_process) replaced by scm_piped_process, now
+replaced by scm_spawn_process. Executes the program @var{command} with
+optional arguments @var{args} (all strings) in a subprocess. A port to
+the process (based on pipes) is created and returned. @var{mode}
+specifies whether an input, an output or an input-output port to the
+process is created: it should be the value of @code{OPEN_READ},
+@code{OPEN_WRITE} or @code{OPEN_BOTH}."
(define (unbuffered port)
(setvbuf port 'none)
port)
@@ -107,19 +132,25 @@ or @code{OPEN_BOTH}."
(and ports
(cons (port->fdes (car ports)) (port->fdes (cdr ports)))))
- (let* ((from (and (or (string=? mode OPEN_READ)
- (string=? mode OPEN_BOTH))
- (pipe)))
- (to (and (or (string=? mode OPEN_WRITE)
- (string=? mode OPEN_BOTH))
- (pipe)))
- (pid (piped-process command args
- (fdes-pair from)
- (fdes-pair to))))
+ (let* ((child-to-parent (and (or (string=? mode OPEN_READ)
+ (string=? mode OPEN_BOTH))
+ (pipe)))
+ (parent-to-child (and (or (string=? mode OPEN_WRITE)
+ (string=? mode OPEN_BOTH))
+ (pipe)))
+ (in (or (and=> parent-to-child car) (current-input-port)))
+ (out (or (and=> child-to-parent cdr) (current-output-port)))
+ (pid (spawn command (cons command args)
+ #:in in
+ #:out out)))
+ (when child-to-parent
+ (close (cdr child-to-parent)))
+ (when parent-to-child
+ (close (car parent-to-child)))
;; The original 'open-process' procedure would return unbuffered
;; ports; do the same here.
- (values (and from (unbuffered (car from)))
- (and to (unbuffered (cdr to)))
+ (values (and child-to-parent (unbuffered (car child-to-parent)))
+ (and parent-to-child (unbuffered (cdr parent-to-child)))
pid)))
(define (open-pipe* mode command . args)
@@ -224,10 +255,16 @@ a list of PIDs of the processes executing the @var{commands}."
(pipeline (fold (lambda (from proc prev)
(let* ((to (car prev))
(pids (cdr prev))
- (pid (piped-process (car proc)
- (cdr proc)
- from
- to)))
+ (pid (spawn* (car proc)
+ proc
+ (car to)
+ (cdr from)
+ (port->fdes
+ (port-with-defaults
+ (current-error-port)
+ "w")))))
+ (close-fdes (car to))
+ (close-fdes (cdr from))
(cons from (cons pid pids))))
`(,to)
pipes
diff --git a/test-suite/tests/popen.test b/test-suite/tests/popen.test
index 3df863375..fd810e376 100644
--- a/test-suite/tests/popen.test
+++ b/test-suite/tests/popen.test
@@ -257,18 +257,18 @@ exec 2>~a; read REPLY"
(list (read-string from)
(status:exit-val (cdr (waitpid pid))))))
-(pass-if-equal "piped-process"
+(pass-if-equal "spawn"
42
(status:exit-val
- (cdr (waitpid ((@@ (ice-9 popen) piped-process)
- "./meta/guile" '("-c" "(exit 42)"))))))
+ (cdr (waitpid (spawn
+ "./meta/guile" '("./meta/guile" "-c" "(exit 42)"))))))
-(pass-if-equal "piped-process: with output"
+(pass-if-equal "spawn: with output"
'("foo bar\n" 0)
(let* ((p (pipe))
- (pid ((@@ (ice-9 popen) piped-process) "echo" '("foo" "bar")
- (cons (port->fdes (car p))
- (port->fdes (cdr p))))))
+ (pid (spawn "echo" '("echo" "foo" "bar")
+ #:out (cdr p))))
+ (close (cdr p))
(list (read-string (car p))
(status:exit-val (cdr (waitpid pid))))))
diff --git a/test-suite/tests/posix.test b/test-suite/tests/posix.test
index 500dbb94a..157f21e24 100644
--- a/test-suite/tests/posix.test
+++ 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)))))
-
- (pass-if-equal "exit code for nonexistent file"
- 127 ;aka. EX_NOTFOUND
- (status:exit-val (system* "something-that-does-not-exist")))
-
- (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")))))
+ (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))))
;;
;; crypt
--
2.37.2
L
L
Ludovic Courtès wrote on 29 Nov 2022 16:05
Re: bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
875yex4x9a.fsf_-_@gnu.org
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:

Toggle quote (26 lines)
> * 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.

Toggle quote (4 lines)
> 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).

Toggle quote (5 lines)
> +(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.

Toggle quote (4 lines)
> + (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.

Toggle quote (12 lines)
> +++ 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?

Toggle quote (4 lines)
> - (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’?

Toggle quote (8 lines)
> - (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.

Toggle quote (19 lines)
> + (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’.
J
J
Josselin Poiret wrote on 11 Dec 2022 21:16
(name . Ludovic Courtès)(address . ludo@gnu.org)
87sfhl8zml.fsf@jpoiret.xyz
Hi Ludo,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (3 lines)
> 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.

Toggle quote (3 lines)
> 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.

Toggle quote (10 lines)
> 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.

Toggle quote (4 lines)
> 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.

Toggle quote (14 lines)
>> +++ 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.

Toggle quote (10 lines)
>> - (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
L
L
Ludovic Courtès wrote on 13 Dec 2022 00:49
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
87iligyyh9.fsf@gnu.org
Hello,

Josselin Poiret <dev@jpoiret.xyz> skribis:

Toggle quote (7 lines)
>> 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.

Toggle quote (9 lines)
>> 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!

Toggle quote (36 lines)
>>> +++ 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’.
J
J
Josselin Poiret wrote on 22 Dec 2022 13:49
[PATCH v6 1/3] Add spawn*.
(name . Ludovic Courtès)(address . ludo@gnu.org)
93fe711248cf1ebe86a03bb273ae76e86342ddf8.1671710701.git.dev@jpoiret.xyz
* libguile/posix.c: Include spawn.h from Gnulib.
(do_spawn, scm_spawn_process): New functions.
---
libguile/posix.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
libguile/posix.h | 2 ++
2 files changed, 83 insertions(+)

Toggle diff (114 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index b5352c2c4..e92625483 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -33,6 +33,7 @@
#include <sys/types.h>
#include <uniconv.h>
#include <unistd.h>
+#include <spawn.h>
#ifdef HAVE_SCHED_H
# include <sched.h>
@@ -1426,6 +1427,86 @@ start_child (const char *exec_file, char **exec_argv,
}
#endif
+static int
+do_spawn (char *exec_file, char **exec_argv, char **exec_env, int in, int out, int err)
+{
+ int pid = -1;
+
+ posix_spawn_file_actions_t actions;
+ posix_spawnattr_t *attrp = NULL;
+
+ int max_fd = 1024;
+
+#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
+ {
+ struct rlimit lim = { 0, 0 };
+ if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
+ max_fd = lim.rlim_cur;
+ }
+#endif
+
+ posix_spawn_file_actions_init (&actions);
+
+ int free_fd_slots = 0;
+ int fd_slot[3];
+
+ for (int fdnum = 3;free_fd_slots < 3 && fdnum < max_fd;fdnum++)
+ {
+ if (fdnum != in && fdnum != out && fdnum != err)
+ {
+ fd_slot[free_fd_slots] = fdnum;
+ free_fd_slots++;
+ }
+ }
+
+ /* Move the fds out of the way, so that duplicate fds or fds equal
+ to 0, 1, 2 don't trample each other */
+
+ posix_spawn_file_actions_adddup2 (&actions, in, fd_slot[0]);
+ posix_spawn_file_actions_adddup2 (&actions, out, fd_slot[1]);
+ posix_spawn_file_actions_adddup2 (&actions, err, fd_slot[2]);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[0], 0);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[1], 1);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[2], 2);
+
+ while (--max_fd > 2)
+ posix_spawn_file_actions_addclose (&actions, max_fd);
+
+ if (posix_spawnp (&pid, exec_file, &actions, attrp, exec_argv, environ) != 0)
+ return -1;
+
+ return pid;
+}
+
+SCM_DEFINE (scm_spawn_process, "spawn*", 5, 0, 0,
+ (SCM prog, SCM args, SCM in, SCM out, SCM err),
+"Spawns a new child process executing @var{prog} with arguments\n"
+"@var{args}, with its standard input, output and error file descriptors\n"
+"set to @var{in}, @var{out}, @var{err}.")
+#define FUNC_NAME s_scm_spawn_process
+{
+ int pid;
+ char *exec_file;
+ char **exec_argv;
+ char **exec_env = environ;
+
+ exec_file = scm_to_locale_string (prog);
+ exec_argv = scm_i_allocate_string_pointers (args);
+
+ pid = do_spawn (exec_file, exec_argv, exec_env,
+ scm_to_int (in),
+ scm_to_int (out),
+ scm_to_int (err));
+
+ free (exec_file);
+
+ if (pid == -1)
+ SCM_SYSERROR;
+
+ return scm_from_int (pid);
+}
+#undef FUNC_NAME
+
#ifdef HAVE_START_CHILD
static SCM
scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
diff --git a/libguile/posix.h b/libguile/posix.h
index 6504eaea8..c2703f9ab 100644
--- a/libguile/posix.h
+++ b/libguile/posix.h
@@ -69,6 +69,8 @@ SCM_API SCM scm_tmpnam (void);
SCM_API SCM scm_tmpfile (void);
SCM_API SCM scm_open_pipe (SCM pipestr, SCM modes);
SCM_API SCM scm_close_pipe (SCM port);
+SCM_API SCM scm_spawn_process (SCM prog, SCM args,
+ SCM in, SCM out, SCM err);
SCM_API SCM scm_system_star (SCM cmds);
SCM_API SCM scm_utime (SCM object, SCM actime, SCM modtime,
SCM actimens, SCM modtimens, SCM flags);
--
2.38.1
J
J
Josselin Poiret wrote on 22 Dec 2022 13:49
[PATCH v6 0/3] Move spawning procedures to posix_spawn.
(name . Ludovic Courtès)(address . ludo@gnu.org)
cover.1671710701.git.dev@jpoiret.xyz
Hello Ludo,

Here is hopefully the last reroll of this patchset. First of all, I did not
include the gnulib patch again because it still applies cleanly and it is
extremely large, but it should be applied before those 3 patches.

The first two patches should be applied on the current major release, while the
third one should be applied on the next major release to finish the migration to
spawn.

The first patch adds the new spawn* procedure, using an internal do_spawn
function. The second patch changes system* and piped-process to use this new
function, but it still tries to mimick the old behavior of start_child by
inspecting the possible errnos, and spawning a dummy child that instantly exits
with code 127 in some cases.

The third patch gets rid of those special cases, which makes system* and friends
throw more exceptions instead of having the child fail with exit code 127 (note
that YMMV depending on how spawn is implemented for your system).

I've added docstrings to user-facing Guile procedures, and also did the
micro-optimization we talked about, since I had already factorized do_spawn.

The tests seem to pass both with and without 3.

One nice thing I've noticed is that gnulib has posix_spawn for WinNT as well,
which means it might be okay to remove the dependency on having fork for
system*, among others!

WDYT?

Josselin Poiret (3):
Add spawn*.
Make system* and piped-process internally use spawn.
Move popen and posix procedures to spawn*.

libguile/posix.c | 248 +++++++++++-------------------------
libguile/posix.h | 2 +
module/ice-9/popen.scm | 87 +++++++++----
test-suite/tests/popen.test | 14 +-
test-suite/tests/posix.test | 36 +++---
5 files changed, 161 insertions(+), 226 deletions(-)


base-commit: f3ea8f7fa1d84a559c7bf834fe5b675abe0ae7b8
prerequisite-patch-id: 71184f71260952109165ec62c588c2b646e238f6
--
2.38.1
J
J
Josselin Poiret wrote on 22 Dec 2022 13:49
[PATCH v6 2/3] Make system* and piped-process internally use spawn.
(name . Ludovic Courtès)(address . ludo@gnu.org)
ca962003ea413100f09d8ebe439392627f168f4d.1671710701.git.dev@jpoiret.xyz
* libguile/posix.c (scm_system_star, scm_piped_process): Use do_spawn.
(start_child): Remove function.
---
libguile/posix.c | 181 ++++++++++-------------------------------------
1 file changed, 39 insertions(+), 142 deletions(-)

Toggle diff (249 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index e92625483..f9c36d7ac 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1308,125 +1308,6 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#undef FUNC_NAME
#endif /* HAVE_FORK */
-#ifdef HAVE_FORK
-/* 'renumber_file_descriptor' is a helper function for 'start_child'
- below, and is specialized for that particular environment where it
- doesn't make sense to report errors via exceptions. It uses dup(2)
- to duplicate the file descriptor FD, closes the original FD, and
- returns the new descriptor. If dup(2) fails, print an error message
- to ERR and abort. */
-static int
-renumber_file_descriptor (int fd, int err)
-{
- int new_fd;
-
- do
- new_fd = dup (fd);
- while (new_fd == -1 && errno == EINTR);
-
- if (new_fd == -1)
- {
- /* At this point we are in the child process before exec. We
- cannot safely raise an exception in this environment. */
- const char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
- _exit (127); /* Use exit status 127, as with other exec errors. */
- }
-
- close (fd);
- return new_fd;
-}
-#endif /* HAVE_FORK */
-
-#ifdef HAVE_FORK
-#define HAVE_START_CHILD 1
-/* Since Guile uses threads, we have to be very careful to avoid calling
- functions that are not async-signal-safe in the child. That's why
- this function is implemented in C. */
-static pid_t
-start_child (const char *exec_file, char **exec_argv,
- int reading, int c2p[2], int writing, int p2c[2],
- int in, int out, int err)
-{
- int pid;
- int max_fd = 1024;
-
-#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
- {
- struct rlimit lim = { 0, 0 };
- if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
- max_fd = lim.rlim_cur;
- }
-#endif
-
- pid = fork ();
-
- if (pid != 0)
- /* The parent, with either and error (pid == -1), or the PID of the
- child. Return directly in either case. */
- return pid;
-
- /* The child. */
- if (reading)
- close (c2p[0]);
- if (writing)
- close (p2c[1]);
-
- /* Close all file descriptors in ports inherited from the parent
- except for in, out, and err. Heavy-handed, but robust. */
- while (max_fd--)
- if (max_fd != in && max_fd != out && max_fd != err)
- close (max_fd);
-
- /* Ignore errors on these open() calls. */
- if (in == -1)
- in = open ("/dev/null", O_RDONLY);
- if (out == -1)
- out = open ("/dev/null", O_WRONLY);
- if (err == -1)
- err = open ("/dev/null", O_WRONLY);
-
- if (in > 0)
- {
- if (out == 0)
- out = renumber_file_descriptor (out, err);
- if (err == 0)
- err = renumber_file_descriptor (err, err);
- do dup2 (in, 0); while (errno == EINTR);
- close (in);
- }
- if (out > 1)
- {
- if (err == 1)
- err = renumber_file_descriptor (err, err);
- do dup2 (out, 1); while (errno == EINTR);
- if (out > 2)
- close (out);
- }
- if (err > 2)
- {
- do dup2 (err, 2); while (errno == EINTR);
- close (err);
- }
-
- execvp (exec_file, exec_argv);
-
- /* The exec failed! There is nothing sensible to do. */
- {
- const char *msg = strerror (errno);
- fprintf (fdopen (2, "a"), "In execvp of %s: %s\n",
- exec_file, msg);
- }
-
- /* Use exit status 127, like shells in this case, as per POSIX
- <http://pubs.opengroup.org/onlinepubs/007904875/utilities/xcu_chap02.html#tag_02_09_01_01>. */
- _exit (127);
-
- /* Not reached. */
- return -1;
-}
-#endif
-
static int
do_spawn (char *exec_file, char **exec_argv, char **exec_env, int in, int out, int err)
{
@@ -1507,7 +1388,7 @@ SCM_DEFINE (scm_spawn_process, "spawn*", 5, 0, 0,
}
#undef FUNC_NAME
-#ifdef HAVE_START_CHILD
+#ifdef HAVE_FORK
static SCM
scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
#define FUNC_NAME "piped-process"
@@ -1519,6 +1400,7 @@ scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
int pid;
char *exec_file;
char **exec_argv;
+ char **exec_env = environ;
exec_file = scm_to_locale_string (prog);
exec_argv = scm_i_allocate_string_pointers (scm_cons (prog, args));
@@ -1551,27 +1433,44 @@ scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
in = SCM_FPORT_FDES (port);
}
- pid = start_child (exec_file, exec_argv, reading, c2p, writing, p2c,
- in, out, err);
+ pid = do_spawn (exec_file, exec_argv, exec_env, in, out, err);
+ int errno_save = errno;
if (pid == -1)
{
- int errno_save = errno;
- free (exec_file);
- if (reading)
- {
- close (c2p[0]);
- close (c2p[1]);
- }
- if (writing)
- {
- close (p2c[0]);
- close (p2c[1]);
- }
- errno = errno_save;
- SCM_SYSERROR;
+ /* TODO This is a compatibility shim until the next major release */
+ switch (errno) {
+ /* If the error seemingly comes from fork */
+ case EAGAIN:
+ case ENOMEM:
+ case ENOSYS:
+ free (exec_file);
+
+ if (reading)
+ {
+ close (c2p[0]);
+ }
+ if (writing)
+ {
+ close (p2c[1]);
+ }
+ errno = errno_save;
+ SCM_SYSERROR;
+ break;
+ /* Else create a dummy process that exits with value 127 */
+ default:
+ dprintf (err, "In execvp of %s: %s\n", exec_file,
+ strerror (errno_save));
+ pid = fork ();
+ if (pid == -1)
+ SCM_SYSERROR;
+ if (pid == 0)
+ _exit (127);
+ }
}
+ free (exec_file);
+
if (reading)
close (c2p[1]);
if (writing)
@@ -1651,7 +1550,7 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
return scm_from_int (status);
}
#undef FUNC_NAME
-#endif /* HAVE_START_CHILD */
+#endif /* HAVE_FORK */
#ifdef HAVE_UNAME
SCM_DEFINE (scm_uname, "uname", 0, 0, 0,
@@ -2497,13 +2396,13 @@ SCM_DEFINE (scm_gethostname, "gethostname", 0, 0, 0,
#endif /* HAVE_GETHOSTNAME */
-#ifdef HAVE_START_CHILD
+#ifdef HAVE_FORK
static void
scm_init_popen (void)
{
scm_c_define_gsubr ("piped-process", 2, 2, 0, scm_piped_process);
}
-#endif /* HAVE_START_CHILD */
+#endif /* HAVE_FORK */
void
scm_init_posix ()
@@ -2621,12 +2520,10 @@ scm_init_posix ()
#ifdef HAVE_FORK
scm_add_feature ("fork");
-#endif /* HAVE_FORK */
-#ifdef HAVE_START_CHILD
scm_add_feature ("popen");
scm_c_register_extension ("libguile-" SCM_EFFECTIVE_VERSION,
"scm_init_popen",
(scm_t_extension_init_func) scm_init_popen,
NULL);
-#endif /* HAVE_START_CHILD */
+#endif /* HAVE_FORK */
}
--
2.38.1
J
J
Josselin Poiret wrote on 22 Dec 2022 13:49
[PATCH v6 3/3] Move popen and posix procedures to spawn*.
(name . Ludovic Courtès)(address . ludo@gnu.org)
2423f06c9596dc05ab669247551b5b7bd7c134a0.1671710701.git.dev@jpoiret.xyz
* libguile/posix.c (scm_piped_process, scm_init_popen): 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 | 144 ++++++++----------------------------
module/ice-9/popen.scm | 87 +++++++++++++++-------
test-suite/tests/popen.test | 14 ++--
test-suite/tests/posix.test | 36 ++++-----
4 files changed, 118 insertions(+), 163 deletions(-)

Toggle diff (407 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index f9c36d7ac..1401a9118 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -64,6 +64,7 @@
#include "fports.h"
#include "gettext.h"
#include "gsubr.h"
+#include "ioext.h"
#include "list.h"
#include "modules.h"
#include "numbers.h"
@@ -1388,98 +1389,6 @@ SCM_DEFINE (scm_spawn_process, "spawn*", 5, 0, 0,
}
#undef FUNC_NAME
-#ifdef HAVE_FORK
-static SCM
-scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
-#define FUNC_NAME "piped-process"
-{
- int reading, writing;
- int c2p[2]; /* Child to parent. */
- int p2c[2]; /* Parent to child. */
- int in = -1, out = -1, err = -1;
- int pid;
- char *exec_file;
- char **exec_argv;
- char **exec_env = environ;
-
- exec_file = scm_to_locale_string (prog);
- exec_argv = scm_i_allocate_string_pointers (scm_cons (prog, args));
-
- reading = scm_is_pair (from);
- writing = scm_is_pair (to);
-
- if (reading)
- {
- c2p[0] = scm_to_int (scm_car (from));
- c2p[1] = scm_to_int (scm_cdr (from));
- out = c2p[1];
- }
-
- if (writing)
- {
- p2c[0] = scm_to_int (scm_car (to));
- p2c[1] = scm_to_int (scm_cdr (to));
- in = p2c[0];
- }
-
- {
- SCM port;
-
- if (SCM_OPOUTFPORTP ((port = scm_current_error_port ())))
- err = SCM_FPORT_FDES (port);
- if (out == -1 && SCM_OPOUTFPORTP ((port = scm_current_output_port ())))
- out = SCM_FPORT_FDES (port);
- if (in == -1 && SCM_OPINFPORTP ((port = scm_current_input_port ())))
- in = SCM_FPORT_FDES (port);
- }
-
- pid = do_spawn (exec_file, exec_argv, exec_env, in, out, err);
- int errno_save = errno;
-
- if (pid == -1)
- {
- /* TODO This is a compatibility shim until the next major release */
- switch (errno) {
- /* If the error seemingly comes from fork */
- case EAGAIN:
- case ENOMEM:
- case ENOSYS:
- free (exec_file);
-
- if (reading)
- {
- close (c2p[0]);
- }
- if (writing)
- {
- close (p2c[1]);
- }
- errno = errno_save;
- SCM_SYSERROR;
- break;
- /* Else create a dummy process that exits with value 127 */
- default:
- dprintf (err, "In execvp of %s: %s\n", exec_file,
- strerror (errno_save));
- pid = fork ();
- if (pid == -1)
- SCM_SYSERROR;
- if (pid == 0)
- _exit (127);
- }
- }
-
- free (exec_file);
-
- if (reading)
- close (c2p[1]);
- if (writing)
- close (p2c[0]);
-
- return scm_from_int (pid);
-}
-#undef FUNC_NAME
-
static void
restore_sigaction (SCM pair)
{
@@ -1501,6 +1410,15 @@ scm_dynwind_sigaction (int sig, SCM handler, SCM flags)
SCM_F_WIND_EXPLICITLY);
}
+static int
+port_to_fd_with_default (SCM port, int mode)
+{
+ if (!SCM_FPORTP (port))
+ return open_or_open64 ("/dev/null", mode);
+ return SCM_FPORT_FDES (port);
+
+}
+
SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
(SCM args),
"Execute the command indicated by @var{args}. The first element must\n"
@@ -1521,13 +1439,14 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
"Example: (system* \"echo\" \"foo\" \"bar\")")
#define FUNC_NAME s_scm_system_star
{
- SCM prog, pid;
- int status, wait_result;
+ int pid, status, wait_result;
+
+ int in, out, err;
+ char *exec_file;
+ char **exec_argv;
if (scm_is_null (args))
SCM_WRONG_NUM_ARGS ();
- prog = scm_car (args);
- args = scm_cdr (args);
scm_dynwind_begin (0);
/* Make sure the child can't kill us (as per normal system call). */
@@ -1540,8 +1459,23 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
SCM_UNDEFINED);
#endif
- pid = scm_piped_process (prog, args, SCM_UNDEFINED, SCM_UNDEFINED);
- SCM_SYSCALL (wait_result = waitpid (scm_to_int (pid), &status, 0));
+ exec_file = scm_to_locale_string (scm_car (args));
+ exec_argv = scm_i_allocate_string_pointers (args);
+
+ in = port_to_fd_with_default (scm_current_input_port (), O_RDONLY);
+ out = port_to_fd_with_default (scm_current_output_port (), O_WRONLY);
+ err = port_to_fd_with_default (scm_current_error_port (), O_WRONLY);
+
+ pid = do_spawn (exec_file, exec_argv, environ, in, out, err);
+ if (pid == -1)
+ {
+ int errno_save = errno;
+ free (exec_file);
+ errno = errno_save;
+ SCM_SYSERROR;
+ }
+
+ SCM_SYSCALL (wait_result = waitpid (pid, &status, 0));
if (wait_result == -1)
SCM_SYSERROR;
@@ -1550,7 +1484,6 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
return scm_from_int (status);
}
#undef FUNC_NAME
-#endif /* HAVE_FORK */
#ifdef HAVE_UNAME
SCM_DEFINE (scm_uname, "uname", 0, 0, 0,
@@ -2396,14 +2329,6 @@ SCM_DEFINE (scm_gethostname, "gethostname", 0, 0, 0,
#endif /* HAVE_GETHOSTNAME */
-#ifdef HAVE_FORK
-static void
-scm_init_popen (void)
-{
- scm_c_define_gsubr ("piped-process", 2, 2, 0, scm_piped_process);
-}
-#endif /* HAVE_FORK */
-
void
scm_init_posix ()
{
@@ -2520,10 +2445,5 @@ scm_init_posix ()
#ifdef HAVE_FORK
scm_add_feature ("fork");
- scm_add_feature ("popen");
- scm_c_register_extension ("libguile-" SCM_EFFECTIVE_VERSION,
- "scm_init_popen",
- (scm_t_extension_init_func) scm_init_popen,
- NULL);
#endif /* HAVE_FORK */
}
diff --git a/module/ice-9/popen.scm b/module/ice-9/popen.scm
index e638726a4..547f56d5f 100644
--- a/module/ice-9/popen.scm
+++ b/module/ice-9/popen.scm
@@ -25,11 +25,34 @@
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-9)
#:export (port/pid-table open-pipe* open-pipe close-pipe open-input-pipe
- open-output-pipe open-input-output-pipe pipeline))
+ open-output-pipe open-input-output-pipe pipeline spawn))
-(eval-when (expand load eval)
- (load-extension (string-append "libguile-" (effective-version))
- "scm_init_popen"))
+(define (port-with-defaults port default-mode)
+ (if (file-port? port)
+ port
+ (open-file "/dev/null" default-mode)))
+
+(define* (spawn exec-file argv #:key
+ (in (current-input-port))
+ (out (current-output-port))
+ (err (current-error-port)))
+ "Spawns a new child process executing @var{prog} with arguments
+@var{args}, with its standard input, output and error file descriptors
+set to @var{in}, @var{out}, @var{err}."
+ (let* ((in (port-with-defaults in "r"))
+ (out (port-with-defaults out "w"))
+ (err (port-with-defaults err "w"))
+ ;; Increment port revealed counts while to prevent ports GC'ing and
+ ;; closing the associated fds while we spawn the process.
+ (result (spawn* exec-file
+ argv
+ (port->fdes in)
+ (port->fdes out)
+ (port->fdes err))))
+ (release-port-handle in)
+ (release-port-handle out)
+ (release-port-handle err)
+ result))
(define-record-type <pipe-info>
(make-pipe-info pid)
@@ -92,13 +115,13 @@
(define (open-process mode command . args)
"Backwards compatible implementation of the former procedure in
-libguile/posix.c (scm_open_process) replaced by
-scm_piped_process. Executes the program @var{command} with optional
-arguments @var{args} (all strings) in a subprocess. A port to the
-process (based on pipes) is created and returned. @var{mode} specifies
-whether an input, an output or an input-output port to the process is
-created: it should be the value of @code{OPEN_READ}, @code{OPEN_WRITE}
-or @code{OPEN_BOTH}."
+libguile/posix.c (scm_open_process) replaced by scm_piped_process, now
+replaced by scm_spawn_process. Executes the program @var{command} with
+optional arguments @var{args} (all strings) in a subprocess. A port to
+the process (based on pipes) is created and returned. @var{mode}
+specifies whether an input, an output or an input-output port to the
+process is created: it should be the value of @code{OPEN_READ},
+@code{OPEN_WRITE} or @code{OPEN_BOTH}."
(define (unbuffered port)
(setvbuf port 'none)
port)
@@ -107,19 +130,25 @@ or @code{OPEN_BOTH}."
(and ports
(cons (port->fdes (car ports)) (port->fdes (cdr ports)))))
- (let* ((from (and (or (string=? mode OPEN_READ)
- (string=? mode OPEN_BOTH))
- (pipe)))
- (to (and (or (string=? mode OPEN_WRITE)
- (string=? mode OPEN_BOTH))
- (pipe)))
- (pid (piped-process command args
- (fdes-pair from)
- (fdes-pair to))))
+ (let* ((child-to-parent (and (or (string=? mode OPEN_READ)
+ (string=? mode OPEN_BOTH))
+ (pipe)))
+ (parent-to-child (and (or (string=? mode OPEN_WRITE)
+ (string=? mode OPEN_BOTH))
+ (pipe)))
+ (in (or (and=> parent-to-child car) (current-input-port)))
+ (out (or (and=> child-to-parent cdr) (current-output-port)))
+ (pid (spawn command (cons command args)
+ #:in in
+ #:out out)))
+ (when child-to-parent
+ (close (cdr child-to-parent)))
+ (when parent-to-child
+ (close (car parent-to-child)))
;; The original 'open-process' procedure would return unbuffered
;; ports; do the same here.
- (values (and from (unbuffered (car from)))
- (and to (unbuffered (cdr to)))
+ (values (and child-to-parent (unbuffered (car child-to-parent)))
+ (and parent-to-child (unbuffered (cdr parent-to-child)))
pid)))
(define (open-pipe* mode command . args)
@@ -224,10 +253,16 @@ a list of PIDs of the processes executing the @var{commands}."
(pipeline (fold (lambda (from proc prev)
(let* ((to (car prev))
(pids (cdr prev))
- (pid (piped-process (car proc)
- (cdr proc)
- from
- to)))
+ (pid (spawn* (car proc)
+ proc
+ (car to)
+ (cdr from)
+ (port->fdes
+ (port-with-defaults
+ (current-error-port)
+ "w")))))
+ (close-fdes (car to))
+ (close-fdes (cdr from))
(cons from (cons pid pids))))
`(,to)
pipes
diff --git a/test-suite/tests/popen.test b/test-suite/tests/popen.test
index 3df863375..fd810e376 100644
--- a/test-suite/tests/popen.test
+++ b/test-suite/tests/popen.test
@@ -257,18 +257,18 @@ exec 2>~a; read REPLY"
(list (read-string from)
(status:exit-val (cdr (waitpid pid))))))
-(pass-if-equal "piped-process"
+(pass-if-equal "spawn"
42
(status:exit-val
- (cdr (waitpid ((@@ (ice-9 popen) piped-process)
- "./meta/guile" '("-c" "(exit 42)"))))))
+ (cdr (waitpid (spawn
+ "./meta/guile" '("./meta/guile" "-c" "(exit 42)"))))))
-(pass-if-equal "piped-process: with output"
+(pass-if-equal "spawn: with output"
'("foo bar\n" 0)
(let* ((p (pipe))
- (pid ((@@ (ice-9 popen) piped-process) "echo" '("foo" "bar")
- (cons (port->fdes (car p))
- (port->fdes (cdr p))))))
+ (pid (spawn "echo" '("echo" "foo" "bar")
+ #:out (cdr p))))
+ (close (cdr p))
(list (read-string (car p))
(status:exit-val (cdr (waitpid pid))))))
diff --git a/test-suite/tests/posix.test b/test-suite/tests/posix.test
index bfc6f168e..5c971f4f7 100644
--- a/test-suite/tests/posix.test
+++ b/test-suite/tests/posix.test
@@ -340,24 +340,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)))))
-
- (pass-if-equal "exit code for nonexistent file"
- 127 ;aka. EX_NOTFOUND
- (status:exit-val (system* "something-that-does-not-exist")))
-
- (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")))))
+ (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))))
;;
;; crypt
--
2.38.1
L
L
Ludovic Courtès wrote on 23 Dec 2022 11:53
Re: bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
87y1qyz909.fsf_-_@gnu.org
Hi Josselin,

Josselin Poiret <dev@jpoiret.xyz> skribis:

Toggle quote (4 lines)
> Here is hopefully the last reroll of this patchset. First of all, I did not
> include the gnulib patch again because it still applies cleanly and it is
> extremely large, but it should be applied before those 3 patches.

Yay!

Toggle quote (4 lines)
> The first two patches should be applied on the current major release, while the
> third one should be applied on the next major release to finish the migration to
> spawn.

I pushed it to ‘wip-posix-spawn’ along with fixups I’m proposing, mostly
along the lines of what I suggested in

• Avoiding the extra ‘fork’ in ‘system*’ upon error;

• Keep ‘scm_spawn_process’ internal.

I also added Andrew Whatson’s patch from

If that’s fine with you, I can squash the “fixup!” commits and merge the
branch. Let me know!

Earlier we agreed it’d be nice to expose ‘spawn*’/‘primitive-spawn’. I
still think it’s a good idea, but the interface would need some work IMO
to be more generally useful. In essence, we could provide something
similar to ‘fork+exec-command’ in the Shepherd, where one can pass
environment variables, stdin/stdout/stderr, etc., all that with keyword
arguments and reasonable defaults.

It’s a bit of extra work though so we can discuss that later,
separately.

Thank you!

Ludo’.
J
J
Josselin Poiret wrote on 23 Dec 2022 18:15
(name . Ludovic Courtès)(address . ludo@gnu.org)
87v8m2jb3c.fsf@jpoiret.xyz
Hi Ludo, thanks for the quick review and fixes.

Toggle quote (4 lines)
> I pushed it to ‘wip-posix-spawn’ along with fixups I’m proposing, mostly
> along the lines of what I suggested in
> <https://issues.guix.gnu.org/52835#18>:

Nice but also see below.

Toggle quote (3 lines)
> I also added Andrew Whatson’s patch from
> <https://issues.guix.gnu.org/59321>.

Great, hadn't see that one go by!

Toggle quote (10 lines)
> If that’s fine with you, I can squash the “fixup!” commits and merge the
> branch. Let me know!
>
> Earlier we agreed it’d be nice to expose ‘spawn*’/‘primitive-spawn’. I
> still think it’s a good idea, but the interface would need some work IMO
> to be more generally useful. In essence, we could provide something
> similar to ‘fork+exec-command’ in the Shepherd, where one can pass
> environment variables, stdin/stdout/stderr, etc., all that with keyword
> arguments and reasonable defaults.

I've just polished it up a bit: the `spawn*` procedure defined in C now
takes another argument, a list of environment variables. I think this
interface is good enough to cover most use cases, if anyone needs
something more complicated they should go through their own C code.
I've added a convenience module (ice-9 spawn) with a `spawn` procedure
in it, which takes an optional argument list which defaults to just the
executable, and optional environment variables as well as in, out and
err ports. I also think everything in (ice-9 popen) should be migrated
on the next major release, as well as being re-implemented in terms of
`spawn` purely, so that the change is immediately noticeable.

We're reaching the bike-shedding time now, but IMHO having such a
`spawn*` exposed to the user seems fine, it's a pretty simple "raw"
interface with fdes, and there is a convenience `spawn` function that is
nicer for users.

Do we need to add a documentation page as well?

Best,
--
Josselin Poiret
J
J
Josselin Poiret wrote on 23 Dec 2022 18:17
[PATCH v7 1/2] Add spawn* and spawn.
(name . Ludovic Courtès)(address . ludo@gnu.org)
388cbca36850d1837b3c478d9bd9dd5e222215b0.1671815759.git.dev@jpoiret.xyz
* libguile/posix.c: Include spawn.h from Gnulib.
(do_spawn, scm_spawn_process): New functions.
* module/ice-9/spawn.scm: New file
(spawn): New procedure.
---
libguile/posix.c | 82 ++++++++++++++++++++++++++++++++++++++++++
libguile/posix.h | 2 ++
module/ice-9/spawn.scm | 54 ++++++++++++++++++++++++++++
3 files changed, 138 insertions(+)
create mode 100644 module/ice-9/spawn.scm

Toggle diff (177 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index b5352c2c4..52dc11e57 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -33,6 +33,7 @@
#include <sys/types.h>
#include <uniconv.h>
#include <unistd.h>
+#include <spawn.h>
#ifdef HAVE_SCHED_H
# include <sched.h>
@@ -1426,6 +1427,87 @@ start_child (const char *exec_file, char **exec_argv,
}
#endif
+static pid_t
+do_spawn (char *exec_file, char **exec_argv, char **exec_env, int in, int out, int err)
+{
+ pid_t pid = -1;
+
+ posix_spawn_file_actions_t actions;
+ posix_spawnattr_t *attrp = NULL;
+
+ int max_fd = 1024;
+
+#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
+ {
+ struct rlimit lim = { 0, 0 };
+ if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
+ max_fd = lim.rlim_cur;
+ }
+#endif
+
+ posix_spawn_file_actions_init (&actions);
+
+ int free_fd_slots = 0;
+ int fd_slot[3];
+
+ for (int fdnum = 3;free_fd_slots < 3 && fdnum < max_fd;fdnum++)
+ {
+ if (fdnum != in && fdnum != out && fdnum != err)
+ {
+ fd_slot[free_fd_slots] = fdnum;
+ free_fd_slots++;
+ }
+ }
+
+ /* Move the fds out of the way, so that duplicate fds or fds equal
+ to 0, 1, 2 don't trample each other */
+
+ posix_spawn_file_actions_adddup2 (&actions, in, fd_slot[0]);
+ posix_spawn_file_actions_adddup2 (&actions, out, fd_slot[1]);
+ posix_spawn_file_actions_adddup2 (&actions, err, fd_slot[2]);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[0], 0);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[1], 1);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[2], 2);
+
+ while (--max_fd > 2)
+ posix_spawn_file_actions_addclose (&actions, max_fd);
+
+ if (posix_spawnp (&pid, exec_file, &actions, attrp, exec_argv, exec_env) != 0)
+ return -1;
+
+ return pid;
+}
+
+SCM_DEFINE (scm_spawn_process, "spawn*", 6, 0, 0,
+ (SCM prog, SCM args, SCM env, SCM in, SCM out, SCM err),
+ "Spawns a new child process executing @var{prog} with arguments\n"
+ "@var{args}, with its standard input, output and error file descriptors\n"
+ "set to @var{in}, @var{out}, @var{err}, and environment to @var{env}.")
+#define FUNC_NAME s_scm_spawn_process
+{
+ int pid;
+ char *exec_file;
+ char **exec_argv;
+ char **exec_env;
+
+ exec_file = scm_to_locale_string (prog);
+ exec_argv = scm_i_allocate_string_pointers (args);
+ exec_env = scm_i_allocate_string_pointers (env);
+
+ pid = do_spawn (exec_file, exec_argv, exec_env,
+ scm_to_int (in),
+ scm_to_int (out),
+ scm_to_int (err));
+
+ free (exec_file);
+
+ if (pid == -1)
+ SCM_SYSERROR;
+
+ return scm_from_int (pid);
+}
+#undef FUNC_NAME
+
#ifdef HAVE_START_CHILD
static SCM
scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
diff --git a/libguile/posix.h b/libguile/posix.h
index 6504eaea8..35c502bc1 100644
--- a/libguile/posix.h
+++ b/libguile/posix.h
@@ -69,6 +69,8 @@ SCM_API SCM scm_tmpnam (void);
SCM_API SCM scm_tmpfile (void);
SCM_API SCM scm_open_pipe (SCM pipestr, SCM modes);
SCM_API SCM scm_close_pipe (SCM port);
+SCM_API SCM scm_spawn_process (SCM prog, SCM args, SCM env,
+ SCM in, SCM out, SCM err);
SCM_API SCM scm_system_star (SCM cmds);
SCM_API SCM scm_utime (SCM object, SCM actime, SCM modtime,
SCM actimens, SCM modtimens, SCM flags);
diff --git a/module/ice-9/spawn.scm b/module/ice-9/spawn.scm
new file mode 100644
index 000000000..ae4f54efa
--- /dev/null
+++ b/module/ice-9/spawn.scm
@@ -0,0 +1,54 @@
+;; Spawning programs
+
+;;;; Copyright (C) 2022
+;;;; Free Software Foundation, Inc.
+;;;;
+;;;; This library is free software; you can redistribute it and/or
+;;;; modify it under the terms of the GNU Lesser General Public
+;;;; License as published by the Free Software Foundation; either
+;;;; version 3 of the License, or (at your option) any later version.
+;;;;
+;;;; This library is distributed in the hope that it will be useful,
+;;;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+;;;; Lesser General Public License for more details.
+;;;;
+;;;; You should have received a copy of the GNU Lesser General Public
+;;;; License along with this library; if not, write to the Free Software
+;;;; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+;;;;
+
+(define-module (ice-9 spawn)
+ #:export (spawn))
+
+(define (port-with-defaults port default-mode)
+ (if (file-port? port)
+ port
+ (open-file "/dev/null" default-mode)))
+
+(define* (spawn exec-file
+ #:optional (args (list exec-file))
+ #:key (env (environ))
+ (in (current-input-port))
+ (out (current-output-port))
+ (err (current-error-port)))
+ "Spawns a new process running the program @var{exec} with arguments
+@var{args}, in the environment specified by the list of environment
+variable strings @var{env}, and with standard input, output and error
+set to the ports specified by @var{in}, @var{out}, @var{err}. Note that
+the last part only works with fd-backed ports."
+ (let* ((in (port-with-defaults in "r"))
+ (out (port-with-defaults out "w"))
+ (err (port-with-defaults err "w"))
+ ;; Increment port revealed counts while to prevent ports GC'ing and
+ ;; closing the associated fds while we spawn the process.
+ (result (spawn* exec-file
+ args
+ env
+ (port->fdes in)
+ (port->fdes out)
+ (port->fdes err))))
+ (release-port-handle in)
+ (release-port-handle out)
+ (release-port-handle err)
+ result))

base-commit: 4711d45176e9b75cef43699ed514669276af62fe
--
2.38.1
J
J
Josselin Poiret wrote on 23 Dec 2022 18:17
[PATCH v7 2/2] Make system* and piped-process internally use spawn.
(name . Ludovic Courtès)(address . ludo@gnu.org)
ba1e2cbd132a99b32d16fa8b3b89bf750217a89a.1671815759.git.dev@jpoiret.xyz
* libguile/posix.c (scm_system_star, scm_piped_process): Use do_spawn.
(start_child): Remove function.

Co-authored-by: Ludovic Courtès <ludo@gnu.org>
---
libguile/posix.c | 233 ++++++++++++++++-------------------------------
1 file changed, 78 insertions(+), 155 deletions(-)

Toggle diff (330 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index 52dc11e57..ecc2b186e 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -77,6 +77,7 @@
#include "threads.h"
#include "values.h"
#include "vectors.h"
+#include "verify.h"
#include "version.h"
#if (SCM_ENABLE_DEPRECATED == 1)
@@ -95,6 +96,13 @@
# define WIFEXITED(stat_val) (((stat_val) & 255) == 0)
#endif
+#ifndef W_EXITCODE
+/* Macro for constructing a status value. Found in glibc. */
+# define W_EXITCODE(ret, sig) ((ret) << 8 | (sig))
+#endif
+verify (WEXITSTATUS (W_EXITCODE (127, 0)) == 127);
+
+
#include <signal.h>
#ifdef HAVE_GRP_H
@@ -1308,125 +1316,6 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#undef FUNC_NAME
#endif /* HAVE_FORK */
-#ifdef HAVE_FORK
-/* 'renumber_file_descriptor' is a helper function for 'start_child'
- below, and is specialized for that particular environment where it
- doesn't make sense to report errors via exceptions. It uses dup(2)
- to duplicate the file descriptor FD, closes the original FD, and
- returns the new descriptor. If dup(2) fails, print an error message
- to ERR and abort. */
-static int
-renumber_file_descriptor (int fd, int err)
-{
- int new_fd;
-
- do
- new_fd = dup (fd);
- while (new_fd == -1 && errno == EINTR);
-
- if (new_fd == -1)
- {
- /* At this point we are in the child process before exec. We
- cannot safely raise an exception in this environment. */
- const char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
- _exit (127); /* Use exit status 127, as with other exec errors. */
- }
-
- close (fd);
- return new_fd;
-}
-#endif /* HAVE_FORK */
-
-#ifdef HAVE_FORK
-#define HAVE_START_CHILD 1
-/* Since Guile uses threads, we have to be very careful to avoid calling
- functions that are not async-signal-safe in the child. That's why
- this function is implemented in C. */
-static pid_t
-start_child (const char *exec_file, char **exec_argv,
- int reading, int c2p[2], int writing, int p2c[2],
- int in, int out, int err)
-{
- int pid;
- int max_fd = 1024;
-
-#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
- {
- struct rlimit lim = { 0, 0 };
- if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
- max_fd = lim.rlim_cur;
- }
-#endif
-
- pid = fork ();
-
- if (pid != 0)
- /* The parent, with either and error (pid == -1), or the PID of the
- child. Return directly in either case. */
- return pid;
-
- /* The child. */
- if (reading)
- close (c2p[0]);
- if (writing)
- close (p2c[1]);
-
- /* Close all file descriptors in ports inherited from the parent
- except for in, out, and err. Heavy-handed, but robust. */
- while (max_fd--)
- if (max_fd != in && max_fd != out && max_fd != err)
- close (max_fd);
-
- /* Ignore errors on these open() calls. */
- if (in == -1)
- in = open ("/dev/null", O_RDONLY);
- if (out == -1)
- out = open ("/dev/null", O_WRONLY);
- if (err == -1)
- err = open ("/dev/null", O_WRONLY);
-
- if (in > 0)
- {
- if (out == 0)
- out = renumber_file_descriptor (out, err);
- if (err == 0)
- err = renumber_file_descriptor (err, err);
- do dup2 (in, 0); while (errno == EINTR);
- close (in);
- }
- if (out > 1)
- {
- if (err == 1)
- err = renumber_file_descriptor (err, err);
- do dup2 (out, 1); while (errno == EINTR);
- if (out > 2)
- close (out);
- }
- if (err > 2)
- {
- do dup2 (err, 2); while (errno == EINTR);
- close (err);
- }
-
- execvp (exec_file, exec_argv);
-
- /* The exec failed! There is nothing sensible to do. */
- {
- const char *msg = strerror (errno);
- fprintf (fdopen (2, "a"), "In execvp of %s: %s\n",
- exec_file, msg);
- }
-
- /* Use exit status 127, like shells in this case, as per POSIX
- <http://pubs.opengroup.org/onlinepubs/007904875/utilities/xcu_chap02.html#tag_02_09_01_01>. */
- _exit (127);
-
- /* Not reached. */
- return -1;
-}
-#endif
-
static pid_t
do_spawn (char *exec_file, char **exec_argv, char **exec_env, int in, int out, int err)
{
@@ -1508,18 +1397,18 @@ SCM_DEFINE (scm_spawn_process, "spawn*", 6, 0, 0,
}
#undef FUNC_NAME
-#ifdef HAVE_START_CHILD
-static SCM
-scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
+#ifdef HAVE_FORK
+static int
+piped_process (pid_t *pid, SCM prog, SCM args, SCM from, SCM to)
#define FUNC_NAME "piped-process"
{
int reading, writing;
int c2p[2]; /* Child to parent. */
int p2c[2]; /* Parent to child. */
int in = -1, out = -1, err = -1;
- int pid;
char *exec_file;
char **exec_argv;
+ char **exec_env = environ;
exec_file = scm_to_locale_string (prog);
exec_argv = scm_i_allocate_string_pointers (scm_cons (prog, args));
@@ -1552,32 +1441,57 @@ scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
in = SCM_FPORT_FDES (port);
}
- pid = start_child (exec_file, exec_argv, reading, c2p, writing, p2c,
- in, out, err);
-
- if (pid == -1)
- {
- int errno_save = errno;
- free (exec_file);
- if (reading)
- {
- close (c2p[0]);
- close (c2p[1]);
- }
- if (writing)
- {
- close (p2c[0]);
- close (p2c[1]);
- }
- errno = errno_save;
- SCM_SYSERROR;
- }
+ *pid = do_spawn (exec_file, exec_argv, exec_env, in, out, err);
+ int errno_save = (*pid < 0) ? errno : 0;
if (reading)
close (c2p[1]);
if (writing)
close (p2c[0]);
+ if (*pid == -1)
+ switch (errno_save)
+ {
+ /* Errors that seemingly come from fork. */
+ case EAGAIN:
+ case ENOMEM:
+ case ENOSYS:
+ errno = err;
+ free (exec_file);
+ SCM_SYSERROR;
+ break;
+
+ default: /* ENOENT, etc. */
+ /* Create a dummy process that exits with value 127. */
+ dprintf (err, "In execvp of %s: %s\n", exec_file,
+ strerror (errno_save));
+ }
+
+ free (exec_file);
+
+ return errno_save;
+}
+#undef FUNC_NAME
+
+static SCM
+scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
+#define FUNC_NAME "piped-process"
+{
+ pid_t pid;
+
+ (void) piped_process (&pid, prog, args, from, to);
+ if (pid == -1)
+ {
+ /* Create a dummy process that exits with value 127 to mimic the
+ previous fork + exec implementation. TODO: This is a
+ compatibility shim to remove in the next stable series. */
+ pid = fork ();
+ if (pid == -1)
+ SCM_SYSERROR;
+ if (pid == 0)
+ _exit (127);
+ }
+
return scm_from_int (pid);
}
#undef FUNC_NAME
@@ -1623,8 +1537,9 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
"Example: (system* \"echo\" \"foo\" \"bar\")")
#define FUNC_NAME s_scm_system_star
{
- SCM prog, pid;
- int status, wait_result;
+ SCM prog;
+ pid_t pid;
+ int err, status, wait_result;
if (scm_is_null (args))
SCM_WRONG_NUM_ARGS ();
@@ -1642,17 +1557,27 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
SCM_UNDEFINED);
#endif
- pid = scm_piped_process (prog, args, SCM_UNDEFINED, SCM_UNDEFINED);
- SCM_SYSCALL (wait_result = waitpid (scm_to_int (pid), &status, 0));
- if (wait_result == -1)
- SCM_SYSERROR;
+ err = piped_process (&pid, prog, args,
+ SCM_UNDEFINED, SCM_UNDEFINED);
+ if (err != 0)
+ /* ERR might be ENOENT or similar. For backward compatibility with
+ the previous implementation based on fork + exec, pretend the
+ child process exited with code 127. TODO: Remove this
+ compatibility shim in the next stable series. */
+ status = W_EXITCODE (127, 0);
+ else
+ {
+ SCM_SYSCALL (wait_result = waitpid (pid, &status, 0));
+ if (wait_result == -1)
+ SCM_SYSERROR;
+ }
scm_dynwind_end ();
return scm_from_int (status);
}
#undef FUNC_NAME
-#endif /* HAVE_START_CHILD */
+#endif /* HAVE_FORK */
#ifdef HAVE_UNAME
SCM_DEFINE (scm_uname, "uname", 0, 0, 0,
@@ -2498,13 +2423,13 @@ SCM_DEFINE (scm_gethostname, "gethostname", 0, 0, 0,
#endif /* HAVE_GETHOSTNAME */
-#ifdef HAVE_START_CHILD
+#ifdef HAVE_FORK
static void
scm_init_popen (void)
{
scm_c_define_gsubr ("piped-process", 2, 2, 0, scm_piped_process);
}
-#endif /* HAVE_START_CHILD */
+#endif /* HAVE_FORK */
void
scm_init_posix ()
@@ -2622,12 +2547,10 @@ scm_init_posix ()
#ifdef HAVE_FORK
scm_add_feature ("fork");
-#endif /* HAVE_FORK */
-#ifdef HAVE_START_CHILD
scm_add_feature ("popen");
scm_c_register_extension ("libguile-" SCM_EFFECTIVE_VERSION,
"scm_init_popen",
(scm_t_extension_init_func) scm_init_popen,
NULL);
-#endif /* HAVE_START_CHILD */
+#endif /* HAVE_FORK */
}
--
2.38.1
L
L
Ludovic Courtès wrote on 25 Dec 2022 17:58
Re: bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
87a63bxvxh.fsf@gnu.org
Hi Josselin,

Josselin Poiret <dev@jpoiret.xyz> skribis:

Toggle quote (7 lines)
> I've added a convenience module (ice-9 spawn) with a `spawn` procedure
> in it, which takes an optional argument list which defaults to just the
> executable, and optional environment variables as well as in, out and
> err ports. I also think everything in (ice-9 popen) should be migrated
> on the next major release, as well as being re-implemented in terms of
> `spawn` purely, so that the change is immediately noticeable.

OK.

Toggle quote (5 lines)
> We're reaching the bike-shedding time now, but IMHO having such a
> `spawn*` exposed to the user seems fine, it's a pretty simple "raw"
> interface with fdes, and there is a convenience `spawn` function that is
> nicer for users.

‘spawn*’ is expressive enough, but a procedure with 6 positional
arguments cannot be exposed as-is IMO.

One tiny thing that’s missing is a boolean to choose between
‘posix_spawn’ and ‘posix_spawnp’.

So we need either ‘spawn’ defined in Scheme as you did, or directly use
‘scm_c_bind_keyword_arguments’ right in ‘scm_spawn_process’ (info
"(guile) Keyword Procedures"), to avoid ending up with two procedures.
I’m fine either way.

Toggle quote (2 lines)
> Do we need to add a documentation page as well?

Yes, please. :-) I realize I’m asking a lot of things, so let me know
if you’d like to share the workload somehow.

Doc could go in the “Processes” section, or possibly in a new section
we’d add right after it, “Spawning Programs”. We should probably add a
“@quotation Note” in the doc of ‘primitive-fork’ that briefly explains
why people should probably use ‘spawn’ rather than fork + exec.

Some cosmetic suggestions follow…

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 25 Dec 2022 18:03
(name . Josselin Poiret)(address . dev@jpoiret.xyz)(address . 52835@debbugs.gnu.org)
873593xvnv.fsf_-_@gnu.org
Josselin Poiret <dev@jpoiret.xyz> skribis:

Toggle quote (10 lines)
> * libguile/posix.c: Include spawn.h from Gnulib.
> (do_spawn, scm_spawn_process): New functions.
> * module/ice-9/spawn.scm: New file
> (spawn): New procedure.
> ---
> libguile/posix.c | 82 ++++++++++++++++++++++++++++++++++++++++++
> libguile/posix.h | 2 ++
> module/ice-9/spawn.scm | 54 ++++++++++++++++++++++++++++
> 3 files changed, 138 insertions(+)

The new module should be added to ‘am/bootstrap.am’.

Toggle quote (3 lines)
> +SCM_API SCM scm_spawn_process (SCM prog, SCM args, SCM env,
> + SCM in, SCM out, SCM err);

Let’s keep it ‘SCM_INTERNAL’.

Toggle quote (7 lines)
> +(define* (spawn exec-file
> + #:optional (args (list exec-file))
> + #:key (env (environ))
> + (in (current-input-port))
> + (out (current-output-port))
> + (err (current-error-port)))

s/exec-file/program/
s/args/arguments/
s/env/environment/

s/in/standard-input/
s/out/standard-output/
s/err/standard-error/

Maybe we could allow these two be either ports or file descriptors?

Toggle quote (17 lines)
> + "Spawns a new process running the program @var{exec} with arguments
> +@var{args}, in the environment specified by the list of environment
> +variable strings @var{env}, and with standard input, output and error
> +set to the ports specified by @var{in}, @var{out}, @var{err}. Note that
> +the last part only works with fd-backed ports."
> + (let* ((in (port-with-defaults in "r"))
> + (out (port-with-defaults out "w"))
> + (err (port-with-defaults err "w"))
> + ;; Increment port revealed counts while to prevent ports GC'ing and
> + ;; closing the associated fds while we spawn the process.
> + (result (spawn* exec-file
> + args
> + env
> + (port->fdes in)
> + (port->fdes out)
> + (port->fdes err))))

I believe ‘spawn*’ is unbound here because it’s defined by
‘scm_init_popen’, which is called within the (ice-9 popen) module.

Ludo’.
L
L
Ludovic Courtès wrote on 25 Dec 2022 18:04
(name . Josselin Poiret)(address . dev@jpoiret.xyz)(address . 52835@debbugs.gnu.org)
87y1qvwh2j.fsf_-_@gnu.org
Josselin Poiret <dev@jpoiret.xyz> skribis:

Toggle quote (3 lines)
> * libguile/posix.c (scm_system_star, scm_piped_process): Use do_spawn.
> (start_child): Remove function.

LGTM, thanks!
J
J
Josselin Poiret wrote on 7 Jan 2023 17:07
(name . Ludovic Courtès)(address . ludo@gnu.org)
878rie9vmi.fsf@jpoiret.xyz
Hi Ludo,

Happy new year!

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (11 lines)
> ‘spawn*’ is expressive enough, but a procedure with 6 positional
> arguments cannot be exposed as-is IMO.
>
> One tiny thing that’s missing is a boolean to choose between
> ‘posix_spawn’ and ‘posix_spawnp’.
>
> So we need either ‘spawn’ defined in Scheme as you did, or directly use
> ‘scm_c_bind_keyword_arguments’ right in ‘scm_spawn_process’ (info
> "(guile) Keyword Procedures"), to avoid ending up with two procedures.
> I’m fine either way.

I've done the above by using the C functions to bind keyword arguments,
and added the #:use-path? keyword argument. One annoying thing though
is that since it uses keyword arguments, the first line of the
documentation generated by SCM_DEFINE only mentions "spawn program
. keyword_args".

Toggle quote (10 lines)
>> Do we need to add a documentation page as well?
>
> Yes, please. :-) I realize I’m asking a lot of things, so let me know
> if you’d like to share the workload somehow.
>
> Doc could go in the “Processes” section, or possibly in a new section
> we’d add right after it, “Spawning Programs”. We should probably add a
> “@quotation Note” in the doc of ‘primitive-fork’ that briefly explains
> why people should probably use ‘spawn’ rather than fork + exec.

I've opted to put it right below primitive-fork, and slightly rewrite
the part about pipes in primitive-fork's description to also mention
spawn. Let me know if the documentation is not descriptive enough.

WDYT?
--
Josselin Poiret
J
J
Josselin Poiret wrote on 7 Jan 2023 17:07
[PATCH v8 1/2] Add spawn.
(address . 52835@debbugs.gnu.org)
54884b48615fa3291c637eda80e02f94c359485f.1673107558.git.dev@jpoiret.xyz
* libguile/posix.c: Include spawn.h from Gnulib.
(do_spawn, scm_spawn_process): New functions.
---
doc/ref/posix.texi | 34 ++++++++--
libguile/posix.c | 162 ++++++++++++++++++++++++++++++++++++++++++++-
libguile/posix.h | 1 +
3 files changed, 192 insertions(+), 5 deletions(-)

Toggle diff (258 lines)
diff --git a/doc/ref/posix.texi b/doc/ref/posix.texi
index bde0f150c..36e1f5040 100644
--- a/doc/ref/posix.texi
+++ b/doc/ref/posix.texi
@@ -2045,15 +2045,41 @@ safe to call after a multithreaded fork, which is a very limited set.
Guile issues a warning if it detects a fork from a multi-threaded
program.
-If you are going to @code{exec} soon after forking, the procedures in
-@code{(ice-9 popen)} may be useful to you, as they fork and exec within
-an async-signal-safe function carefully written to ensure robust program
-behavior, even in the presence of threads. @xref{Pipes}, for more.
+@quotation Note
+If you are only looking to fork+exec with some pipes set up, using pipes
+or the more primitive @code{spawn} will be more robust (e.g. in the
+presence of threads), and is more portable. @xref{Pipes} for more.
+@end quotation
This procedure has been renamed from @code{fork} to avoid a naming conflict
with the scsh fork.
@end deffn
+@deffn {Scheme Procedure} spawn program [#:args=(list program)] @
+ [#:env=(environ)] [#:in=(fileno (current-input-port))] @
+ [#:out=(fileno (current-output-port))] @
+ [#:err=(fileno (current-error-port))] @
+ [#:use-path?=#t]
+
+Spawns a new child process executing @var{program} with argument list
+@var{args} (which must include the name of the executable as a first
+element), with its environment variables set to @var{env} and standard
+input/output/error file descriptors to @var{in}, @var{out}, @var{err},
+after closing all other file descriptors. When @var{use-path?} is
+false, @var{program} should be a path to an executable file, but when
+@var{use-path?} is true, the environment variable @code{PATH} is
+searched to find the corresponding executable.
+
+Failure to exec in the child may be caught early and reported as an
+exception, or the child may also exit with return code 127, depending on
+how spawn is implemented for the specific system. You therefore must be
+able to handle both cases.
+
+The return value is the pid of the spawned child process.
+
+This procedure is portable and should be thread-safe.
+@end deffn
+
@deffn {Scheme Procedure} nice incr
@deffnx {C Function} scm_nice (incr)
@cindex process priority
diff --git a/libguile/posix.c b/libguile/posix.c
index b5352c2c4..f79875075 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -33,6 +33,7 @@
#include <sys/types.h>
#include <uniconv.h>
#include <unistd.h>
+#include <spawn.h>
#ifdef HAVE_SCHED_H
# include <sched.h>
@@ -63,6 +64,7 @@
#include "fports.h"
#include "gettext.h"
#include "gsubr.h"
+#include "keywords.h"
#include "list.h"
#include "modules.h"
#include "numbers.h"
@@ -1426,6 +1428,158 @@ start_child (const char *exec_file, char **exec_argv,
}
#endif
+static pid_t
+do_spawn (char *exec_file, char **exec_argv, char **exec_env,
+ int in, int out, int err, int spawnp)
+{
+ pid_t pid = -1;
+
+ posix_spawn_file_actions_t actions;
+ posix_spawnattr_t *attrp = NULL;
+
+ int max_fd = 1024;
+
+#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
+ {
+ struct rlimit lim = { 0, 0 };
+ if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
+ max_fd = lim.rlim_cur;
+ }
+#endif
+
+ posix_spawn_file_actions_init (&actions);
+
+ int free_fd_slots = 0;
+ int fd_slot[3];
+
+ for (int fdnum = 3;free_fd_slots < 3 && fdnum < max_fd;fdnum++)
+ {
+ if (fdnum != in && fdnum != out && fdnum != err)
+ {
+ fd_slot[free_fd_slots] = fdnum;
+ free_fd_slots++;
+ }
+ }
+
+ /* Move the fds out of the way, so that duplicate fds or fds equal
+ to 0, 1, 2 don't trample each other */
+
+ posix_spawn_file_actions_adddup2 (&actions, in, fd_slot[0]);
+ posix_spawn_file_actions_adddup2 (&actions, out, fd_slot[1]);
+ posix_spawn_file_actions_adddup2 (&actions, err, fd_slot[2]);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[0], 0);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[1], 1);
+ posix_spawn_file_actions_adddup2 (&actions, fd_slot[2], 2);
+
+ while (--max_fd > 2)
+ posix_spawn_file_actions_addclose (&actions, max_fd);
+
+ int res = -1;
+ if (spawnp)
+ res = posix_spawnp (&pid, exec_file, &actions, attrp,
+ exec_argv, exec_env);
+ else
+ res = posix_spawn (&pid, exec_file, &actions, attrp,
+ exec_argv, exec_env);
+ if (res != 0)
+ return -1;
+
+ return pid;
+}
+
+SCM k_args, k_env, k_in, k_out, k_err, k_use_path;
+
+SCM_DEFINE (scm_spawn_process, "spawn", 1, 0, 1,
+ (SCM program, SCM keyword_args),
+ "Spawns a new child process executing @var{program} with no arguments.\n\n"
+ "If the boolean keyword argument @code{#:use-path?} is provided, it\n"
+ "selects whether the @code{PATH} environment variable should be\n"
+ "inspected to find @var{program}. It is true by default.\n\n"
+ "If the keyword arguments @code{#:args}, @code{#:env}, are provided,\n"
+ "they respectively modify the arguments or the environment of the\n"
+ "spawning program.\n\n"
+ "If the keyword arguments @code{#:in}, @code{#:out} or @code{#:err}\n"
+ "are provided, they respectively modify the default input, output\n"
+ "and error file descriptor of the spawning program to these values.")
+#define FUNC_NAME s_scm_spawn_process
+{
+ SCM args, env, in_scm, out_scm, err_scm, use_path;
+ args = SCM_UNDEFINED;
+ env = SCM_UNDEFINED;
+ in_scm = SCM_UNDEFINED;
+ out_scm = SCM_UNDEFINED;
+ err_scm = SCM_UNDEFINED;
+ use_path = SCM_BOOL_T;
+
+ scm_c_bind_keyword_arguments (FUNC_NAME, keyword_args, 0,
+ k_args, &args,
+ k_env, &env,
+ k_in, &in_scm,
+ k_out, &out_scm,
+ k_err, &err_scm,
+ k_use_path, &use_path,
+ SCM_UNDEFINED);
+
+ int pid = -1;
+ char *exec_file;
+ char **exec_argv;
+ char **exec_env;
+ int in, out, err;
+
+ exec_file = scm_to_locale_string (program);
+
+ if (SCM_UNBNDP (args))
+ {
+ /* We use scm_gc_malloc here because that's the same as what
+ scm_i_allocate_string_pointers would do. */
+ exec_argv = scm_gc_malloc (2 * sizeof (char *),
+ "string pointers");
+ exec_argv[0] = exec_file;
+ exec_argv[1] = NULL;
+ }
+ else
+ {
+ exec_argv = scm_i_allocate_string_pointers (args);
+ if (exec_argv[0] == NULL)
+ {
+ free (exec_file);
+ scm_misc_error (FUNC_NAME, "Argument list must not be empty.",
+ SCM_EOL);
+ }
+ }
+
+ if (SCM_UNBNDP (env))
+ exec_env = environ;
+ else
+ exec_env = scm_i_allocate_string_pointers (env);
+
+ if (SCM_UNBNDP (in_scm))
+ in = SCM_FPORT_FDES (scm_current_input_port ());
+ else
+ in = scm_to_int (in_scm);
+
+ if (SCM_UNBNDP (out_scm))
+ out = SCM_FPORT_FDES (scm_current_output_port ());
+ else
+ out = scm_to_int (out_scm);
+
+ if (SCM_UNBNDP (err_scm))
+ err = SCM_FPORT_FDES (scm_current_error_port ());
+ else
+ err = scm_to_int (err_scm);
+
+ pid = do_spawn (exec_file, exec_argv, exec_env,
+ in, out, err, scm_to_bool (use_path));
+
+ free (exec_file);
+
+ if (pid == -1)
+ SCM_SYSERROR;
+
+ return scm_from_int (pid);
+}
+#undef FUNC_NAME
+
#ifdef HAVE_START_CHILD
static SCM
scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
@@ -2547,5 +2701,11 @@ scm_init_posix ()
"scm_init_popen",
(scm_t_extension_init_func) scm_init_popen,
NULL);
-#endif /* HAVE_START_CHILD */
+#endif /* HAVE_FORK */
+ k_args = scm_from_utf8_keyword ("args");
+ k_env = scm_from_utf8_keyword ("env");
+ k_in = scm_from_utf8_keyword ("in");
+ k_out = scm_from_utf8_keyword ("out");
+ k_err = scm_from_utf8_keyword ("err");
+ k_use_path = scm_from_utf8_keyword ("use-path?");
}
diff --git a/libguile/posix.h b/libguile/posix.h
index 6504eaea8..5eeafd4cb 100644
--- a/libguile/posix.h
+++ b/libguile/posix.h
@@ -69,6 +69,7 @@ SCM_API SCM scm_tmpnam (void);
SCM_API SCM scm_tmpfile (void);
SCM_API SCM scm_open_pipe (SCM pipestr, SCM modes);
SCM_API SCM scm_close_pipe (SCM port);
+SCM_API SCM scm_spawn_process (SCM prog, SCM keyword_args);
SCM_API SCM scm_system_star (SCM cmds);
SCM_API SCM scm_utime (SCM object, SCM actime, SCM modtime,
SCM actimens, SCM modtimens, SCM flags);

base-commit: 4711d45176e9b75cef43699ed514669276af62fe
--
2.38.1
J
J
Josselin Poiret wrote on 7 Jan 2023 17:07
[PATCH v8 2/2] Make system* and piped-process internally use spawn.
(address . 52835@debbugs.gnu.org)
dfd3ffaae3a82e803fff1c116a4eb5e8c5bc2588.1673107558.git.dev@jpoiret.xyz
* libguile/posix.c (scm_system_star, scm_piped_process): Use do_spawn.
(start_child): Remove function.

Co-authored-by: Ludovic Courtès <ludo@gnu.org>
---
libguile/posix.c | 231 ++++++++++++++++-------------------------------
1 file changed, 77 insertions(+), 154 deletions(-)

Toggle diff (325 lines)
diff --git a/libguile/posix.c b/libguile/posix.c
index f79875075..2c6c3c84a 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -78,6 +78,7 @@
#include "threads.h"
#include "values.h"
#include "vectors.h"
+#include "verify.h"
#include "version.h"
#if (SCM_ENABLE_DEPRECATED == 1)
@@ -96,6 +97,13 @@
# define WIFEXITED(stat_val) (((stat_val) & 255) == 0)
#endif
+#ifndef W_EXITCODE
+/* Macro for constructing a status value. Found in glibc. */
+# define W_EXITCODE(ret, sig) ((ret) << 8 | (sig))
+#endif
+verify (WEXITSTATUS (W_EXITCODE (127, 0)) == 127);
+
+
#include <signal.h>
#ifdef HAVE_GRP_H
@@ -1309,125 +1317,6 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#undef FUNC_NAME
#endif /* HAVE_FORK */
-#ifdef HAVE_FORK
-/* 'renumber_file_descriptor' is a helper function for 'start_child'
- below, and is specialized for that particular environment where it
- doesn't make sense to report errors via exceptions. It uses dup(2)
- to duplicate the file descriptor FD, closes the original FD, and
- returns the new descriptor. If dup(2) fails, print an error message
- to ERR and abort. */
-static int
-renumber_file_descriptor (int fd, int err)
-{
- int new_fd;
-
- do
- new_fd = dup (fd);
- while (new_fd == -1 && errno == EINTR);
-
- if (new_fd == -1)
- {
- /* At this point we are in the child process before exec. We
- cannot safely raise an exception in this environment. */
- const char *msg = strerror (errno);
- fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
- _exit (127); /* Use exit status 127, as with other exec errors. */
- }
-
- close (fd);
- return new_fd;
-}
-#endif /* HAVE_FORK */
-
-#ifdef HAVE_FORK
-#define HAVE_START_CHILD 1
-/* Since Guile uses threads, we have to be very careful to avoid calling
- functions that are not async-signal-safe in the child. That's why
- this function is implemented in C. */
-static pid_t
-start_child (const char *exec_file, char **exec_argv,
- int reading, int c2p[2], int writing, int p2c[2],
- int in, int out, int err)
-{
- int pid;
- int max_fd = 1024;
-
-#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
- {
- struct rlimit lim = { 0, 0 };
- if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
- max_fd = lim.rlim_cur;
- }
-#endif
-
- pid = fork ();
-
- if (pid != 0)
- /* The parent, with either and error (pid == -1), or the PID of the
- child. Return directly in either case. */
- return pid;
-
- /* The child. */
- if (reading)
- close (c2p[0]);
- if (writing)
- close (p2c[1]);
-
- /* Close all file descriptors in ports inherited from the parent
- except for in, out, and err. Heavy-handed, but robust. */
- while (max_fd--)
- if (max_fd != in && max_fd != out && max_fd != err)
- close (max_fd);
-
- /* Ignore errors on these open() calls. */
- if (in == -1)
- in = open ("/dev/null", O_RDONLY);
- if (out == -1)
- out = open ("/dev/null", O_WRONLY);
- if (err == -1)
- err = open ("/dev/null", O_WRONLY);
-
- if (in > 0)
- {
- if (out == 0)
- out = renumber_file_descriptor (out, err);
- if (err == 0)
- err = renumber_file_descriptor (err, err);
- do dup2 (in, 0); while (errno == EINTR);
- close (in);
- }
- if (out > 1)
- {
- if (err == 1)
- err = renumber_file_descriptor (err, err);
- do dup2 (out, 1); while (errno == EINTR);
- if (out > 2)
- close (out);
- }
- if (err > 2)
- {
- do dup2 (err, 2); while (errno == EINTR);
- close (err);
- }
-
- execvp (exec_file, exec_argv);
-
- /* The exec failed! There is nothing sensible to do. */
- {
- const char *msg = strerror (errno);
- fprintf (fdopen (2, "a"), "In execvp of %s: %s\n",
- exec_file, msg);
- }
-
- /* Use exit status 127, like shells in this case, as per POSIX
- <http://pubs.opengroup.org/onlinepubs/007904875/utilities/xcu_chap02.html#tag_02_09_01_01>. */
- _exit (127);
-
- /* Not reached. */
- return -1;
-}
-#endif
-
static pid_t
do_spawn (char *exec_file, char **exec_argv, char **exec_env,
int in, int out, int err, int spawnp)
@@ -1580,18 +1469,18 @@ SCM_DEFINE (scm_spawn_process, "spawn", 1, 0, 1,
}
#undef FUNC_NAME
-#ifdef HAVE_START_CHILD
-static SCM
-scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
+#ifdef HAVE_FORK
+static int
+piped_process (pid_t *pid, SCM prog, SCM args, SCM from, SCM to)
#define FUNC_NAME "piped-process"
{
int reading, writing;
int c2p[2]; /* Child to parent. */
int p2c[2]; /* Parent to child. */
int in = -1, out = -1, err = -1;
- int pid;
char *exec_file;
char **exec_argv;
+ char **exec_env = environ;
exec_file = scm_to_locale_string (prog);
exec_argv = scm_i_allocate_string_pointers (scm_cons (prog, args));
@@ -1624,32 +1513,57 @@ scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
in = SCM_FPORT_FDES (port);
}
- pid = start_child (exec_file, exec_argv, reading, c2p, writing, p2c,
- in, out, err);
-
- if (pid == -1)
- {
- int errno_save = errno;
- free (exec_file);
- if (reading)
- {
- close (c2p[0]);
- close (c2p[1]);
- }
- if (writing)
- {
- close (p2c[0]);
- close (p2c[1]);
- }
- errno = errno_save;
- SCM_SYSERROR;
- }
+ *pid = do_spawn (exec_file, exec_argv, exec_env, in, out, err, 1);
+ int errno_save = (*pid < 0) ? errno : 0;
if (reading)
close (c2p[1]);
if (writing)
close (p2c[0]);
+ if (*pid == -1)
+ switch (errno_save)
+ {
+ /* Errors that seemingly come from fork. */
+ case EAGAIN:
+ case ENOMEM:
+ case ENOSYS:
+ errno = err;
+ free (exec_file);
+ SCM_SYSERROR;
+ break;
+
+ default: /* ENOENT, etc. */
+ /* Create a dummy process that exits with value 127. */
+ dprintf (err, "In execvp of %s: %s\n", exec_file,
+ strerror (errno_save));
+ }
+
+ free (exec_file);
+
+ return errno_save;
+}
+#undef FUNC_NAME
+
+static SCM
+scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
+#define FUNC_NAME "piped-process"
+{
+ pid_t pid;
+
+ (void) piped_process (&pid, prog, args, from, to);
+ if (pid == -1)
+ {
+ /* Create a dummy process that exits with value 127 to mimic the
+ previous fork + exec implementation. TODO: This is a
+ compatibility shim to remove in the next stable series. */
+ pid = fork ();
+ if (pid == -1)
+ SCM_SYSERROR;
+ if (pid == 0)
+ _exit (127);
+ }
+
return scm_from_int (pid);
}
#undef FUNC_NAME
@@ -1695,8 +1609,9 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
"Example: (system* \"echo\" \"foo\" \"bar\")")
#define FUNC_NAME s_scm_system_star
{
- SCM prog, pid;
- int status, wait_result;
+ SCM prog;
+ pid_t pid;
+ int err, status, wait_result;
if (scm_is_null (args))
SCM_WRONG_NUM_ARGS ();
@@ -1714,17 +1629,27 @@ SCM_DEFINE (scm_system_star, "system*", 0, 0, 1,
SCM_UNDEFINED);
#endif
- pid = scm_piped_process (prog, args, SCM_UNDEFINED, SCM_UNDEFINED);
- SCM_SYSCALL (wait_result = waitpid (scm_to_int (pid), &status, 0));
- if (wait_result == -1)
- SCM_SYSERROR;
+ err = piped_process (&pid, prog, args,
+ SCM_UNDEFINED, SCM_UNDEFINED);
+ if (err != 0)
+ /* ERR might be ENOENT or similar. For backward compatibility with
+ the previous implementation based on fork + exec, pretend the
+ child process exited with code 127. TODO: Remove this
+ compatibility shim in the next stable series. */
+ status = W_EXITCODE (127, 0);
+ else
+ {
+ SCM_SYSCALL (wait_result = waitpid (pid, &status, 0));
+ if (wait_result == -1)
+ SCM_SYSERROR;
+ }
scm_dynwind_end ();
return scm_from_int (status);
}
#undef FUNC_NAME
-#endif /* HAVE_START_CHILD */
+#endif /* HAVE_FORK */
#ifdef HAVE_UNAME
SCM_DEFINE (scm_uname, "uname", 0, 0, 0,
@@ -2570,13 +2495,13 @@ SCM_DEFINE (scm_gethostname, "gethostname", 0, 0, 0,
#endif /* HAVE_GETHOSTNAME */
-#ifdef HAVE_START_CHILD
+#ifdef HAVE_FORK
static void
scm_init_popen (void)
{
scm_c_define_gsubr ("piped-process", 2, 2, 0, scm_piped_process);
}
-#endif /* HAVE_START_CHILD */
+#endif /* HAVE_FORK */
void
scm_init_posix ()
@@ -2694,8 +2619,6 @@ scm_init_posix ()
#ifdef HAVE_FORK
scm_add_feature ("fork");
-#endif /* HAVE_FORK */
-#ifdef HAVE_START_CHILD
scm_add_feature ("popen");
scm_c_register_extension ("libguile-" SCM_EFFECTIVE_VERSION,
"scm_init_popen",
--
2.38.1
L
L
Ludovic Courtès wrote on 12 Jan 2023 23:02
Re: bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
(name . Josselin Poiret)(address . dev@jpoiret.xyz)
871qnzh0ns.fsf@gnu.org
Hi Josselin, and a happy new year full of good hacks! :-)

Josselin Poiret <dev@jpoiret.xyz> skribis:

Toggle quote (6 lines)
> I've done the above by using the C functions to bind keyword arguments,
> and added the #:use-path? keyword argument. One annoying thing though
> is that since it uses keyword arguments, the first line of the
> documentation generated by SCM_DEFINE only mentions "spawn program
> . keyword_args".

That’s OK.

Toggle quote (4 lines)
> I've opted to put it right below primitive-fork, and slightly rewrite
> the part about pipes in primitive-fork's description to also mention
> spawn. Let me know if the documentation is not descriptive enough.

It looks good to me.

A few suggestions of mine got lost, so I pushed a new ‘wip-posix-spawn’
with additional commits taking those into account and adding tests. The
changes in this v8.1 are:

- make 'arguments' positional rather than keyword
- rename keyword arguments to avoid abbreviations
- add example in the manual
- mark 'scm_spawn_process' as SCM_INTERNAL
- add tests

Because this is now a core binding and no longer in (ice-9 spawn), I
hope ‘spawn’ is not clashing with someone else’s library, though the
worst that could happen is a run-time warning “module X overrides core
binding 'spawn'”.

I’m very pleased with the branch and could finally merge if you agree!
(I’ll put myself as co-author and take the blame for half of the bugs.
:-))

Ah yes, one thing that came to mind: we could change #:input #f to mean
“close file descriptor 0”, and so on. Let’s first merge this, though.

Thoughts?

Ludo’.
A
A
Andrew Whatson wrote on 13 Jan 2023 02:11
Re: [PATCH 0/2] Fix spawning a child not setting standard fds properly
(address . 52835@debbugs.gnu.org)
83e7661b7b89afbe406184b3c12a0310@tailcall.au
Hello Josselin & Ludo,

Thanks for your work on this, posix_spawn is a nice improvement!

My comments on the changes in the wip-posix-spawn branch follow.

In doc/ref/posix.texi:

Toggle quote (8 lines)
> @quotation Note
> If you are only looking to fork+exec with some pipes set up, using
> pipes
> or the more @code{spawn} procedure described below will be more robust
> (in particular in multi-threaded contexts), more portable, and usually
> more efficient.
> @end quotation

Should be "... the more primitive @code{spawn} procedure".

Toggle quote (11 lines)
> @deffn {Scheme Procedure} spawn @var{program} @var{arguments} @
> [#:environment=(environ)] @
> [#:input=(current-input-port)] @
> [#:output=(current-output-port)] @
> [#:error=(current-error-port)] @
> [#:search-path?=#t]
> Spawn a new child process executing @var{program} with the
> given @var{arguments}, a list of one or more strings, and
> return its PID. Raise a @code{system-error} exception if
> @var{program} could not be found or could not be executed.

The description of arguments as "a list of one or more strings" is
surprising, I think some users might expect to be able to pass zero
arguments, but we don't explain what the single argument should be.

An earlier version of the docs for this function clarified this with
"(which must include the name of the executable as a first element)".
The manual for exec(3) says "the first argument, by convention, should
point to the filename associated with the file being executed."

Maybe we could clarify in a second paragraph: "By convention, the first
element of @var{arguments} should be the name of the program being
executed. If calling a C program, for example, this will be passed as
the value for @code{argv[0]}."

In libguile/posix.c, piped_process, around line 1565:

Toggle quote (18 lines)
> if (*pid == -1)
> switch (errno_save)
> {
> /* Errors that seemingly come from fork. */
> case EAGAIN:
> case ENOMEM:
> case ENOSYS:
> errno = err;
> free (exec_file);
> SCM_SYSERROR;
> break;
>
> default: /* ENOENT, etc. */
> /* Create a dummy process that exits with value 127. */
> dprintf (err, "In execvp of %s: %s\n", exec_file,
> strerror (errno_save));
> }

The "dummy process" comment here is misleading, this function simply
returns the error code. Its callers 'scm_piped_process' and
'scm_system_star' are responsible for simulating exit code 127, and are
already well commented. Probably just remove this redundant comment.

Otherwise, looks good to me :)

Thanks again!

Cheers,
Andrew
L
L
Ludovic Courtès wrote on 13 Jan 2023 16:20
Re: bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
(name . Andrew Whatson)(address . whatson@tailcall.au)
87k01qbgw3.fsf@gnu.org
Hi Andrew & Josselin,

Andrew Whatson <whatson@tailcall.au> skribis:

Toggle quote (4 lines)
> Thanks for your work on this, posix_spawn is a nice improvement!
>
> My comments on the changes in the wip-posix-spawn branch follow.

As discussed on IRC, I took your comments into account. I also added a
‘system*’ test based on what Josselin provided at the beginning of this
bug report (let’s not forget we were initially fixing an actual bug :-))
and updated ‘NEWS’:

527c257d6 Make 'system*' and 'piped-process' internally use 'spawn'.
551929e4f Add 'spawn'.
edfca3b7e Update gnulib to 0.1.5414-8204d and add posix_spawn, posix_spawnp.

Let me know if you notice anything fishy!

Thanks a lot, Josselin, for your hard work and for your patience.

Ludo’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 52835@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 52835
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch