1 |
On Saturday 29 October 2005 19:56, Jason Stubbs wrote: |
2 |
> All file descriptors opened by spawn() are now closed in the correct |
3 |
> places. The code that closes all unnecessary descriptors via brute force is |
4 |
> no longer required. |
5 |
> |
6 |
> I found that a file descriptor was being left open and passed around. |
7 |
> However, it was definately not created in spawn(). It seems to me though |
8 |
> that code outside of spawn should take care of closing (or telling spawn to |
9 |
> close) any unnecessary FDs. |
10 |
|
11 |
|
12 |
On Saturday 29 October 2005 23:22, Brian Harring wrote: |
13 |
> fd_unused doesn't cut it either also, or at the very least introduces |
14 |
> an ass biter hidden behaviour into spawn- consider |
15 |
> |
16 |
> fd_pipes={} |
17 |
> |
18 |
> Previous code *would* close all fd's, and execute the process as such. |
19 |
> 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 |
21 |
> looked). Very least, all callers of spawn with fd_pipes != {0:0, 1:1, |
22 |
> 2:2} need verification, since the handles they were having explicitly |
23 |
> closed prior, are now left open. |
24 |
|
25 |
Not explicitly closed. Implicitly closed. |
26 |
|
27 |
> With the fd_unused route, there is no way clean way to have all fd's |
28 |
> closed. You don't know what fd's are open when you spawn (nor is |
29 |
> there any way to know, beyond testing each potential fd, or overriding |
30 |
> the file builtin); going the fd_unused route means fd's the process |
31 |
> shouldn't have access to, will have access to. |
32 |
|
33 |
This is a difference of viewpoint, I think. Having unknown fds left open by |
34 |
the calling code seems unclean to me. Iterating through 100s or 1000s of fds |
35 |
and attempting to close them just in case the caller was sloppy seems equally |
36 |
unclean. |
37 |
|
38 |
By the way, this method does not allow having a pipe open to a child process |
39 |
other than stdin, stdout or stderr. |
40 |
|
41 |
> Mind you the fd_unused comments are about this patch and the one +2 |
42 |
> down the set. |
43 |
> |
44 |
> So... regressions, both in fd_pipes handling, and in open handles |
45 |
> leaking to the spawned processes |
46 |
|
47 |
fd_pipes handling is now addressed. The open handles leaking is an issue, but |
48 |
where the issue stems from is yet to be resolved. |
49 |
|
50 |
> The original code is ugly, and knowing a bit more this time around I'd |
51 |
> write it a bit differently, that said the protections/behaviour |
52 |
> originally in it should be left- it's more robust, and is expected |
53 |
> now. |
54 |
|
55 |
I disagree here. Presently, other than the execve call, every try/except block |
56 |
in portage_exec.py has bugs that are hidden by the "protections". |
57 |
|
58 |
-- |
59 |
Jason Stubbs |
60 |
-- |
61 |
gentoo-portage-dev@g.o mailing list |