Alexey Serbin has posted comments on this change. Change subject: KuduRPC integration with OpenSSL ......................................................................
Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: PS4, Line 626: static_cast<SSLSocket*>(socket_.get()); What happens if the socket_ does not actually contains an object of SSLSocket type? Consider using dynamic_cast instead and verify that dynamic_cast returns non-null pointer. If it returns null, return an error. http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/ssl_factory.cc File src/kudu/util/net/ssl_factory.cc: PS4, Line 66: [](SSL_CTX* x) { SSL_CTX_free(x); } Nit: if ctx_ were defined as unique_ptr<SSL_CTX, std::function<void(SSL_CTX*)>>, here it would be possible to write just ctx_(nullptr, SSL_CTX_free) http://gerrit.cloudera.org:8080/#/c/4789/4/src/kudu/util/net/ssl_factory.h File src/kudu/util/net/ssl_factory.h: PS4, Line 17: #ifndef KUDU_UTIL_NET_SSL_FACTORY_H : #define KUDU_UTIL_NET_SSL_FACTORY_H nit: consider using #pragma once instead PS4, Line 37: function nit: function --> method PS4, Line 39: OpenSSLInit Is this forward declaration? PS4, Line 51: void Teardown(); As far as I know, the OpenSSL library contains some statics which cannot be unset during tear-down, making it non-reentrant. Given that, do you think it makes sense to make this method public? Also, I could not find the implementation of this method in the corresponding .cc file. PS4, Line 57: nit: an extra space PS4, Line 64: SSLSocket* Consider returning unique_ptr<SSLSocket> instead: the signature with smart pointer for return type makes users of the API less leak-prone. Besides, it explicitly expresses memory ownership for the returned pointers, so no convention/comments are needed. -- To view, visit http://gerrit.cloudera.org:8080/4789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I27167faa4e6a78e59b46093055b16682c93af0ea Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
