From dbe26095102cbdc6d5bef3509f05bc7b42c418cc Mon Sep 17 00:00:00 2001 From: Zac Medico Date: Tue, 15 Jan 2013 12:09:21 -0800 Subject: [PATCH] SpawnProcess: improve dummy pipe allocation logic By using allocated file descriptors for keys in fd_pipes, we naturally avoid interference with callers such as FileDigester and MergeProcess. See the _setup_pipes docstring for more benefits of this allocation approach. --- bin/ebuild.sh | 7 ++++-- pym/_emerge/EbuildProcess.py | 12 +++++++--- pym/_emerge/EbuildSpawnProcess.py | 10 ++++++-- pym/_emerge/MiscFunctionsProcess.py | 7 +++++- pym/_emerge/SpawnProcess.py | 23 ++++++++----------- pym/_emerge/SubProcess.py | 9 ++------ .../ebuild/_config/special_env_vars.py | 4 ++-- pym/portage/process.py | 15 ++++++++++++ 8 files changed, 57 insertions(+), 30 deletions(-) diff --git a/bin/ebuild.sh b/bin/ebuild.sh index 80bdd8001..a4be7c067 100755 --- a/bin/ebuild.sh +++ b/bin/ebuild.sh @@ -1,5 +1,5 @@ #!/bin/bash -# Copyright 1999-2012 Gentoo Foundation +# Copyright 1999-2013 Gentoo Foundation # Distributed under the terms of the GNU General Public License v2 PORTAGE_BIN_PATH="${PORTAGE_BIN_PATH:-/usr/lib/portage/bin}" @@ -713,7 +713,10 @@ else ( # Don't allow subprocesses to inherit the pipe which # emerge uses to monitor ebuild.sh. - exec 9>&- + if [[ -n ${PORTAGE_PIPE_FD} ]] ; then + eval "exec ${PORTAGE_PIPE_FD}>&-" + unset PORTAGE_PIPE_FD + fi __ebuild_main ${EBUILD_SH_ARGS} exit 0 ) diff --git a/pym/_emerge/EbuildProcess.py b/pym/_emerge/EbuildProcess.py index ce97aff0f..333ad7bd0 100644 --- a/pym/_emerge/EbuildProcess.py +++ b/pym/_emerge/EbuildProcess.py @@ -1,4 +1,4 @@ -# Copyright 1999-2010 Gentoo Foundation +# Copyright 1999-2013 Gentoo Foundation # Distributed under the terms of the GNU General Public License v2 from _emerge.AbstractEbuildProcess import AbstractEbuildProcess @@ -17,5 +17,11 @@ class EbuildProcess(AbstractEbuildProcess): if actionmap is None: actionmap = _spawn_actionmap(self.settings) - return _doebuild_spawn(self.phase, self.settings, - actionmap=actionmap, **kwargs) + if self._dummy_pipe_fd is not None: + self.settings["PORTAGE_PIPE_FD"] = str(self._dummy_pipe_fd) + + try: + return _doebuild_spawn(self.phase, self.settings, + actionmap=actionmap, **kwargs) + finally: + self.settings.pop("PORTAGE_PIPE_FD", None) diff --git a/pym/_emerge/EbuildSpawnProcess.py b/pym/_emerge/EbuildSpawnProcess.py index e1f682a66..26d26fc77 100644 --- a/pym/_emerge/EbuildSpawnProcess.py +++ b/pym/_emerge/EbuildSpawnProcess.py @@ -1,4 +1,4 @@ -# Copyright 2010 Gentoo Foundation +# Copyright 2010-2013 Gentoo Foundation # Distributed under the terms of the GNU General Public License v2 from _emerge.AbstractEbuildProcess import AbstractEbuildProcess @@ -13,4 +13,10 @@ class EbuildSpawnProcess(AbstractEbuildProcess): __slots__ = ('fakeroot_state', 'spawn_func') def _spawn(self, args, **kwargs): - return self.spawn_func(args, env=self.settings.environ(), **kwargs) + + env = self.settings.environ() + + if self._dummy_pipe_fd is not None: + env["PORTAGE_PIPE_FD"] = str(self._dummy_pipe_fd) + + return self.spawn_func(args, env=env, **kwargs) diff --git a/pym/_emerge/MiscFunctionsProcess.py b/pym/_emerge/MiscFunctionsProcess.py index afa44fb2a..bada79d86 100644 --- a/pym/_emerge/MiscFunctionsProcess.py +++ b/pym/_emerge/MiscFunctionsProcess.py @@ -1,4 +1,4 @@ -# Copyright 1999-2011 Gentoo Foundation +# Copyright 1999-2013 Gentoo Foundation # Distributed under the terms of the GNU General Public License v2 from _emerge.AbstractEbuildProcess import AbstractEbuildProcess @@ -29,6 +29,10 @@ class MiscFunctionsProcess(AbstractEbuildProcess): AbstractEbuildProcess._start(self) def _spawn(self, args, **kwargs): + + if self._dummy_pipe_fd is not None: + self.settings["PORTAGE_PIPE_FD"] = str(self._dummy_pipe_fd) + # Temporarily unset EBUILD_PHASE so that bashrc code doesn't # think this is a real phase. phase_backup = self.settings.pop("EBUILD_PHASE", None) @@ -37,3 +41,4 @@ class MiscFunctionsProcess(AbstractEbuildProcess): finally: if phase_backup is not None: self.settings["EBUILD_PHASE"] = phase_backup + self.settings.pop("PORTAGE_PIPE_FD", None) diff --git a/pym/_emerge/SpawnProcess.py b/pym/_emerge/SpawnProcess.py index 9c754add5..ebba7d3d4 100644 --- a/pym/_emerge/SpawnProcess.py +++ b/pym/_emerge/SpawnProcess.py @@ -70,23 +70,20 @@ class SpawnProcess(SubProcess): fd_pipes_orig = fd_pipes.copy() - if log_file_path is not None: + if log_file_path is not None or self.background: fd_pipes[1] = slave_fd fd_pipes[2] = slave_fd else: - # Create a dummy pipe so the scheduler can monitor - # the process from inside a poll() loop. Ensure that - # it doesn't interfere with a random fd that's already - # in fd_pipes though (as least FileDigester can pass - # in a random fd returned from os.pipe()). - unique_dummy_fd = self._dummy_pipe_fd - while unique_dummy_fd in fd_pipes: - unique_dummy_fd += 1 - fd_pipes[unique_dummy_fd] = slave_fd - if self.background: - fd_pipes[1] = slave_fd - fd_pipes[2] = slave_fd + # Create a dummy pipe that PipeLogger uses to efficiently + # monitors for process exit by listening for the EOF event. + # Re-use of the allocated fd number for the key in fd_pipes + # guarantees that the keys will not collide for similarly + # allocated pipes which are used by callers such as + # FileDigester and MergeProcess. See the _setup_pipes + # docstring for more benefits of this allocation approach. + self._dummy_pipe_fd = slave_fd + fd_pipes[slave_fd] = slave_fd kwargs = {} for k in self._spawn_kwarg_names: diff --git a/pym/_emerge/SubProcess.py b/pym/_emerge/SubProcess.py index 92cbc27ab..4ccf9164f 100644 --- a/pym/_emerge/SubProcess.py +++ b/pym/_emerge/SubProcess.py @@ -1,4 +1,4 @@ -# Copyright 1999-2012 Gentoo Foundation +# Copyright 1999-2013 Gentoo Foundation # Distributed under the terms of the GNU General Public License v2 from portage import os @@ -9,12 +9,7 @@ import errno class SubProcess(AbstractPollTask): __slots__ = ("pid",) + \ - ("_files", "_reg_id") - - # A file descriptor is required for the scheduler to monitor changes from - # inside a poll() loop. When logging is not enabled, create a pipe just to - # serve this purpose alone. - _dummy_pipe_fd = 9 + ("_dummy_pipe_fd", "_files", "_reg_id") # This is how much time we allow for waitpid to succeed after # we've sent a kill signal to our subprocess. diff --git a/pym/portage/package/ebuild/_config/special_env_vars.py b/pym/portage/package/ebuild/_config/special_env_vars.py index 578e417f2..93bb98a42 100644 --- a/pym/portage/package/ebuild/_config/special_env_vars.py +++ b/pym/portage/package/ebuild/_config/special_env_vars.py @@ -22,7 +22,7 @@ env_blacklist = frozenset(( "PDEPEND", "PF", "PKGUSE", "PORTAGE_BACKGROUND", "PORTAGE_BACKGROUND_UNMERGE", "PORTAGE_BUILDDIR_LOCKED", "PORTAGE_BUILT_USE", "PORTAGE_CONFIGROOT", "PORTAGE_IUSE", - "PORTAGE_NONFATAL", "PORTAGE_REPO_NAME", + "PORTAGE_NONFATAL", "PORTAGE_PIPE_FD", "PORTAGE_REPO_NAME", "PORTAGE_USE", "PROPERTIES", "PROVIDE", "RDEPEND", "REPOSITORY", "RESTRICT", "ROOT", "SLOT", "SRC_URI" )) @@ -63,7 +63,7 @@ environ_whitelist += [ "PORTAGE_GID", "PORTAGE_GRPNAME", "PORTAGE_INST_GID", "PORTAGE_INST_UID", "PORTAGE_IPC_DAEMON", "PORTAGE_IUSE", - "PORTAGE_LOG_FILE", "PORTAGE_OVERRIDE_EPREFIX", + "PORTAGE_LOG_FILE", "PORTAGE_OVERRIDE_EPREFIX", "PORTAGE_PIPE_FD", "PORTAGE_PYM_PATH", "PORTAGE_PYTHON", "PORTAGE_QUIET", "PORTAGE_REPO_NAME", "PORTAGE_RESTRICT", "PORTAGE_SIGPIPE_STATUS", diff --git a/pym/portage/process.py b/pym/portage/process.py index 4cf1cec60..23a909466 100644 --- a/pym/portage/process.py +++ b/pym/portage/process.py @@ -429,6 +429,21 @@ def _setup_pipes(fd_pipes, close_fds=True): and also with CPython under some circumstances (as triggered by xmpppy in bug #374335). In order to close a safe subset of file descriptors, see portage.locks._close_fds(). + + NOTE: When not followed by exec, even when close_fds is False, + it's still possible for dup2() calls to cause interference in a + way that's similar to the way that close_fds interferes (since + dup2() has to close the target fd if it happens to be open). + It's possible to avoid such interference by using allocated + file descriptors as the keys in fd_pipes. For example: + + pr, pw = os.pipe() + fd_pipes[pw] = pw + + By using the allocated pw file descriptor as the key in fd_pipes, + it's not necessary for dup2() to close a file descriptor (it + actually does nothing in this case), which avoids possible + interference. """ my_fds = {} # To protect from cases where direct assignment could -- 2.26.2