Mike Percy has posted comments on this change. Change subject: Undefined behavior in TlsSocket::Writev() ......................................................................
Patch Set 4: (3 comments) The fix looks good. +1 to what Dan said http://gerrit.cloudera.org:8080/#/c/7141/4/src/kudu/security/tls_socket.cc File src/kudu/security/tls_socket.cc: PS4, Line 80: // Don't want to update the number written if there was an error. : // We don't return yet, since we want to turn off the Tcp Cork. I think this comment can be simplified by saying something like: // If Write() returns an error then the value of 'nwritten' is undefined. PS4, Line 84: // nwritten should have the correct amount written. How about: // If we were unable to write the requested number of bytes we return early, but not before uncorking. Or something similar. http://gerrit.cloudera.org:8080/#/c/7141/4/src/kudu/security/tls_socket.h File src/kudu/security/tls_socket.h: Line 35: // If the result is an error then the value in nwritten is not valid. > Could you instead add these docs to the parent class in socket.h, they shou +1 for moving to class declaration for Socket. For things like this that are wrapping POSIX calls, I usually get inspiration from the man pages i.e. http://man7.org/linux/man-pages/man2/write.2.html So I would just write something like // Write up to 'amt' bytes from 'buf' to the socket. The number of bytes // actually written will be stored in 'nwritten'. If an error is returned, // the value of 'nwritten' is undefined. I don't think you need to detail what happens when amt == 0 since it seems intuitive to me. -- To view, visit http://gerrit.cloudera.org:8080/7141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia5b5bbb3fd2ec8fcd1a48873446f3aa09546eaac Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Edward Fancher <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Edward Fancher <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-HasComments: Yes
