From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lists.gentoo.org ([140.105.134.102] helo=robin.gentoo.org) by nuthatch.gentoo.org with esmtp (Exim 4.50) id 1EVrby-00056t-VW for garchives@archives.gentoo.org; Sat, 29 Oct 2005 14:23:11 +0000 Received: from robin.gentoo.org (localhost [127.0.0.1]) by robin.gentoo.org (8.13.5/8.13.5) with SMTP id j9TEMZMC007554; Sat, 29 Oct 2005 14:22:35 GMT Received: from smtp.gentoo.org (smtp.gentoo.org [134.68.220.30]) by robin.gentoo.org (8.13.5/8.13.5) with ESMTP id j9TEMYqV011698 for ; Sat, 29 Oct 2005 14:22:34 GMT Received: from cpe-65-26-255-237.wi.res.rr.com ([65.26.255.237] helo=nightcrawler) by smtp.gentoo.org with esmtpa (Exim 4.43) id 1EVrbO-0004XX-B6 for gentoo-portage-dev@lists.gentoo.org; Sat, 29 Oct 2005 14:22:34 +0000 Date: Sat, 29 Oct 2005 09:22:07 -0500 From: Brian Harring To: gentoo-portage-dev@lists.gentoo.org Subject: Re: [gentoo-portage-dev] [3/7] portage_exec cleanups Message-ID: <20051029142207.GL24883@nightcrawler> References: <200510231545.16596.jstubbs@gentoo.org> <200510291934.13207.jstubbs@gentoo.org> <200510291949.50029.jstubbs@gentoo.org> Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-portage-dev@gentoo.org Reply-to: gentoo-portage-dev@lists.gentoo.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="AA9g+nFNFPYNJKiL" Content-Disposition: inline In-Reply-To: <200510291949.50029.jstubbs@gentoo.org> User-Agent: Mutt/1.5.8i X-Archives-Salt: 8b570252-f0d5-4abb-9481-e96e88b42404 X-Archives-Hash: 5a54a5ce75365e333fb68cfe617551d8 --AA9g+nFNFPYNJKiL Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Oct 29, 2005 at 07:49:49PM +0900, Jason Stubbs wrote: > This patch simplifies the moving around and closing of file descriptors w= hen=20 > setting up the pipe. The addition of fd_unused to the parameter list is i= n=20 > 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 t= o ensure no > - # handles that are needed are accidentally closed during the final dup= 2 calls. > - trg_fd=3D[] > - if type(fd_pipes)=3D=3Dtypes.DictType: > - src_fd=3D[] > - k=3Dfd_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] =3D=3D src_fd[x]: > - continue > - if trg_fd[x] in src_fd[x+1:]: > - new=3Dos.dup2(trg_fd[x],max(src_fd) + 1) > - os.close(trg_fd[x]) > - try: > - while True:=20 > - src_fd[s.index(trg_fd[x])]=3Dnew > - except SystemExit, e: > - raise > - except: > - pass > - for x in range(0,len(trg_fd)): > - if trg_fd[x] !=3D src_fd[x]: > - os.dup2(src_fd[x], trg_fd[x]) > - else: > - trg_fd=3D[0,1,2] Above *does* need reworking, knew that when I wrote it- but the reason=20 it's so damn ugly is to cover against dup2 calls stomping handles that=20 are needed. Must be a collect, then re-arrange algo, not single pass fd_pipes =3D [1:2, 2:1] > + for x in fd_pipes: > + if x !=3D 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=20 duplicating fd 2 to a throw away handle so it remains open, end result=20 is that fd 2 is now fd 1, so the 2 -> 1 move does nothing. Code above will result in 1 =3D=3D 1, 2 =3D=3D 1, rather then 1 =3D=3D 2, 2= =3D=3D 1 Which is a regression, although existing code _probably_ won't choke=20 on it- the protection I added made it robust so that it handles the=20 corner case without tripping up any dev writing the code, rather then=20 silently tieing the fd's together. fd_unused doesn't cut it either also, or at the very least introduces=20 an ass biter hidden behaviour into spawn- consider fd_pipes=3D{} Previous code *would* close all fd's, and execute the process as such. =20 With fd_unused addition, it won't anymore- meaning at the very least a=20 chunk of the regen code will need updating (closes stdin last I=20 looked). Very least, all callers of spawn with fd_pipes !=3D {0:0, 1:1,=20 2:2} need verification, since the handles they were having explicitly=20 closed prior, are now left open. With the fd_unused route, there is no way clean way to have all fd's=20 closed. You don't know what fd's are open when you spawn (nor is=20 there any way to know, beyond testing each potential fd, or overriding=20 the file builtin); going the fd_unused route means fd's the process=20 shouldn't have access to, will have access to. Mind you the fd_unused comments are about this patch and the one +2=20 down the set. So... regressions, both in fd_pipes handling, and in open handles=20 leaking to the spawned processes The original code is ugly, and knowing a bit more this time around I'd=20 write it a bit differently, that said the protections/behaviour=20 originally in it should be left- it's more robust, and is expected=20 now. ~harring --AA9g+nFNFPYNJKiL Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFDY4WPvdBxRoA3VU0RAmuxAJwMgIHhJ25il3KHXWA7s5YBMQAUVwCgxLyC ZalMpFqDYmXzCavYfuP3axA= =DqY/ -----END PGP SIGNATURE----- --AA9g+nFNFPYNJKiL-- -- gentoo-portage-dev@gentoo.org mailing list