Alexey Serbin has posted comments on this change. Change subject: KuduRPC integration with OpenSSL ......................................................................
Patch Set 2: (17 comments) http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/testutil/server-cert.pem File src/kudu/testutil/server-cert.pem: Consider having these in code of the test and writing into a file under unit test directory. http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/testutil/server-key.pem File src/kudu/testutil/server-key.pem: Instead of adding this file, consider having this as a string in corresponding test class/file and writing it into the unit test directory during run-time. http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_factory.cc File src/kudu/util/net/ssl_factory.cc: PS2, Line 29: Mutex* Consider using std::mutex instead. Also, what is the reason of allocating them dynamically, i.e. why storing pointers instead of objects themselves? PS2, Line 42: return pthread_self(); This is not portable: pthread_t can be any type (like a structure). Instead, consider using something like: std::hash<std::thread::id> hasher; return hasher(std::this_thread::get_id()); PS2, Line 52: new Mutex Where is the corresponding delete part? PS2, Line 57: CRYPTO_set_id_callback >From 1.0.0, CRYPTO_set_id_callback() is replaced/deprecated by >CRYPTO_THREADID_set_callback. PS2, Line 60: GoogleOnceType Consider using std::flag_once from the standard library. PS2, Line 63: GoogleOnceInit Consider using std::call_once() from the standard library. http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_factory.h File src/kudu/util/net/ssl_factory.h: PS2, Line 20: #include <openssl/err.h> : #include <openssl/ssl.h> : #include <string> : #include <sys/uio.h> nit: consider re-ordering these as prescribed by google style guide: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_socket.cc File src/kudu/util/net/ssl_socket.cc: Line 19: #include <openssl/err.h> nit: please add an empty line between these: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes PS2, Line 54: if (*cn_size < 0) return std::string() Is it necessary to release memory allocated by cn_asn1 if returning from here? PS2, Line 105: X509* cert Consider using unique_ptr with custom deleter to avoid memory leaks in case of returning from this method prior to reaching line 131 PS2, Line 116: return Is it necessary to release cert before returning? PS2, Line 122: return Is it necessary to release cert before returning? PS2, Line 129: return Is it necessary to release cert before returning? http://gerrit.cloudera.org:8080/#/c/4789/2/src/kudu/util/net/ssl_socket.h File src/kudu/util/net/ssl_socket.h: PS2, Line 20: #include <openssl/ssl.h> : #include <sys/uio.h> : #include <string> nit: consider re-ordering these as prescribed by google style guide: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes PS2, Line 34: explicit Is it really needed here? -- 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: 2 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
