Re: [libiberty patch] PEX-unix forking
On 08/20/2018 05:36 PM, Ian Lance Taylor wrote:
As a matter of style I don't personally like the pattern in which a
condition has both tests and actions. It's too easy to miss the
action. I would prefer to see this more like the original code:
if (!bad_fn && in != STDIN_FILE_NO)
{
if (close(in) < 0)
bad_fn = "close";
}
This is OK with those changes.
Fair enough, committing the attached. I've elided the unnecessary braces, as
there are no trailing elses in this case.
nathan
--
Nathan Sidwell
2018-08-20 Nathan Sidwell
* pex-unix.c (pex_child_error): Delete.
(pex_unix_exec_child): Commonize error paths to single message &
exit.
Index: pex-unix.c
===
--- pex-unix.c (revision 263676)
+++ pex-unix.c (working copy)
@@ -298,8 +298,6 @@ pex_wait (struct pex_obj *obj, pid_t pid
#endif /* ! defined (HAVE_WAITPID) */
#endif /* ! defined (HAVE_WAIT4) */
-static void pex_child_error (struct pex_obj *, const char *, const char *, int)
- ATTRIBUTE_NORETURN;
static int pex_unix_open_read (struct pex_obj *, const char *, int);
static int pex_unix_open_write (struct pex_obj *, const char *, int, int);
static pid_t pex_unix_exec_child (struct pex_obj *, int, const char *,
@@ -366,28 +364,6 @@ pex_unix_close (struct pex_obj *obj ATTR
return close (fd);
}
-/* Report an error from a child process. We don't use stdio routines,
- because we might be here due to a vfork call. */
-
-static void
-pex_child_error (struct pex_obj *obj, const char *executable,
- const char *errmsg, int err)
-{
- int retval = 0;
-#define writeerr(s) retval |= (write (STDERR_FILE_NO, s, strlen (s)) < 0)
- writeerr (obj->pname);
- writeerr (": error trying to exec '");
- writeerr (executable);
- writeerr ("': ");
- writeerr (errmsg);
- writeerr (": ");
- writeerr (xstrerror (err));
- writeerr ("\n");
-#undef writeerr
- /* Exit with -2 if the error output failed, too. */
- _exit (retval == 0 ? -1 : -2);
-}
-
/* Execute a child. */
#if defined(HAVE_SPAWNVE) && defined(HAVE_SPAWNVPE)
@@ -592,21 +568,22 @@ pex_unix_exec_child (struct pex_obj *obj
int in, int out, int errdes,
int toclose, const char **errmsg, int *err)
{
- pid_t pid;
+ pid_t pid = -1;
/* We declare these to be volatile to avoid warnings from gcc about
them being clobbered by vfork. */
- volatile int sleep_interval;
+ volatile int sleep_interval = 1;
volatile int retries;
/* We vfork and then set environ in the child before calling execvp.
This clobbers the parent's environ so we need to restore it.
It would be nice to use one of the exec* functions that takes an
- environment as a parameter, but that may have portability issues. */
+ environment as a parameter, but that may have portability
+ issues. */
char **save_environ = environ;
- sleep_interval = 1;
- pid = -1;
+ const char *bad_fn = NULL;
+
for (retries = 0; retries < 4; ++retries)
{
pid = vfork ();
@@ -625,57 +602,76 @@ pex_unix_exec_child (struct pex_obj *obj
case 0:
/* Child process. */
- if (in != STDIN_FILE_NO)
+ if (!bad_fn && in != STDIN_FILE_NO)
{
if (dup2 (in, STDIN_FILE_NO) < 0)
- pex_child_error (obj, executable, "dup2", errno);
- if (close (in) < 0)
- pex_child_error (obj, executable, "close", errno);
+ bad_fn = "dup2";
+ else if (close (in) < 0)
+ bad_fn = "close";
}
- if (out != STDOUT_FILE_NO)
+ if (!bad_fn && out != STDOUT_FILE_NO)
{
if (dup2 (out, STDOUT_FILE_NO) < 0)
- pex_child_error (obj, executable, "dup2", errno);
- if (close (out) < 0)
- pex_child_error (obj, executable, "close", errno);
+ bad_fn = "dup2";
+ else if (close (out) < 0)
+ bad_fn = "close";
}
- if (errdes != STDERR_FILE_NO)
+ if (!bad_fn && errdes != STDERR_FILE_NO)
{
if (dup2 (errdes, STDERR_FILE_NO) < 0)
- pex_child_error (obj, executable, "dup2", errno);
- if (close (errdes) < 0)
- pex_child_error (obj, executable, "close", errno);
+ bad_fn = "dup2";
+ else if (close (errdes) < 0)
+ bad_fn = "close";
}
- if (toclose >= 0)
+ if (!bad_fn && toclose >= 0)
{
if (close (toclose) < 0)
- pex_child_error (obj, executable, "close", errno);
+ bad_fn = "close";
}
- if ((flags & PEX_STDERR_TO_STDOUT) != 0)
+ if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0)
{
if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0)
- pex_child_error (obj, executable, "dup2", errno);
+ bad_fn = "dup2";
}
-
- if (env)
+ if (!bad_fn)
{
- /* NOTE: In a standard vfork implementation this clobbers the
- parent's copy of environ "too" (in reality there's only one copy).
- This is ok as we restore it below. */
- environ = (char**) env;
+ if (env)
+ /* NOTE: In a standard vfork implementation this clobbers
+
Re: [libiberty patch] PEX-unix forking
On Mon, Aug 20, 2018 at 10:38 AM, Nathan Sidwell wrote:
>
> This is the first of a pair of patches I've had on the modules branch for a
> while. They improve the error behaviour in the case of child failure when
> vfork is the forking mechanism.
>
> This one commonizes the error paths in the parent and child to a pair of
> single blocks that print or return the error respectively. Currently each
> error case calls pex_child_error, or returns the error in the parent.
>
> The patch records the name of a failing system call, and uses that to print
> or return the error. It doesn't change functionality but will simplify the
> next patch that does.
>
> booted on x86_64-linux (which uses vfork), ok?
+ if (!bad_fn && in != STDIN_FILE_NO && close (in) < 0)
+bad_fn = "close";
As a matter of style I don't personally like the pattern in which a
condition has both tests and actions. It's too easy to miss the
action. I would prefer to see this more like the original code:
if (!bad_fn && in != STDIN_FILE_NO)
{
if (close(in) < 0)
bad_fn = "close";
}
This is OK with those changes.
Thanks.
Ian
[libiberty patch] PEX-unix forking
This is the first of a pair of patches I've had on the modules branch
for a while. They improve the error behaviour in the case of child
failure when vfork is the forking mechanism.
This one commonizes the error paths in the parent and child to a pair of
single blocks that print or return the error respectively. Currently
each error case calls pex_child_error, or returns the error in the parent.
The patch records the name of a failing system call, and uses that to
print or return the error. It doesn't change functionality but will
simplify the next patch that does.
booted on x86_64-linux (which uses vfork), ok?
nathan
--
Nathan Sidwell
2018-08-20 Nathan Sidwell
* pex-unix.c (pex_child_error): Delete.
(pex_unix_exec_child): Commonize error paths to single message &
exit.
Index: pex-unix.c
===
--- pex-unix.c (revision 263667)
+++ pex-unix.c (working copy)
@@ -298,8 +298,6 @@ pex_wait (struct pex_obj *obj, pid_t pid
#endif /* ! defined (HAVE_WAITPID) */
#endif /* ! defined (HAVE_WAIT4) */
-static void pex_child_error (struct pex_obj *, const char *, const char *, int)
- ATTRIBUTE_NORETURN;
static int pex_unix_open_read (struct pex_obj *, const char *, int);
static int pex_unix_open_write (struct pex_obj *, const char *, int, int);
static pid_t pex_unix_exec_child (struct pex_obj *, int, const char *,
@@ -366,28 +364,6 @@ pex_unix_close (struct pex_obj *obj ATTR
return close (fd);
}
-/* Report an error from a child process. We don't use stdio routines,
- because we might be here due to a vfork call. */
-
-static void
-pex_child_error (struct pex_obj *obj, const char *executable,
- const char *errmsg, int err)
-{
- int retval = 0;
-#define writeerr(s) retval |= (write (STDERR_FILE_NO, s, strlen (s)) < 0)
- writeerr (obj->pname);
- writeerr (": error trying to exec '");
- writeerr (executable);
- writeerr ("': ");
- writeerr (errmsg);
- writeerr (": ");
- writeerr (xstrerror (err));
- writeerr ("\n");
-#undef writeerr
- /* Exit with -2 if the error output failed, too. */
- _exit (retval == 0 ? -1 : -2);
-}
-
/* Execute a child. */
#if defined(HAVE_SPAWNVE) && defined(HAVE_SPAWNVPE)
@@ -592,21 +568,22 @@ pex_unix_exec_child (struct pex_obj *obj
int in, int out, int errdes,
int toclose, const char **errmsg, int *err)
{
- pid_t pid;
+ pid_t pid = -1;
/* We declare these to be volatile to avoid warnings from gcc about
them being clobbered by vfork. */
- volatile int sleep_interval;
+ volatile int sleep_interval = 1;
volatile int retries;
/* We vfork and then set environ in the child before calling execvp.
This clobbers the parent's environ so we need to restore it.
It would be nice to use one of the exec* functions that takes an
- environment as a parameter, but that may have portability issues. */
+ environment as a parameter, but that may have portability
+ issues. */
char **save_environ = environ;
- sleep_interval = 1;
- pid = -1;
+ const char *bad_fn = NULL;
+
for (retries = 0; retries < 4; ++retries)
{
pid = vfork ();
@@ -625,57 +602,76 @@ pex_unix_exec_child (struct pex_obj *obj
case 0:
/* Child process. */
- if (in != STDIN_FILE_NO)
+ if (!bad_fn && in != STDIN_FILE_NO)
{
if (dup2 (in, STDIN_FILE_NO) < 0)
- pex_child_error (obj, executable, "dup2", errno);
- if (close (in) < 0)
- pex_child_error (obj, executable, "close", errno);
+ bad_fn = "dup2";
+ else if (close (in) < 0)
+ bad_fn = "close";
}
- if (out != STDOUT_FILE_NO)
+ if (!bad_fn && out != STDOUT_FILE_NO)
{
if (dup2 (out, STDOUT_FILE_NO) < 0)
- pex_child_error (obj, executable, "dup2", errno);
- if (close (out) < 0)
- pex_child_error (obj, executable, "close", errno);
+ bad_fn = "dup2";
+ else if (close (out) < 0)
+ bad_fn = "close";
}
- if (errdes != STDERR_FILE_NO)
+ if (!bad_fn && errdes != STDERR_FILE_NO)
{
if (dup2 (errdes, STDERR_FILE_NO) < 0)
- pex_child_error (obj, executable, "dup2", errno);
- if (close (errdes) < 0)
- pex_child_error (obj, executable, "close", errno);
+ bad_fn = "dup2";
+ else if (close (errdes) < 0)
+ bad_fn = "close";
}
- if (toclose >= 0)
+ if (!bad_fn && toclose >= 0)
{
if (close (toclose) < 0)
- pex_child_error (obj, executable, "close", errno);
+ bad_fn = "close";
}
- if ((flags & PEX_STDERR_TO_STDOUT) != 0)
+ if (!bad_fn && (flags & PEX_STDERR_TO_STDOUT) != 0)
{
if (dup2 (STDOUT_FILE_NO, STDERR_FILE_NO) < 0)
- pex_child_error (obj, executable, "dup2", errno);
+ bad_fn = "dup2";
}
-
- if (env)
+ if (!bad_fn)
{
- /* NOTE: In a standard vfork implementation this clobbers the
- parent's copy of environ "too" (in reality there's only one copy).
- This is ok as we restore it below. */
-
