hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bob Hansen (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-10450) libhdfs++: Implement Cyrus SASL implementation in sasl_enigne.cc
Date Wed, 07 Sep 2016 17:51:21 GMT

    [ 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: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org


Mime
View raw message