impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5221: Fix TSaslTransport negotiation order
Date Thu, 08 Jun 2017 22:38:18 GMT
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 <sailesh@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message