Re: [gentoo-portage-dev] [0/7] portage_exec cleanups (WAS: [Bug 104705] emerge doesn't print complete error message)

[Jason Stubbs <jstubbs@gentoo.org>]

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

On Sunday 30 October 2005 02:32, Jason Stubbs wrote:
> On Saturday 29 October 2005 19:34, Jason Stubbs wrote:
> > On Sunday 23 October 2005 15:45, Jason Stubbs wrote:
> > > Commented on the bug due to reasoning behind this patch. Essentially,
> > > SIGTERM is sent to "tee", a WNOHANG waitpid() is performed followed by
> > > SIGKILL if it hasn't exited. So if tee doesn't exit immediately upon
> > > getting the SIGTERM, its buffers won't get a chance to get to disk due
> > > to it being killed. This patch simply adds a 1 second window between
> > > the SIGTERM and the SIGKILL.
> >
> > Per discussion on the bug, the problem is actually that tee shouldn't be
> > sent a SIGTERM or SIGKILL at all. The decided solution was to wait on tee
> > rather ebuild. I went a bit further than that though and cleaned up most
> > of spawn().
>
> Here's the full patch after incorporating feedback. It should be fairly
> easy to follow given the other explanations. The only change beyond what's
> already been gone over is that spawn() is now removing pids from
> spawned_pids as it cleans them up.

02:30.. Brain is starting to stop working. Here's a patch of _just_ 
portage_exec.py. With regard to splitting up patches, I agree it's much 
easier to parse a complete patch when familiar with the code. When one isn't 
familiar with the code however... Splitting it up a bit makes explaining the 
whys easier too - as well as keeping discussion threads seperate.

--
Jason Stubbs

[-- Attachment #2: exec-cleanup.patch --]
[-- Type: text/x-diff, Size: 4994 bytes --]

Index: pym/portage_exec.py
===================================================================
--- pym/portage_exec.py	(revision 2199)
+++ pym/portage_exec.py	(working copy)
@@ -11,11 +11,8 @@
 
 try:
 	import resource
-	max_fd_limit=resource.getrlimit(RLIMIT_NOFILE)
-except SystemExit, e:
-	raise
-except:
-	# hokay, no resource module.
+	max_fd_limit=resource.getrlimit(resource.RLIMIT_NOFILE)[1]
+except ImportError:
 	max_fd_limit=256
 
 spawned_pids = []
@@ -23,12 +20,9 @@
 	global spawned_pids
 	while spawned_pids:
 		pid = spawned_pids.pop()
-		try:
-			os.kill(pid,SIGKILL)
-		except SystemExit, e:
-			raise
-		except:
-			pass
+		if os.waitpid(pid,os.WNOHANG) == (0,0):
+			os.kill(pid,signal.SIGKILL)
+			os.waitpid(pid,0)
 atexit.register(cleanup)
 
 from portage_const import BASH_BINARY,SANDBOX_BINARY,SANDBOX_PIDS_FILE
@@ -67,6 +61,7 @@
 
 # base spawn function
 def spawn(mycommand,env={},opt_name=None,fd_pipes=None,returnpid=False,uid=None,gid=None,groups=None,umask=None,logfile=None,path_lookup=True):
+	global spawned_pids
 	if type(mycommand)==types.StringType:
 		mycommand=mycommand.split()
 	myc = mycommand[0]
@@ -76,21 +71,15 @@
 		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()
-		mypid.extend(spawn(('tee','-i','-a',logfile),returnpid=True,fd_pipes={0:pr,1:1,2:2}))
-		retval=os.waitpid(mypid[-1],os.WNOHANG)[1]
-		if retval != 0:
-			# he's dead jim.
-			if (retval & 0xff)==0:
-				return (retval >> 8) # exit code
-			else:
-				return ((retval & 0xff) << 8) # signal
-		if not fd_pipes:
-			fd_pipes={}
-			fd_pipes[0] = 0
+		mypid.extend(spawn(('tee','-i','-a',logfile),returnpid=True,fd_pipes={0:pr,1:fd_pipes[1],2:fd_pipes[2]}))
+		os.close(pr)
 		fd_pipes[1]=pw
 		fd_pipes[2]=pw
 		
@@ -100,41 +89,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]
-		for x in range(0,max_fd_limit):
-			if x not in trg_fd:
-				try: 
+		my_pipes = {}
+		for x in fd_pipes:
+			my_pipes[x] = os.dup(fd_pipes[x])
+		for x in my_pipes:
+			os.dup2(my_pipes[x], x)
+		for x in range(max_fd_limit):
+			if x not in my_pipes:
+				try:
 					os.close(x)
-				except SystemExit, e:
-					raise
-				except:
+				except OSError:
 					pass
 		# note this order must be preserved- can't change gid/groups if you change uid first.
 		if gid:
@@ -145,50 +109,29 @@
 			os.setuid(uid)
 		if umask:
 			os.umask(umask)
-		try:
-			# XXX: We would do this to stop ebuild.sh from getting any
-			# XXX: output, and consequently, we'd get to handle the sigINT.
-			#os.close(sys.stdin.fileno())
-			pass
-		except SystemExit, e:
-			raise
-		except:
-			pass
 
 		try:
-			#print "execing", myc, myargs
 			os.execve(myc,myargs,env)
-		except SystemExit, e:
-			raise
 		except Exception, e:
-			raise str(e)+":\n   "+myc+" "+string.join(myargs)
-		# If the execve fails, we need to report it, and exit
-		# *carefully* --- report error here
-		os._exit(1)
-		sys.exit(1)
-		return # should never get reached
+			print str(e)+":\n   "+myc+" "+string.join(myargs)
+			os._exit(1)
 
 	if logfile:
-		os.close(pr)
 		os.close(pw)
 	
 	if returnpid:
-		global spawned_pids
 		spawned_pids.append(mypid[-1])
 		return mypid
 	while len(mypid):
-		retval=os.waitpid(mypid[-1],0)[1]
+		pid = mypid.pop(0)
+		retval=os.waitpid(pid,0)[1]
+		if pid in spawned_pids:
+			spawned_pids.remove(pid)
 		if retval != 0:
-			for x in mypid[0:-1]:
-				try:
-					os.kill(x,signal.SIGTERM)
-					if os.waitpid(x,os.WNOHANG)[1] == 0:
-						# feisty bugger, still alive.
-						os.kill(x,signal.SIGKILL)
+			for x in mypid:
+				if os.waitpid(x,os.WNOHANG) == (0,0):
+					os.kill(x,signal.SIGKILL)
 					os.waitpid(x,0)
-				except OSError, oe:
-					if oe.errno not in (10,3):
-						raise oe
 			
 			# at this point we've killed all other kid pids generated via this call.
 			# return now.
@@ -197,8 +140,6 @@
 				return (retval >> 8) # return exit code
 			else:
 				return ((retval & 0xff) << 8) # interrupted by signal
-		else:
-			mypid.pop(-1)
 	return 0
 
 def find_binary(myc):
@@ -211,5 +152,3 @@
 			return "%s/%s" % (x,myc)
 
 	return None
-
-