@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. > @@ -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. > @@ -465,7 +574,7 @@ class XrdAccSciTokens : public XrdAccAuthorize, public XrdSciTokensHelper pthread_rwlock_unlock(&m_config_lock); XrdSysRWLock > @@ -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 > @@ -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 > @@ -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 > @@ -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 > @@ -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. > @@ -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 or view it on GitHub: https://github.com/xrootd/xrootd/pull/1644#pullrequestreview-961313807 You are receiving this because you are subscribed to this thread. Message ID: <[log in to unmask]> ######################################################################## 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