[gentoo-commits] proj/portage:master commit in: pym/portage/package/ebuild/_config/, bin/, pym/portage/, pym/_emerge/

[gentoo-commits] proj/portage:master commit in: pym/portage/package/ebuild/_config/, bin/, pym/portage/, pym/_emerge/

[Zac Medico]

commit:     dbe26095102cbdc6d5bef3509f05bc7b42c418cc
Author:     Zac Medico <zmedico <AT> gentoo <DOT> org>
AuthorDate: Tue Jan 15 20:09:21 2013 +0000
Commit:     Zac Medico <zmedico <AT> gentoo <DOT> org>
CommitDate: Tue Jan 15 20:09:21 2013 +0000
URL:        http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=dbe26095

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 +------
 .../package/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 80bdd80..a4be7c0 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 ce97aff..333ad7b 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 e1f682a..26d26fc 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 afa44fb..bada79d 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 9c754ad..ebba7d3 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 92cbc27..4ccf916 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 578e417..93bb98a 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 4cf1cec..23a9094 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