Todd Lipcon has posted comments on this change. Change subject: [security] groundwork for cert signing service ......................................................................
Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.cc File src/kudu/security/crypto/cert_management.cc: Line 113: "BIO_get_mem_ptr() failed"); > Yes, but why is it better than just returning an error? I generally feel like if the only way you'd hit this is a memory allocation failure, then it's probably not worth worrying about trying to do any handling/recovery. eg returning a bad status itself will have to malloc and that'll probably fail, or else something else will fail shortly thereafter. In general in the way people typically configure Linux, malloc never fails, it just overcommits and later kills you from the OOM killer, so trying to gracefully handle these paths just means that we'll have code paths that never get executed. Line 315: "Error setting parameters for RSA key generation"); > Yes, it's possible to replace it with CHECK, but I would prefer to keep it See above -- yea, I think crashing on malloc failure is appropriate. Line 349: CERT_RET_IF_NULL(name, "Null subject name sub-structure in X509 CSR"); > That field might be null in case of transient memory allocation failure. D yep (see above) -- To view, visit http://gerrit.cloudera.org:8080/5671 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
