[
https://issues.apache.org/jira/browse/HDFS-10450?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15471063#comment-15471063
]
James Clampffer commented on HDFS-10450:
----------------------------------------
First pass of simple things, I'm going to go some reading about sasl and come
back for the protocol level stuff.
In sasl_engine.cc
{code}
chosen_mech_.mechanism = std::string("", 0);
chosen_mech_.protocol = std::string("", 0);
chosen_mech_.serverid = std::string("", 0);
chosen_mech_.challenge = std::string("", 0);
{code}
Any reason to do explicit string instantiation with count here rather than just
assign ""?
sasl_engine.cc:
{code}
for (SaslMethod auth: resp_auths) {
if ( auth.mechanism != "GSSAPI") continue; // Hack: only GSSAPI for now
// do a proper deep copy of the vector element
// that we like, because the original v ector will go away:
chosen_mech_.mechanism = auth.mechanism;
chosen_mech_.protocol = auth.protocol;
chosen_mech_.serverid = auth.serverid;
chosen_mech_.challenge = auth.challenge;
return auth.mechanism.c_str();
} // for()
{code}
I'm guessing the "// for()" was for your use while debugging or something and
could be removed. I do think they are helpful when ending namespaces and stuff
but here it's pretty clear what the body of the loop is.
in sasl_protocol.h
{code}
/*
* File: SaslProtocol.h
* Author: bhansen
*
* Created on May 11, 2016, 8:51 AM
*/
{code}
Are we adding authors to files now? Or was this something added by an IDE?
This showed up in a few files.
in sasl_protocol.h
{code}
using namespace hdfs;
namespace hdfs {
{code}
Let's get rid of the "using namespace hdfs" in the header. Doesn't look like
it's required anymore anyway.
in sasl_protocol.cc
{code}
+ new_method.serverid = pb_auth.serverid();
+ new_method.challenge = pb_auth.has_challenge() ?
+ pb_auth.challenge() : "";
+ resp_auths.push_back(new_method);
{code}
Nit: make the assignment to challenge line up with the rest of the member
assignments.
in sasl_protocol.cc
{code}
+ for (int i = 0; i < pb_auths.size(); ++i) {
+ auto pb_auth = pb_auths.Get(i);
+ AuthInfo::AuthMethod method = ParseMethod(pb_auth.method())
{code}
Could you use the full type name for pb_auth or typedef it here? Theres another
level of auto type direction to get to pb_auths so it's trickier than it needs
to be to figure out exactly what pb_auth is. I try to avoid auto for doing
things like "auto foo = bar(); baz(foo)" because it's just hard to follow, and
ambiguous if the auto deduced var a ref type or const qualified etc. This
happens in a few other places and it's harder to follow for someone who isn't
used to the sasl stuff like myself.
in cyrus_sasl_engine.h
{code}
friend int getrealm(void *, int, const char **availrealms
__attribute__((unused)),
const char **);
{code}
Why do we need the attribute unused here? Add a comment about that. I'd also
consider changing this to a vector of std strings given our issues with memory
stomps in the past.
in cyrus_sasl_engine.h
Make CyCaslEngine::per_connection_callbacks_ a struct of function pointers
rather than an vector so the compiler can statically check member access and we
can avoid tracing down issues where we key in with the wrong thing. Was the
plan to use this as a jump table in a DFA? If that's the case a vector (or
preferably an array) is fine I can't find anything to indicate that.
Nit: CySaslEngine::InitCyrusSasl is only indented by 1 space so it's a bit
harder to read.
in cyrus_sasl_engine:
{code}
// Initialize the sasl_li brary with per-connection configuration:
const char * fqdn = chosen_mech_.serverid.c_str();
const char * proto = chosen_mech_.protocol.c_str();
rc = sasl_client_new( proto, fqdn, NULL, NULL,
&per_connection_callbacks_[0], 0, &conn_);
if (rc != SASL_OK) return SaslError(rc);
{code}
Is CySaslEngine::InitCyrusSasl guarded by a lock at a higher level? My concern
is holding onto the result of c_str() calls and if one of those strings was to
change in another InitCyrusSasl call.
> libhdfs++: Implement Cyrus SASL implementation in sasl_enigne.cc
> ----------------------------------------------------------------
>
> Key: HDFS-10450
> URL: https://issues.apache.org/jira/browse/HDFS-10450
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Reporter: Bob Hansen
> Assignee: Bob Hansen
> Attachments: HDFS-10450.HDFS-8707.000.patch,
> HDFS-10450.HDFS-8707.001.patch, HDFS-10450.HDFS-8707.002.patch
>
>
> The current sasl_engine implementation was proven out using GSASL, which is
> does not have an ASF-approved license. It included a framework to use Cyrus
> SASL (libsasl2.so) instead; we should complete that implementation.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
