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

[Jason Stubbs]

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