Alexey Serbin has posted comments on this change. Change subject: KUDU-1674: Fix SubProcess:Call SEGV when trying to capture stderr alone ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/4594/1/src/kudu/util/subprocess-test.cc File src/kudu/util/subprocess-test.cc: Line 141: ASSERT_FALSE(stderr.empty()); Nit: since you check for the presence on non-empty substring in stderr, this check could be omitted. Line 148: ASSERT_FALSE(stdout.empty()); ditto. 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 implementation details of the vector, and it might become 4 even if the vector is empty. Also, why is it necessary to check it here? Does it make sense to be more specific and check that the result vector contains appropriate number of elements in accordance with the size of the fds passed in? Line 504: *stdout_out = std::move(outv.front()); Do you think it's worth to check that !outv.empty() before calling outv.front()? Line 507: *stderr_out = std::move(outv.back()); Ditto here: do you think it's worth to check that !outv.empty() before calling outv.back()? -- 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 <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: Kudu Jenkins Gerrit-HasComments: Yes
