Henry Robinson has posted comments on this change. Change subject: IMPALA-5221: Fix TSaslTransport negotiation order ......................................................................
Patch Set 1: (11 comments) Looks pretty good. Have you confirmed this works with LDAP as well as GSSAPI? http://gerrit.cloudera.org:8080/#/c/7116/1//COMMIT_MSG Commit Message: PS1, Line 7: Fix TSaslTransport negotiation order specifically: don't re-use old sasl contexts, right? http://gerrit.cloudera.org:8080/#/c/7116/1/be/src/transport/TSasl.cpp File be/src/transport/TSasl.cpp: PS1, Line 119: this->service = service; : this->serverFQDN = serverFQDN; : this->callbacks = callbacks; : chosenMech = mechList = mechanisms; : authComplete = false; : clientStarted = false; move these initializations to the member initializer list (e.g. service(service), serverFQDN(serverFQDN) ...). The constant ones can be initialized in their declarations. Line 129: result = sasl_client_new(service.c_str(), serverFQDN.c_str(), NULL, NULL, callbacks, IIUC, we should DCHECK(conn == nullptr) here to confirm we're not hitting the bug. PS1, Line 128: int result; : result = int result = ... PS1, Line 208: this->service = service; : this->serverFQDN = serverFQDN; : this->callbacks = callbacks; : this->flags = flags; : this->userRealm = userRealm; : authComplete = false; : serverStarted = false; move to member initializer list etc. PS1, Line 218: int result; : result int result = ... http://gerrit.cloudera.org:8080/#/c/7116/1/be/src/transport/TSasl.h File be/src/transport/TSasl.h: PS1, Line 61: setupNegotiation I think setupSaslContext is a better name. PS1, Line 66: dispose can't this lead to calling sasl_dispose(&nullptr) if there's a negotiation error? (i.e. dispose() is called from resetNegotiation() and then from the d'tor). Or if the TSasl is destroyed without ever actually setting up the negotiation. PS1, Line 69: virtual void dispose() { : sasl_dispose(&conn); : conn = NULL; : } can this be protected? http://gerrit.cloudera.org:8080/#/c/7116/1/be/src/transport/TSaslClientTransport.h File be/src/transport/TSaslClientTransport.h: PS1, Line 51: Setup Set up http://gerrit.cloudera.org:8080/#/c/7116/1/be/src/transport/TSaslServerTransport.h File be/src/transport/TSaslServerTransport.h: PS1, Line 44: /* Setup the Sasl server state for a connection. */ If this is a no-op, the comment shold be here. -- To view, visit http://gerrit.cloudera.org:8080/7116 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7d509e14dfe46fa28d0626cf84daf6de82955d80 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
