[master] Restart waitpid on EINTR
Vratislav Podzimek
vpodzime at redhat.com
Tue Aug 12 07:02:30 UTC 2014
On Mon, 2014-08-11 at 13:31 -0400, David Shea wrote:
> Depending on the order or child exits and signals and waitpid starting,
> calling waitpid alone sometimes works and sometimes crashes anaconda.
> Check if an exception indicates that waitpid should restart.
> ---
> anaconda | 8 ++++++--
> pyanaconda/iutil.py | 16 ++++++++++++++++
> pyanaconda/users.py | 2 +-
> tests/pyanaconda_tests/iutil_test.py | 36 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/anaconda b/anaconda
> index b4dddc9..8e58c18 100755
> --- a/anaconda
> +++ b/anaconda
> @@ -56,6 +56,8 @@ class ExitError(RuntimeError):
> pass
>
> def sigchld_handler(num, frame):
> + from pyanaconda.iutil import really_waitpid
> +
> # Check whether anything in the list of processes being watched has
> # exited. We don't want to call waitpid(-1), since that would break
> # anything else using wait/waitpid (like the subprocess module).
> @@ -64,7 +66,7 @@ def sigchld_handler(num, frame):
>
> for child_pid in forever_pids:
> try:
> - pid_result, status = os.waitpid(child_pid, os.WNOHANG)
> + pid_result, status = really_waitpid(child_pid, os.WNOHANG)
> except OSError as e:
> if e.errno == errno.ECHILD:
> continue
> @@ -281,6 +283,8 @@ def startMetacityWM():
> return childpid
>
> def startAuditDaemon():
> + from pyanaconda.iutil import really_waitpid
> +
> childpid = os.fork()
> if not childpid:
> cmd = '/sbin/auditd'
> @@ -290,7 +294,7 @@ def startAuditDaemon():
> log.error("Error running the audit daemon: %s", e)
> os._exit(0)
> # auditd will turn into a daemon so catch the immediate child pid now:
> - os.waitpid(childpid, 0)
> + really_waitpid(childpid, 0)
>
> # function to handle X startup special issues for anaconda
> def doStartupX11Actions():
> diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
> index e426511..af87860 100644
> --- a/pyanaconda/iutil.py
> +++ b/pyanaconda/iutil.py
> @@ -913,3 +913,19 @@ def persistent_root_image():
> return False
>
> return True
> +
> +def really_waitpid(pid, options):
> + """Run waitpid, and restart if waitpid returns with EINTR.
> +
> + :param int pid: the PID to wait for
> + :param options: Options to pass to os.waitpid
> + :return: A tuple containing the process ID and exit status
> + :rtype: tuple
> + """
> + while True:
> + try:
> + return os.waitpid(pid, options)
> + except OSError as e:
> + if e.errno == errno.EINTR:
> + continue
> + raise
> diff --git a/pyanaconda/users.py b/pyanaconda/users.py
> index 82cd2d3..aebe65d 100644
> --- a/pyanaconda/users.py
> +++ b/pyanaconda/users.py
> @@ -218,7 +218,7 @@ class Users:
> def _finishChroot(self, childpid):
> assert childpid > 0
> try:
> - status = os.waitpid(childpid, 0)[1]
> + status = iutil.really_waitpid(childpid, 0)[1]
> except OSError as e:
> log.critical("exception from waitpid: %s %s", e.errno, e.strerror)
> return False
> diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py
> index ac4fb76..76824a9 100644
> --- a/tests/pyanaconda_tests/iutil_test.py
> +++ b/tests/pyanaconda_tests/iutil_test.py
> @@ -25,6 +25,7 @@ import types
> import os
> import shutil
> from test_constants import ANACONDA_TEST_DIR
> +import errno
>
> class UpcaseFirstLetterTests(unittest.TestCase):
>
> @@ -434,3 +435,38 @@ class RunProgramTests(unittest.TestCase):
> self.assertFalse(iutil.have_word_match("", None))
> self.assertFalse(iutil.have_word_match(None, ""))
> self.assertFalse(iutil.have_word_match(None, None))
> +
> + def waitpid_test(self):
> + """Test restarting waitpid when a signal is received."""
> +
> + # Instead of trying to interrupt an actual waitpid call, which is
> + # hard to do without some inherent race, just replace os.waitpid with
> + # a version that raises EINTR
> +
> + _oldwaitpid = os.waitpid
> + def _intrwaitpid(pid, options):
> + os.waitpid = _oldwaitpid
> + raise OSError(errno.EINTR, "Interrupted")
> +
> + # Test that waitpid doesn't work
> + try:
> + os.waitpid = _intrwaitpid
> + childpid = os.fork()
> + if childpid == 0:
> + os._exit(0)
> + with self.assertRaises(OSError):
> + os.waitpid(childpid, 0)
> + finally:
> + os.waitpid = _oldwaitpid
> +
> + # Test that really_waitpid does work
> + try:
> + os.waitpid = _intrwaitpid
> + childpid = os.fork()
> + if childpid == 0:
> + os._exit(0)
> + (_pid, result) = iutil.really_waitpid(childpid, 0)
> + self.assertTrue(os.WIFEXITED(result))
> + self.assertEqual(os.WEXITSTATUS(result), 0)
> + finally:
> + os.waitpid = _oldwaitpid
This looks good to me.
--
Vratislav Podzimek
Anaconda Rider | RHCE | Red Hat, Inc. | Brno - Czech Republic
More information about the anaconda-patches
mailing list