Alexey Serbin has posted comments on this change. Change subject: TLS-negotiation [6/n]: Refactor RPC negotiation ......................................................................
Patch Set 10: (13 comments) Overall looks great, just a few nits. Probably, I'll do another pass after digesting the information bit more. I also have some a question about memory management in the code of ssl_socket.cc (not the subject of this change, though). http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/connection.h File src/kudu/rpc/connection.h: PS10, Line 159: take_socket nit: release_socket ? PS10, Line 163: set_socket nit: adopt_socket ? http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/negotiation.h File src/kudu/rpc/negotiation.h: PS10, Line 25: class Socket; Why is it necessary in here? http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/reactor.cc File src/kudu/rpc/reactor.cc: PS10, Line 345: std::unique_ptr<Socket> new_socket; : if (reactor()->messenger()->ssl_enabled()) { : new_socket = reactor()->messenger()->ssl_factory()->CreateSocket(sock.Release(), false); : } else { : new_socket.reset(new Socket(sock.Release())); : } : : // Register the new connection in our map. : *conn = new Connection(this, conn_id.remote(), std::move(new_socket), Connection::CLIENT); : nit: consider adding an embedded scope for this to signify that new_socket variable is no longer available for usage after std::move(new_socket) was called. http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/sasl_helper.h File src/kudu/rpc/sasl_helper.h: PS10, Line 30: namespace google { : namespace protobuf { : class MessageLite; : } // namespace protobuf : } // namespace google nit: is this needed? PS10, Line 38: class MonoTime; nit: is this needed? http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: PS10, Line 20: #include <sasl/sasl.h> nit: could you add a comment about the reason of putting this before the block of the system headers? PS10, Line 56: * nit: since the asterisk disposition for other pointer-like parameters has been updated for some parameters, consider doing so for the rest. PS10, Line 61: * ditto PS10, Line 453: E nit: I'm not sure whether we have already adopted the idea of starting those messages from non-capital letters for 'easy log chaining', but if so, consider replacing this lower-case letters. You could talk to Todd about this. At least, in one of his reviews he asked me to change the messages. I think the convention is originated from the Golang code style guide. http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/rpc/server_negotiation.h File src/kudu/rpc/server_negotiation.h: PS10, Line 101: take_ nit: consider renaming into 'release_socket' http://gerrit.cloudera.org:8080/#/c/5760/10/src/kudu/security/ssl_socket.cc File src/kudu/security/ssl_socket.cc: PS10, Line 126: (*nwritten < frame_size) Is it possible for in-the-middle Write() to return an error even if *nwritten == frame_size? PS10, Line 167: SSL_free(ssl_); Is it possible that an object of this type is destructed without prior call of the Close() method? If yes, would it make sense to de-allocate ssl_ in the destructor? -- To view, visit http://gerrit.cloudera.org:8080/5760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I567b62f3341a1e74342c30c76b63f2ca5d7990bd Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
