Combined fixes for uv_spawn stdio mangling #309
Conversation
|
/cc @sam-github |
|
@bnoordhuis thanks for the review, pushed a few fixups, can you PTAL? |
|
LGTM with a question. |
|
@bnoordhuis added a couple more fixups, if you have no further comments I'll rebase and land later. Thanks Ben! |
|
LGTM. I would perhaps not assign the result of dup2 to fd on the off chance that dup2 has a bug but that's bordering on paranoid. |
If a descriptor was being copied to an fd that *was not its current value*, it should get closed after being copied. But the existing code would close the fd, even when it no longer referred to the original descriptor. Don't do this, only close fds that are not in the desired range for the child process. See nodejs/node#862 PR-URL: libuv#309 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Alternative implementation (and test) to libuv#226 Fixes: joyent/libuv#1084 PR-URL: libuv#309 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: libuv#309 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
saghul
force-pushed
the
process_fd_fixes
branch
from
97b992e
to
09cdc92
Compare
|
The CI was as unhappy as before (https://jenkins-iojs.nodesource.com/view/libuv/job/libuv+any-pr+multi/78/) so I landed it. I made a small change while landing: https://github.com/libuv/libuv/pull/309/files#diff-993e2d1f75ebf418adfbe81df8050a95R289 it's possible that we get a legit -1 there, so fcntl would fail with EBADF. Thanks again for the review Ben! |
|
@saghul Thanks for finishing this up, I wasn't able to dig myself out of my work pile far enough to finish this. |
|
No problem Sam!
|
This PR combines #224 (Sam's patch + my test) and #226 (alternate implementation by yours truly and a test).
R=@bnoordhuis
/cc @sam-github