Alexey Serbin has posted comments on this change. Change subject: [util/crypto] certificate management (part 1) ......................................................................
Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/4799/7/src/kudu/security/crypto/cert_management-test.cc File src/kudu/security/crypto/cert_management-test.cc: PS7, Line 45: Substitute > Use JoinPathSegments from path_util.h here and below. Done PS7, Line 84: generatation > generation Done http://gerrit.cloudera.org:8080/#/c/4799/6/src/kudu/security/crypto/cert_management.cc File src/kudu/security/crypto/cert_management.cc: Line 100: return Status::IllegalState("Not initialized"); > > Also, it makes sense to transfer ownership to the parent key: calling les ok, this or that, it's not crucial. Removing rsa.release() would make the code cleaner, so I'll go the way you suggested. http://gerrit.cloudera.org:8080/#/c/4799/7/src/kudu/security/crypto/cert_management.cc File src/kudu/security/crypto/cert_management.cc: PS7, Line 79: https://www.openssl.org/docs/manmaster/apps/x509v3_config.html > This URL doesn't resolve. Perhaps https://www.openssl.org/docs/man1.0.1/ap Interesting. I just copied it from browser URL field at the time of writing. Thank you for the update! I'm updating this and one other reference which had manmaster instead of man1.0.1 Line 85: const_cast<char*>("critical,serverAuth,clientAuth"))); > It looks like those weren't actually removed? There are now two calls to A That was about the next patch. Line 318: if (!ret) { > I think this would be simpler as a CHECK_NOTNULL. Having a return statemen LOG(DFATAL) works only for debug builds, right? But I agree -- in this context CHECK_NOTNULL() is better. Will update. -- To view, visit http://gerrit.cloudera.org:8080/4799 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I69c1da97e6d013a034aefda59988b593ae1d6304 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
