[
https://issues.apache.org/jira/browse/HDFS-10450?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15471315#comment-15471315
]
Bob Hansen commented on HDFS-10450:
-----------------------------------
Thanks, [~James C]. Lots of good feedback.
{quote}
chosen_mech_.mechanism = std::string("", 0);
chosen_mech_.protocol = std::string("", 0);
chosen_mech_.serverid = std::string("", 0);
chosen_mech_.challenge = std::string("", 0);
Any reason to do explicit string instantiation with count here rather than just
assign ""?
{quote}
Replaced with chosen_mech_ = SaslMethod();
{quote}
I'm guessing the "// for()" was for your use while debugging or something and
could be removed.
{quote}
Copypasta dirt. Fixed.
{quote}
Are we adding authors to files now? Or was this something added by an IDE?
{quote}
The latter. Fixed.
{quote}
using namespace hdfs;
Let's get rid of the "using namespace hdfs" in the header. Doesn't look like
it's required anymore anyway.
{quote}
More copypasta that slipped by. Fixed.
bq. Nit: make the assignment to challenge line up with the rest of the member
assignments.
Done
{quote}
+ for (int i = 0; i < pb_auths.size(); ++i) {
+ auto pb_auth = pb_auths.Get(i);
+ AuthInfo::AuthMethod method = ParseMethod(pb_auth.method())
Could you use the full type name for pb_auth or typedef it here?
{quote}
Fixed. It's RpcSaslProto_SaslAuth, which starts getting a bit wordy in the
source.
{quote}
friend int getrealm(void *, int, const char **availrealms
__attribute__((unused)),
const char **);
Why do we need the attribute unused here?
{quote}
More dirt left over from the originator. I fixed it in the implementation, but
not the declaration.
{quote}
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.
{quote}
per_connection_callbacks is required by the Cyrus SASL library to be a
specially-terminated array of structs. We're (without heroic effort) limited
by the public interface of the library we're using.
bq. Nit: CySaslEngine::InitCyrusSasl is only indented by 1 space so it's a bit
harder to read.
Fixed
{quote}
// 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);
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.
{quote}
The strings are copied by the engine in the call to sasl_client_new, so I think
we're good on that one.
> 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]
