Alexey Serbin has posted comments on this change. Change subject: KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone ......................................................................
Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/4594/3/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: Line 27: #include "kudu/gutil/strings/numbers.h" Is this necessary for the updated version? PS3, Line 138: string Nit: consider 'const string str = ...' or 'static const string str = ...' just to express it's not about to change during the test. PS3, Line 140: Status s = Subprocess::Call({"/bin/sh", "-c", cmd_str}, nullptr, &stderr); : ASSERT_TRUE(s.ok()); Nit: since 's' is used only for ASSERT_TRUE(s.ok()) check, consider replacing there two strings with one like ASSERT_OK(Subprocess::Call(...)); PS3, Line 146: s = Subprocess::Call({"/bin/ls", "/dev/null"}, &stdout, nullptr); : ASSERT_TRUE(s.ok()); Ditto for merging these two strings into ASSERT_OK(...) PS3, Line 150: s = Subprocess::Call({"/bin/ls", "/dev/zero"}, nullptr, nullptr); : ASSERT_TRUE(s.ok()); Ditto for merging these two strings into ASSERT_OK(...) -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
