Dinesh Bhat has posted comments on this change. Change subject: [util] added Subprocess::GetExitStatus() ......................................................................
Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/4648/7/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: Line 436: } > OK, I'll add DCHECK(). In general, I don't think adding CHECK() is a good We can CHECK If this really is a programmer error of calling GetExitStatus without calling Wait first. i.e, state != kExited can happen only if the parent is asking for an exit status without calling Wait(), No ? In the case of WNOHANG waiting indefinitely, we can differentiate that by way of child_pid_ added in the mix too. i.e CHECK(state_ == exited || child_pid_ != -1) Line 556: // as many strings as there were 'fds' in the vector and in that order. > Correction: my original code was something like do {} while(!(WIFEXITED(sta I see, thanks for the update here. I am fine if you still want to have this CHECK, but it didn't seem to be buying much here to narrow down the expected outcome of waitpid. Also, routine is anyways returning status back to caller, so caller is better situated to make such decisions. http://gerrit.cloudera.org:8080/#/c/4648/8/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: PS8, Line 420: from s/from// ? PS8, Line 422: which the child invokes execvp() when the child is invoked via execvp ? Line 451: } Still digesting this piece, will get back on this.. -- 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: 8 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: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
