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

[Brian Harring]

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
> -					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.


fd_unused doesn't cut it either also, or at the very least introduces 
an ass biter hidden behaviour into spawn- consider

fd_pipes={}

Previous code *would* close all fd's, and execute the process as such.  
With fd_unused addition, it won't anymore- meaning at the very least a 
chunk of the regen code will need updating (closes stdin last I 
looked).  Very least, all callers of spawn with fd_pipes != {0:0, 1:1, 
2:2} need verification, since the handles they were having explicitly 
closed prior, are now left open.

With the fd_unused route, there is no way clean way to have all fd's 
closed.  You don't know what fd's are open when you spawn (nor is 
there any way to know, beyond testing each potential fd, or overriding 
the file builtin); going the fd_unused route means fd's the process 
shouldn't have access to, will have access to.

Mind you the fd_unused comments are about this patch and the one +2 
down the set.

So... regressions, both in fd_pipes handling, and in open handles 
leaking to the spawned processes

The original code is ugly, and knowing a bit more this time around I'd 
write it a bit differently, that said the protections/behaviour 
originally in it should be left- it's more robust, and is expected 
now.
~harring