Dinesh Bhat 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 ? Line 436: if (state_ != kExited) { Isn't this a programmatic error of calling this routine before p.Wait() ? If thats the case we could CHECK here. we could be bit more error-proof here by checking for wait_status_ or child_pid_ etc. Line 535: if (wait_status != nullptr) { nit: if (wait_status) ? PS7, Line 547: options Thinking aloud here, I wonder hiding the 'options' from the actual callsite would hinder the flexibility provided by waitpid ? Cuz WaitMode allows it to have only 1 mode at present. Line 556: CHECK(WIFEXITED(status) || WIFSIGNALED(status)); Isn't this error-prone ? there are couple of other states the child process could be in ? WIFSTOPPED if process being ptraced. Line 561: if (wait_status != nullptr) { nit: same as above. -- 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
