Alexey Serbin has posted comments on this change.

Change subject: KUDU-2091: Certificates with intermediate CA's do not work with 

Patch Set 1:

File src/kudu/rpc/

PS1, Line 162: CHECK_OK

PS1, Line 171: LOG(INFO) << "Connecting to " << server_addr.ToString();
Is this crucial to have this info line in the test?  As I understand, this 
might be useful only if test fails -- if really necessary, consider using 
SCOPED_TRACE() with the server_addr.ToString().

PS1, Line 178: for (int i = 0; i < 10; i++) {
nit: just curious why calling it once is not enough
File src/kudu/security/

Line 214: void Cert::AdoptAndAddRefRawData(STACK_OF(X509)* data) {
Why not just to use X509_chain_up_ref() and remove the constraint on 
File src/kudu/security/cert.h:

PS1, Line 76: STACK_OF(X509)
nit: this is RawDataType typedef
File src/kudu/security/openssl_util.h:

Line 128: STACK_OF(X509)* PEM_read_STACK_OF_X509(BIO* bio, void* unused1, 
pem_password_cb* unused2,
> yea, I think this is better off in the .cc file anyway since it's too large
once moved into the .cc file, consider adding SCOPED_OPENSSL_NO_PENDING_ERRORS

PS1, Line 178:   STACK_OF(X509)* sk = sk_X509_new_null();
             :   if (sk_X509_push(sk, x) == 0) return nullptr;
There might be a memory leak if the 'sk' is allocated but xk_X509_push() 
failed.  Consider using ssl_make_unique() construct (you could look around for 
File src/kudu/security/test/

Line 499: Status CreateTestSSLCertSignedByChain(const string& dir,
> add a comment explaining how this was generated?
nit: indentation for the last 3 parameters
File src/kudu/security/

PS1, Line 194:   if (cert) remote_cert_.AdoptX509(cert);
nit: consider keeping the original brace style here

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <>
Gerrit-Reviewer: Alexey Serbin <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to