Alexey Serbin has posted comments on this change. Change subject: [util] added Subprocess::GetExitStatus() ......................................................................
Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/4648/5//COMMIT_MSG Commit Message: PS5, Line 13: The motivation for this change was that most use-cases involving the : Subprocess interface need exit status of the sub-process, : but not wait status returned to the parent by waitpid() syscall. > I dont really understand what you're saying here. Isn't the existing Wait() It didn't return exit status code, it returned wait status. In most cases users of this API are interested in getting exit status: in some places the wait status was mistakenly used as status code, in other places there was code doing WIFEXITED/WIFSIGNALED and friends. http://gerrit.cloudera.org:8080/#/c/4648/5/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: PS5, Line 349: // Block all signals for a while: if a signal delivered to the forked : // child before execv(), the parent's signal handler will be executed. : // This is not the expected behavior here. Also, some signal handlers of the : // parent is not async-signal safe (e.g., glog-related handlers). : // In the child's code, the signal handlers are reset to defaults except for : // SIGPIPE, which is ignored. It's a duplicated work since execv() resets : // the handlers as well, but at least it helps to get rid of unexpected : > hrm, this seems like an elaborate workaround for what's really a TSAN bug, As I understand, this is not just a TSAN bug. What prevents a signal to be delivered to the child process before execve() does its job? But I agree this does not look good. What would you recommend? Just don't send interceptable signals in the test or add some sort of delay between p.Start() and p.Kill()? Line 412: // Reset all signal handlers, except for ignored SIGPIPE, to defaults. > if the fork() failed we wouldn't reset this, which seems bad. Good catch. -- To view, visit http://gerrit.cloudera.org:8080/4648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b16e2a2a53a01982f816b9ee41cc61fd93d4bf Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
