Alexey Serbin has posted comments on this change. Change subject: [util] added Subprocess::GetExitStatus() ......................................................................
Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/4648/7/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: Line 276 > Wonder why this routine was moved ? I reordered those to match declaration order in the header file. Line 436: if (state_ != kExited) { > Isn't this a programmatic error of calling this routine before p.Wait() ? I OK, I'll add DCHECK(). In general, I don't think adding CHECK() is a good idea when it's possible to report on a error using different means. We have an error code here and this situation is recoverable both for the caller and the callee, right? Line 535: if (wait_status != nullptr) { > nit: if (wait_status) ? Done PS7, Line 547: options > Thinking aloud here, I wonder hiding the 'options' from the actual callsite The original and current implementation does not work correctly with any other options besides WNOHANG, so this change makes the original leaky interface consistent. Line 556: CHECK(WIFEXITED(status) || WIFSIGNALED(status)); > Isn't this error-prone ? there are couple of other states the child process I tried that -- my original update here was to use do {} while(!(CHECK(WIFEXITED(status) || WIFSIGNALED(status)))) cycle. However, it never gets stopped when using tracing -- I tried both gdb and strace. If you can demonstrate how I can get WSTOPPED here, it would be nice since I'm also curious about that inconsistency in manual page. Line 561: if (wait_status != nullptr) { > nit: same as above. Done -- 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: 7 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: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
