Re: [gentoo-portage-dev] [3/7] portage_exec cleanups

[Jason Stubbs <jstubbs@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 2534 bytes --]

On Saturday 29 October 2005 23:22, Brian Harring wrote:
> On Sat, Oct 29, 2005 at 07:49:49PM +0900, Jason Stubbs wrote:
> > This patch simplifies the moving around and closing of file descriptors
> > when setting up the pipe. The addition of fd_unused to the parameter list
> > is in preparation of a later patch.
> >
> > +++ portage_exec.py	2005-10-29 19:07:21.000000000 +0900
> > -		# this may look ugly, but basically it moves file descriptors around
> > to ensure no -		# handles that are needed are accidentally closed during
> > the final dup2 calls. -		trg_fd=[]
> > -		if type(fd_pipes)==types.DictType:
> > -			src_fd=[]
> > -			k=fd_pipes.keys()
> > -			k.sort()
> > -			for x in k:
> > -				trg_fd.append(x)
> > -				src_fd.append(fd_pipes[x])
> > -			for x in range(0,len(trg_fd)):
> > -				if trg_fd[x] == src_fd[x]:
> > -					continue
> > -				if trg_fd[x] in src_fd[x+1:]:
> > -					new=os.dup2(trg_fd[x],max(src_fd) + 1)
> > -					os.close(trg_fd[x])
> > -					try:
> > -						while True:
> > -							src_fd[s.index(trg_fd[x])]=new

"s" doesn't exist. Whatever this code was intending to do, it's not doing it.

> > -					except SystemExit, e:
> > -						raise
> > -					except:
> > -						pass
> > -			for x in range(0,len(trg_fd)):
> > -				if trg_fd[x] != src_fd[x]:
> > -					os.dup2(src_fd[x], trg_fd[x])
> > -		else:
> > -			trg_fd=[0,1,2]
>
> Above *does* need reworking, knew that when I wrote it- but the reason
> it's so damn ugly is to cover against dup2 calls stomping handles that
> are needed.  Must be a collect, then re-arrange algo, not single pass
>
> fd_pipes = [1:2, 2:1]
>
> > +		for x in fd_pipes:
> > +			if x != fd_pipes[x]:
> > +				os.dup2(fd_pipes[x], x)
> > +			if fd_pipes[x] not in fd_pipes and fd_pipes[x] not in fd_unused:
> > +				fd_unused.append(fd_pipes[x])
> > +		for x in fd_unused:
> > +			os.close(x)
>
> Replacement code will fail with the simple stdout <-> stderr switch.
> dup2 will set the actual fd 2 handle to fd 1, but *prior* to
> duplicating fd 2 to a throw away handle so it remains open, end result
> is that fd 2 is now fd 1, so the 2 -> 1 move does nothing.
>
> Code above will result in 1 == 1, 2 == 1, rather then 1 == 2, 2 == 1
>
> Which is a regression, although existing code _probably_ won't choke
> on it- the protection I added made it robust so that it handles the
> corner case without tripping up any dev writing the code, rather then
> silently tieing the fd's together.

Yep, missed that case. Respun and left the fd_unused for later.

--
Jason Stubbs

[-- Attachment #2: 03-fd_pipes-switching-take2.patch --]
[-- Type: text/x-diff, Size: 1721 bytes --]

--- portage_exec.py.orig	2005-10-30 00:04:12.000000000 +0900
+++ portage_exec.py	2005-10-30 00:03:49.000000000 +0900
@@ -76,7 +76,10 @@
 		myc = find_binary(myc)
 		if myc == None:
 			return None
-		
+
+	if fd_pipes is None:
+		fd_pipes = {0:0, 1:1, 2:2}
+
 	mypid=[]
 	if logfile:
 		pr,pw=os.pipe()
@@ -88,9 +91,6 @@
 				return (retval >> 8) # exit code
 			else:
 				return ((retval & 0xff) << 8) # signal
-		if not fd_pipes:
-			fd_pipes={}
-			fd_pipes[0] = 0
 		fd_pipes[1]=pw
 		fd_pipes[2]=pw
 		
@@ -100,34 +100,16 @@
 	myargs.extend(mycommand[1:])
 	mypid.append(os.fork())
 	if mypid[-1] == 0:
-		# this may look ugly, but basically it moves file descriptors around to ensure no
-		# handles that are needed are accidentally closed during the final dup2 calls.
-		trg_fd=[]
-		if type(fd_pipes)==types.DictType:
-			src_fd=[]
-			k=fd_pipes.keys()
-			k.sort()
-			for x in k:
-				trg_fd.append(x)
-				src_fd.append(fd_pipes[x])
-			for x in range(0,len(trg_fd)):
-				if trg_fd[x] == src_fd[x]:
-					continue
-				if trg_fd[x] in src_fd[x+1:]:
-					new=os.dup2(trg_fd[x],max(src_fd) + 1)
-					os.close(trg_fd[x])
-					try:
-						while True: 
-							src_fd[s.index(trg_fd[x])]=new
-					except SystemExit, e:
-						raise
-					except:
-						pass
-			for x in range(0,len(trg_fd)):
-				if trg_fd[x] != src_fd[x]:
-					os.dup2(src_fd[x], trg_fd[x])
-		else:
-			trg_fd=[0,1,2]
+		my_pipes = {}
+		close_pipes = {}
+		for x in fd_pipes:
+			my_pipes[x] = os.dup(fd_pipes[x])
+			close_pipes[fd_pipes[x]] = True
+		for x in close_pipes:
+			os.close(x)
+		for x in my_pipes:
+			os.dup2(my_pipes[x], x)
+			os.close(my_pipes[x])
 		for x in range(0,max_fd_limit):
 			if x not in trg_fd:
 				try: