Dinesh Bhat has posted comments on this change. Change subject: KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone ......................................................................
Patch Set 1: (9 comments) TFTR Adar/Alexey, updated the patch, please see responses inline. http://gerrit.cloudera.org:8080/#/c/4594/1/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: Line 22: #include <cstdlib> > What's this one for? needed for std::rand below. Line 29: #include "kudu/gutil/strings/substitute.h" > Not used? yeah, I was initially using strings::Substitute and re-added now while taking care of one of rev comments below. PS1, Line 133: <random> > Why randomize the non-existent command? You can just hardcode some nonsensi That works too, random number prefixed with a nonsensical string could make it more stronger case for stderr I would think. Lemme know. Also I stuck to simple rand() instead of more accurate C++11 versions of rand(). Updated accordingly. PS1, Line 136: TestKUDU1674 > You've already got the bug in the comment just above, so use the test name Done Line 141: ASSERT_FALSE(stderr.empty()); > Nit: since you check for the presence on non-empty substring in stderr, thi Done Line 148: ASSERT_FALSE(stdout.empty()); > ditto. Done http://gerrit.cloudera.org:8080/#/c/4594/1/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: PS1, Line 502: capacity > Why capacity() instead of size()? Capacity is controlled by the implementa Sounds good, fixed the code as well as comment above. Line 504: *stdout_out = std::move(outv.front()); > Do you think it's worth to check that !outv.empty() before calling outv.fro Isn't that redundant if we are already checking for outv.size() as per your comment above ? More useful check could have been to recognize that outv.front had captured stdout(and not stderr), but we don't have visibility into that in this line. Line 507: *stderr_out = std::move(outv.back()); > Ditto here: do you think it's worth to check that !outv.empty() before call same as above. -- To view, visit http://gerrit.cloudera.org:8080/4594 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I67bd462098526a9ba032669b621b8139b56ee5b8 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
