@abh3 approved this pull request.

Nothing that would stop the merge but three suggestions to improve the code which I would appreciate you revisit and apply the suggested improvements.

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -296,7 +391,7 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper
         pthread_rwlock_init(&m_config_lock, nullptr);

You should consider using XrdSysRWLock as this would be very helpful in debugging lock/unlock problems in the future.

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -465,7 +574,7 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper
         pthread_rwlock_unlock(&m_config_lock);

XrdSysRWLock

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -602,7 +712,7 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper
         auto enf = enforcer_create(token_issuer.c_str(), &m_audiences_array[0], &err_msg);
         pthread_rwlock_unlock(&m_config_lock);

XrdSysRWLock

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -636,15 +746,15 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper
         auto iter = m_issuers.find(token_issuer);
         if (iter == m_issuers.end()) {
             pthread_rwlock_unlock(&m_config_lock);

XrdSysRWLock

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -636,15 +746,15 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper
         auto iter = m_issuers.find(token_issuer);
         if (iter == m_issuers.end()) {
             pthread_rwlock_unlock(&m_config_lock);
-            m_log.Emsg("GenerateAcls", "Authorized issuer without a config.");
+            m_log.Log(LogMask::Warning, "GenerateAcls", "Authorized issuer without a config.");
             scitoken_destroy(token);
             return false;
         }
         const auto &config = iter->second;
         value = nullptr;
         if (scitoken_get_claim_string(token, "sub", &value, &err_msg)) {
             pthread_rwlock_unlock(&m_config_lock);

XrdSysRWLock

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -942,7 +1093,7 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper
         }
 
         if (issuers.empty()) {
-            m_log.Emsg("Reconfig", "No issuers configured.");
+            m_log.Log(LogMask::Warning, "Reconfig", "No issuers configured.");
         }
 
         pthread_rwlock_wrlock(&m_config_lock);

XrdSysRWLock

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -87,6 +122,46 @@ XrdAccPrivs AddPriv(Access_Operation op, XrdAccPrivs privs)
     return static_cast<XrdAccPrivs>(new_privs);
 }
 
+const std::string OpToName(Access_Operation op) {

Likely this should be made a generic utility in XrdAcc as another implementation of the same thing exists there. Something to put in the todo list.

In src/XrdSciTokens/XrdSciTokensAccess.cc:

> @@ -29,6 +32,38 @@ XrdVERSIONINFO(XrdAccAuthorizeObjAdd, XrdAccSciTokens);
 
 namespace {
 
+enum LogMask {

This has gotten a life of its own and I see the same implementation in three other modules (XrdMacaroonsHandler.hh, XrdTpcTPC.hh, and XrdVomsMapfile.hh) making this the fourth sych implementation. Shouldn't this be abstracted into a utility function?

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <xrootd/xrootd/pull/1644/review/961313807@github.com>

Use REPLY-ALL to reply to list

To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1