From a5b92ee4ba4f4c34e6b46c8c99d61c42bb8d0442 Mon Sep 17 00:00:00 2001 From: Michael Davis <michael.davis@cern.ch> Date: Wed, 24 Jan 2024 10:28:38 +0100 Subject: [PATCH 1/6] Refactors Errnum class --- common/exception/Errnum.cpp | 48 +++++++++++++------------ common/exception/Errnum.hpp | 70 +++++++++++++++++++----------------- common/exception/XrootCl.cpp | 8 ++--- common/exception/XrootCl.hpp | 20 +++++------ common/utils/utils.cpp | 1 + 5 files changed, 77 insertions(+), 70 deletions(-) diff --git a/common/exception/Errnum.cpp b/common/exception/Errnum.cpp index 62134701ea..c7d05c3c9d 100644 --- a/common/exception/Errnum.cpp +++ b/common/exception/Errnum.cpp @@ -18,54 +18,58 @@ #include "common/exception/Errnum.hpp" #include "common/utils/utils.hpp" -#include <errno.h> -#include <string.h> +namespace cta::exception { -using namespace cta::exception; - -Errnum::Errnum(std::string what):Exception("") { +Errnum::Errnum(std::string_view what) : Exception() { m_errnum = errno; ErrnumConstructorBottomHalf(what); } -Errnum::Errnum(int err, std::string what):Exception("") { +Errnum::Errnum(int err, std::string_view what) : Exception() { m_errnum = err; ErrnumConstructorBottomHalf(what); } -void Errnum::ErrnumConstructorBottomHalf(const std::string & what) { +void Errnum::ErrnumConstructorBottomHalf(std::string_view what) { m_strerror = utils::errnoToString(m_errnum); std::stringstream w2; - if (what.size()) + if(what.size()) { w2 << what << " "; + } w2 << "Errno=" << m_errnum << ": " << m_strerror; getMessage() << w2.str(); } -void Errnum::throwOnReturnedErrno (const int err, const std::string &context) { - if (err) throw Errnum(err, context); +void Errnum::throwOnReturnedErrno(int err, std::string_view context) { + if(err) throw Errnum(err, context); +} + +void Errnum::throwOnNonZero(int status, std::string_view context) { + if(status) throw Errnum(context); } -void Errnum::throwOnNonZero(const int status, const std::string &context) { - if (status) throw Errnum(context); +void Errnum::throwOnZero(int status, std::string_view context) { + if(!status) throw Errnum(context); } -void Errnum::throwOnZero(const int status, const std::string &context) { - if (!status) throw Errnum(context); +void Errnum::throwOnNull(const void* const ptr, std::string_view context) { + if(nullptr == ptr) throw Errnum(context); } -void Errnum::throwOnNull(const void *const f, const std::string &context) { - if (nullptr == f) throw Errnum(context); +void Errnum::throwOnNegative(int ret, std::string_view context) { + if(ret < 0) throw Errnum(context); } -void Errnum::throwOnNegative(const int ret, const std::string &context) { - if (ret < 0) throw Errnum(context); +void Errnum::throwOnMinusOne(int ret, std::string_view context) { + if(ret == -1) throw Errnum(context); } -void Errnum::throwOnMinusOne(const int ret, const std::string &context) { - if (-1 == ret) throw Errnum(context); +void Errnum::throwOnMinusOne(ssize_t ret, std::string_view context) { + if(ret == -1) throw Errnum(context); } -void Errnum::throwOnNegativeErrnoIfNegative(const int ret, const std::string& context) { - if (ret < 0) throw Errnum(-ret, context); +void Errnum::throwOnNegativeErrnoIfNegative(int ret, std::string_view context) { + if(ret < 0) throw Errnum(-ret, context); } + +} // namespace cta::exception diff --git a/common/exception/Errnum.hpp b/common/exception/Errnum.hpp index 3f94ec4794..63ed5fcceb 100644 --- a/common/exception/Errnum.hpp +++ b/common/exception/Errnum.hpp @@ -17,42 +17,46 @@ #pragma once -#include "Exception.hpp" -#include <functional> -#include <system_error> +#include "common/exception/Exception.hpp" namespace cta::exception { - class Errnum: public cta::exception::Exception { - public: - explicit Errnum(std::string what = ""); - Errnum(int err, std::string what = ""); - virtual ~Errnum() = default; - int errorNumber() const { return m_errnum; } - std::string strError() const { return m_strerror; } - static void throwOnReturnedErrno(const int err, const std::string &context = ""); - template <typename F> - static void throwOnReturnedErrnoOrThrownStdException (F f, const std::string &context = "") { - try { - throwOnReturnedErrno(f(), context); - } catch (Errnum &) { - throw; // Let the exception of throwOnReturnedErrno pass through. - } catch (std::error_code & ec) { - throw Errnum(ec.value(), context + " Got an std::error_code: " + ec.message()); - } catch (std::exception & ex) { - throw Exception(context + " Got a standard exception: " + ex.what()); - } +class Errnum : public Exception { +public: + explicit Errnum(std::string_view what = ""); + Errnum(int err, std::string_view what = ""); + virtual ~Errnum() = default; + + int errorNumber() const { return m_errnum; } + std::string strError() const { return m_strerror; } + + static void throwOnReturnedErrno(int err, std::string_view context = ""); + static void throwOnNonZero(int status, std::string_view context = ""); + static void throwOnZero(int status, std::string_view context = ""); + static void throwOnNull(const void *const f, std::string_view context = ""); + static void throwOnNegative(int ret, const std::string_view context = ""); + static void throwOnMinusOne(int ret, const std::string_view context = ""); + static void throwOnMinusOne(ssize_t ret, const std::string_view context = ""); + static void throwOnNegativeErrnoIfNegative(int ret, std::string_view context = ""); + + template <typename F> + static void throwOnReturnedErrnoOrThrownStdException(F f, std::string_view context = "") { + try { + throwOnReturnedErrno(f(), context); + } catch(Errnum&) { + throw; // Let the exception of throwOnReturnedErrno pass through + } catch(std::error_code& ec) { + throw Errnum(ec.value(), std::string(context) + " Got a std::error_code: " + ec.message()); + } catch(std::exception& ex) { + throw Exception(std::string(context) + " Got a std::exception: " + ex.what()); } - static void throwOnNonZero(const int status, const std::string &context = ""); - static void throwOnZero(const int status, const std::string &context = ""); - static void throwOnNull(const void *const f, const std::string &context = ""); - static void throwOnNegative(const int ret, const std::string &context = ""); - static void throwOnMinusOne(const int ret, const std::string &context = ""); - static void throwOnNegativeErrnoIfNegative(const int ret, const std::string &context = ""); - protected: - void ErrnumConstructorBottomHalf(const std::string & what); - int m_errnum; - std::string m_strerror; - }; + } + +private: + void ErrnumConstructorBottomHalf(std::string_view what); + + int m_errnum; + std::string m_strerror; +}; } // namespace cta::exception diff --git a/common/exception/XrootCl.cpp b/common/exception/XrootCl.cpp index 52b79c3a90..dad93074df 100644 --- a/common/exception/XrootCl.cpp +++ b/common/exception/XrootCl.cpp @@ -22,8 +22,8 @@ namespace cta::exception { -XrootCl::XrootCl(const XrdCl::XRootDStatus& status, const std::string & what) { - if (!what.empty()) { +XrootCl::XrootCl(const XrdCl::XRootDStatus& status, std::string_view what) { + if(!what.empty()) { getMessage() << what << " "; } getMessage() << status.ToStr().c_str(); @@ -32,8 +32,8 @@ XrootCl::XrootCl(const XrdCl::XRootDStatus& status, const std::string & what) { << " status:" << status.status; } -void XrootCl::throwOnError(const XrdCl::XRootDStatus& status, const std::string& context) { - if (!status.IsOK()) { +void XrootCl::throwOnError(const XrdCl::XRootDStatus& status, std::string_view context) { + if(!status.IsOK()) { throw XrootCl(status, context); } } diff --git a/common/exception/XrootCl.hpp b/common/exception/XrootCl.hpp index 57b42807e1..8376985c0e 100644 --- a/common/exception/XrootCl.hpp +++ b/common/exception/XrootCl.hpp @@ -17,25 +17,23 @@ #pragma once -#include <string> - -#include "Exception.hpp" - #include <xrootd/XrdCl/XrdClXRootDResponses.hh> +#include "Exception.hpp" namespace cta::exception { /** - * A class turning the XrootCl (xroot 4 object client) error codes - * into castor exceptions. + * A class turning the XrootCl (xroot 4 object client) error codes into CTA exceptions */ -class XrootCl : public cta::exception::Exception { - public: - XrootCl(const XrdCl::XRootDStatus & status, const std::string & context); +class XrootCl : public Exception { +public: + XrootCl(const XrdCl::XRootDStatus& status, std::string_view context); virtual ~XrootCl() = default; const XrdCl::XRootDStatus & xRootDStatus() const { return m_status; } - static void throwOnError(const XrdCl::XRootDStatus & status, const std::string& context = ""); - protected: + static void throwOnError(const XrdCl::XRootDStatus& status, std::string_view context = ""); + +private: XrdCl::XRootDStatus m_status; }; + } // namespace cta::exception diff --git a/common/utils/utils.cpp b/common/utils/utils.cpp index 90b5e1d7ab..ec4d0241ca 100644 --- a/common/utils/utils.cpp +++ b/common/utils/utils.cpp @@ -21,6 +21,7 @@ #include "common/utils/strerror_r_wrapper.hpp" #include "common/utils/utils.hpp" +#include <algorithm> #include <attr/xattr.h> #include <limits> #include <memory> -- GitLab From 8b05852d76650bac6c7531805570d65043c5df05 Mon Sep 17 00:00:00 2001 From: Michael Davis <michael.davis@cern.ch> Date: Wed, 24 Jan 2024 13:39:40 +0100 Subject: [PATCH 2/6] Removes unneeded methods from Errnum class --- common/exception/Errnum.cpp | 16 ------- common/exception/Errnum.hpp | 17 ------- common/exception/ExceptionTest.cpp | 33 ++----------- common/threading/Daemon.cpp | 4 +- disk/DiskFileImplementations.hpp | 11 ----- objectstore/BackendRados.cpp | 76 +++++++++++++++--------------- objectstore/BackendRados.hpp | 15 ++++++ objectstore/BackendTest.cpp | 7 +++ 8 files changed, 66 insertions(+), 113 deletions(-) diff --git a/common/exception/Errnum.cpp b/common/exception/Errnum.cpp index c7d05c3c9d..cb94c0f18d 100644 --- a/common/exception/Errnum.cpp +++ b/common/exception/Errnum.cpp @@ -48,28 +48,12 @@ void Errnum::throwOnNonZero(int status, std::string_view context) { if(status) throw Errnum(context); } -void Errnum::throwOnZero(int status, std::string_view context) { - if(!status) throw Errnum(context); -} - void Errnum::throwOnNull(const void* const ptr, std::string_view context) { if(nullptr == ptr) throw Errnum(context); } -void Errnum::throwOnNegative(int ret, std::string_view context) { - if(ret < 0) throw Errnum(context); -} - -void Errnum::throwOnMinusOne(int ret, std::string_view context) { - if(ret == -1) throw Errnum(context); -} - void Errnum::throwOnMinusOne(ssize_t ret, std::string_view context) { if(ret == -1) throw Errnum(context); } -void Errnum::throwOnNegativeErrnoIfNegative(int ret, std::string_view context) { - if(ret < 0) throw Errnum(-ret, context); -} - } // namespace cta::exception diff --git a/common/exception/Errnum.hpp b/common/exception/Errnum.hpp index 63ed5fcceb..3b871b9d15 100644 --- a/common/exception/Errnum.hpp +++ b/common/exception/Errnum.hpp @@ -32,25 +32,8 @@ public: static void throwOnReturnedErrno(int err, std::string_view context = ""); static void throwOnNonZero(int status, std::string_view context = ""); - static void throwOnZero(int status, std::string_view context = ""); static void throwOnNull(const void *const f, std::string_view context = ""); - static void throwOnNegative(int ret, const std::string_view context = ""); - static void throwOnMinusOne(int ret, const std::string_view context = ""); static void throwOnMinusOne(ssize_t ret, const std::string_view context = ""); - static void throwOnNegativeErrnoIfNegative(int ret, std::string_view context = ""); - - template <typename F> - static void throwOnReturnedErrnoOrThrownStdException(F f, std::string_view context = "") { - try { - throwOnReturnedErrno(f(), context); - } catch(Errnum&) { - throw; // Let the exception of throwOnReturnedErrno pass through - } catch(std::error_code& ec) { - throw Errnum(ec.value(), std::string(context) + " Got a std::error_code: " + ec.message()); - } catch(std::exception& ex) { - throw Exception(std::string(context) + " Got a std::exception: " + ex.what()); - } - } private: void ErrnumConstructorBottomHalf(std::string_view what); diff --git a/common/exception/ExceptionTest.cpp b/common/exception/ExceptionTest.cpp index bf209f8449..c7fe6cfea9 100644 --- a/common/exception/ExceptionTest.cpp +++ b/common/exception/ExceptionTest.cpp @@ -83,46 +83,21 @@ namespace unitTests { TEST(cta_exceptions, Errnum_throwers) { /* throwOnReturnedErrno */ ASSERT_NO_THROW(cta::exception::Errnum::throwOnReturnedErrno(0, "Context")); - ASSERT_THROW(cta::exception::Errnum::throwOnReturnedErrno(ENOSPC, "Context"), - cta::exception::Errnum); - - ASSERT_NO_THROW(cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([](){ return 0; }, "Context")); - ASSERT_THROW(cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([](){ return ENOSPC; }, "Context"), - cta::exception::Errnum); - ASSERT_THROW(cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([](){ throw std::error_code(ENOSPC, std::system_category()); return 0; }, "Context"), - cta::exception::Errnum); - ASSERT_THROW(cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([](){ throw std::exception(); return 0; }, "Context"), - cta::exception::Exception); - + ASSERT_THROW(cta::exception::Errnum::throwOnReturnedErrno(ENOSPC, "Context"), cta::exception::Errnum); /* throwOnNonZero */ errno = ENOENT; ASSERT_NO_THROW(cta::exception::Errnum::throwOnNonZero(0, "Context")); - ASSERT_THROW(cta::exception::Errnum::throwOnNonZero(-1, "Context"), - cta::exception::Errnum); + ASSERT_THROW(cta::exception::Errnum::throwOnNonZero(-1, "Context"), cta::exception::Errnum); /* throwOnMinusOne */ errno = ENOENT; ASSERT_NO_THROW(cta::exception::Errnum::throwOnMinusOne(0, "Context")); - ASSERT_THROW(cta::exception::Errnum::throwOnMinusOne(-1, "Context"), - cta::exception::Errnum); - - /* throwOnNegative */ - errno = ENOENT; - ASSERT_NO_THROW(cta::exception::Errnum::throwOnNegative(0, "Context")); - ASSERT_THROW(cta::exception::Errnum::throwOnNegative(-1, "Context"), - cta::exception::Errnum); + ASSERT_THROW(cta::exception::Errnum::throwOnMinusOne(-1, "Context"), cta::exception::Errnum); /* throwOnNull */ errno = ENOENT; ASSERT_NO_THROW(cta::exception::Errnum::throwOnNull(this, "Context")); - ASSERT_THROW(cta::exception::Errnum::throwOnNull(nullptr, "Context"), - cta::exception::Errnum); - - /* throwOnZero */ - errno = ENOENT; - ASSERT_NO_THROW(cta::exception::Errnum::throwOnZero(1, "Context")); - ASSERT_THROW(cta::exception::Errnum::throwOnZero(0, "Context"), - cta::exception::Errnum); + ASSERT_THROW(cta::exception::Errnum::throwOnNull(nullptr, "Context"), cta::exception::Errnum); } } diff --git a/common/threading/Daemon.cpp b/common/threading/Daemon.cpp index 4182133502..3973b96e96 100644 --- a/common/threading/Daemon.cpp +++ b/common/threading/Daemon.cpp @@ -69,7 +69,7 @@ void cta::server::Daemon::daemonizeIfNotRunInForegroundAndSetUserAndGroup(const { pid_t pid = 0; - cta::exception::Errnum::throwOnNegative(pid = fork(), + cta::exception::Errnum::throwOnMinusOne(pid = fork(), "Failed to daemonize: Failed to fork"); // If we got a good PID, then we can exit the parent process if (0 < pid) { @@ -85,7 +85,7 @@ void cta::server::Daemon::daemonizeIfNotRunInForegroundAndSetUserAndGroup(const umask(0177); // Run the daemon in a new session - cta::exception::Errnum::throwOnNegative(setsid(), + cta::exception::Errnum::throwOnMinusOne(setsid(), "Failed to daemonize: Failed to run daemon is a new session"); // Redirect standard files to /dev/null diff --git a/disk/DiskFileImplementations.hpp b/disk/DiskFileImplementations.hpp index d230397aff..d5d0f03fca 100644 --- a/disk/DiskFileImplementations.hpp +++ b/disk/DiskFileImplementations.hpp @@ -27,7 +27,6 @@ #include <radosstriper/libradosstriper.hpp> namespace cta::disk { - /** * Namespace managing the reading and writing of files to and from disk. */ @@ -60,15 +59,6 @@ namespace cta::disk { bool m_closeTried; }; - //============================================================================== - // CRYPTOPP SIGNER - //============================================================================== - struct CryptoPPSigner { - static std::string sign(const std::string msg, - const CryptoPP::RSA::PrivateKey & privateKey); - static cta::threading::Mutex s_mutex; - }; - //============================================================================== // XROOT FILES //============================================================================== @@ -248,5 +238,4 @@ namespace cta::disk { std::string m_truncatedDirectoryURL; // root://.../ part of the path is removed const uint16_t c_xrootTimeout = 15; }; - } // namespace cta::disk diff --git a/objectstore/BackendRados.cpp b/objectstore/BackendRados.cpp index 52441bb1d7..d3086938f1 100644 --- a/objectstore/BackendRados.cpp +++ b/objectstore/BackendRados.cpp @@ -90,22 +90,22 @@ BackendRados::BackendRados(log::Logger& logger, const std::string& userId, const m_logger(logger), m_user(userId), m_pool(pool), m_namespace(radosNameSpace) { log::LogContext lc(logger); - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this, &userId]() { return -m_cluster.init(userId.c_str()); }, "In BackendRados::BackendRados, failed to m_cluster.init"); try { RadosTimeoutLogger rtl; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this]() { return -m_cluster.conf_read_file(nullptr); }, "In BackendRados::BackendRados, failed to m_cluster.conf_read_file"); rtl.logIfNeeded("In BackendRados::BackendRados(): m_cluster.conf_read_file()", "no object"); rtl.reset(); - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this]() { return -m_cluster.conf_parse_env(nullptr);}, "In BackendRados::BackendRados, failed to m_cluster.conf_parse_env"); rtl.logIfNeeded("In BackendRados::BackendRados(): m_cluster.conf_parse_env()", "no object"); rtl.reset(); - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this]() { return -m_cluster.connect();}, "In BackendRados::BackendRados, failed to m_cluster.connect"); rtl.logIfNeeded("In BackendRados::BackendRados(): m_cluster.connect()", "no object"); @@ -116,7 +116,7 @@ BackendRados::BackendRados(log::Logger& logger, const std::string& userId, const params.add("contextId", i); lc.log(log::DEBUG, "BackendRados::BackendRados() about to create a new context"); rtl.reset(); - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this, &pool]() { return -m_cluster.ioctx_create(pool.c_str(), m_radosCtxPool.back()); }, "In BackendRados::BackendRados, failed to m_cluster.ioctx_create"); rtl.logIfNeeded("In BackendRados::BackendRados(): m_cluster.ioctx_create()", "no object"); @@ -130,7 +130,7 @@ BackendRados::BackendRados(log::Logger& logger, const std::string& userId, const lc.log(log::DEBUG, "BackendRados::BackendRados() namespace set. About to test access."); // Try to read a non-existing object through the newly created context, in hope this will protect against // race conditions(?) when creating the contexts in a tight loop. - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this, &bl, &rtl]() { auto rc = m_radosCtxPool.back().read("TestObjectThatDoesNotNeedToExist", bl, 1, 0); rtl.logIfNeeded("In BackendRados::BackendRados(): m_radosCtxPool.back().read()", "TestObjectThatDoesNotNeedToExist"); @@ -215,7 +215,7 @@ void BackendRados::create(const std::string& name, const std::string& content) { { RadosTimeoutLogger rtl; try { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this, &name, &wop]() { return -getRadosCtx().operate(name, &wop); }, @@ -261,7 +261,7 @@ void BackendRados::atomicOverwrite(const std::string& name, const std::string& c bl.append(content.c_str(), content.size()); wop.write_full(bl); RadosTimeoutLogger rtl; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this, &name, &wop]() { return -getRadosCtx().operate(name, &wop); }, std::string("In BackendRados::atomicOverwrite, failed to assert existence or write: ") + name); rtl.logIfNeeded("In BackendRados::atomicOverwrite(): m_radosCtx.operate(assert_exists+write_full)", name); @@ -272,7 +272,7 @@ std::string BackendRados::read(const std::string& name) { librados::bufferlist bl; RadosTimeoutLogger rtl; try { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this, &name, &bl]() { auto rc = getRadosCtx().read(name, bl, std::numeric_limits<int32_t>::max(), 0); return rc < 0 ? rc : 0; @@ -295,7 +295,7 @@ std::string BackendRados::read(const std::string& name) { void BackendRados::remove(const std::string& name) { RadosTimeoutLogger rtl; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this, &name]() { return -getRadosCtx().remove(name); }); rtl.logIfNeeded("In BackendRados::remove(): m_radosCtx.remove()", name); } @@ -305,7 +305,7 @@ bool BackendRados::exists(const std::string& name) { time_t date; RadosTimeoutLogger rtl; int statRet; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [this, &statRet, &name, &size, &date]() { statRet = getRadosCtx().stat(name, &size, &date); return 0; @@ -327,19 +327,19 @@ std::list<std::string> BackendRados::list() { std::list<std::string> ret; auto& ctx = getRadosCtx(); decltype(ctx.nobjects_begin()) o; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [&o, &ctx]() { o = ctx.nobjects_begin(); return 0; }, "In BackendRados::list(): failed to ctx.nobjects_begin()"); bool go; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [&go, &o, &ctx]() { go = (o != ctx.nobjects_end()); return 0; }, "In BackendRados::list(): failed ctx.nobjects_end()"); while(go) { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException( + throwOnReturnedErrnoOrThrownStdException( [&ret, &o, &go, &ctx]() { ret.push_back(o->get_oid()); o++; @@ -367,7 +367,7 @@ void BackendRados::ScopedLock::releaseNotify() { // we hence overlook the ENOENT errors. TIMESTAMPEDPRINT("Pre-release"); RadosTimeoutLogger rtl1; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([this, &rtl1]() { + throwOnReturnedErrnoOrThrownStdException([this, &rtl1]() { int rc=m_context.unlock(m_oid, "lock", m_clientId); rtl1.logIfNeeded("In BackendRados::ScopedLock::releaseNotify(): m_context.unlock()", m_oid); return rc==-ENOENT?0:-rc; @@ -381,7 +381,7 @@ void BackendRados::ScopedLock::releaseNotify() { // We use a fire and forget aio call. librados::AioCompletion * completion = librados::Rados::aio_create_completion(nullptr, nullptr, nullptr); RadosTimeoutLogger rtl2; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([this, &completion, &bl]() { return -m_context.aio_notify(m_oid, completion, bl, 10000, nullptr);}, + throwOnReturnedErrnoOrThrownStdException([this, &completion, &bl]() { return -m_context.aio_notify(m_oid, completion, bl, 10000, nullptr);}, "In BackendRados::ScopedLock::releaseNotify(): failed to aio_notify()"); rtl2.logIfNeeded("In BackendRados::ScopedLock::releaseNotify(): m_context.aio_notify()", m_oid); completion->release(); @@ -397,7 +397,7 @@ void BackendRados::ScopedLock::releaseBackoff() { TIMESTAMPEDPRINT("Pre-release"); RadosTimeoutLogger rtl1; int rc; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([this, &rc]() { + throwOnReturnedErrnoOrThrownStdException([this, &rc]() { rc=m_context.unlock(m_oid, "lock", m_clientId); return 0; }, "In BackendRados::ScopedLock::releaseBackoff(): failed m_context.unlock()"); @@ -437,7 +437,7 @@ BackendRados::LockWatcher::LockWatcher(librados::IoCtx& context, const std::stri m_internal->m_future = m_internal->m_promise.get_future(); TIMESTAMPEDPRINT("Pre-watch2"); RadosTimeoutLogger rtl; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([this, &name]() { return -m_context.watch2(name, &m_watchHandle, m_internal.get());}, + throwOnReturnedErrnoOrThrownStdException([this, &name]() { return -m_context.watch2(name, &m_watchHandle, m_internal.get());}, "In BackendRados::LockWatcher::LockWatcher(): failed m_context.watch2()"); rtl.logIfNeeded("In BackendRados::LockWatcher::LockWatcher(): m_context.watch2()", name); TIMESTAMPEDPRINT("Post-watch2"); @@ -472,7 +472,7 @@ BackendRados::LockWatcher::~LockWatcher() { librados::AioCompletion *completion = librados::Rados::aio_create_completion(m_internal.release(), nullptr, Internal::deleter); RadosTimeoutLogger rtl; try { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([this, &completion]() { + throwOnReturnedErrnoOrThrownStdException([this, &completion]() { m_context.aio_unwatch(m_watchHandle, completion); return 0; }, "In BackendRados::LockWatcher::~LockWatcher(): failed m_context.aio_unwatch()"); @@ -526,13 +526,13 @@ void BackendRados::lockNotify(const std::string& name, uint64_t timeout_us, Lock TIMESTAMPEDPRINT(lockType==LockType::Shared?"Pre-lock (shared)":"Pre-lock (exclusive)"); RadosTimeoutLogger rtl; if(lockType==LockType::Shared) { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &tv]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &tv]() { rc = radosCtx.lock_shared(name, "lock", clientId, "", "", &tv, 0); return 0; }, "In BackendRados::lockNotify: failed radosCtx.lock_shared()"); rtl.logIfNeeded("In BackendRados::lockNotify(): m_radosCtx.lock_shared()", name); } else { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &tv]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &tv]() { rc = radosCtx.lock_exclusive(name, "lock", clientId, "", &tv, 0); return 0; }, "In BackendRados::lockNotify: failed radosCtx.lock_exclusive()"); @@ -553,13 +553,13 @@ void BackendRados::lockNotify(const std::string& name, uint64_t timeout_us, Lock // We need to retry the lock after establishing the watch: it could have been released during that time. rtl.reset(); if (lockType==LockType::Shared) { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &tv]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &tv]() { rc = radosCtx.lock_shared(name, "lock", clientId, "", "", &tv, 0); return 0; }, "In BackendRados::lockNotify: failed radosCtx.lock_shared()(2)"); rtl.logIfNeeded("In BackendRados::lockNotify(): m_radosCtx.lock_shared()", name); } else { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &tv]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &tv]() { rc = radosCtx.lock_exclusive(name, "lock", clientId, "", &tv, 0); return 0; }, "In BackendRados::lockNotify: failed radosCtx.lock_shared()(2)"); @@ -595,12 +595,12 @@ void BackendRados::lockNotify(const std::string& name, uint64_t timeout_us, Lock // Get the size: uint64_t size; time_t date; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException ([&radosCtx, &name, &size, &date]() { return -radosCtx.stat(name, &size, &date); }, + throwOnReturnedErrnoOrThrownStdException ([&radosCtx, &name, &size, &date]() { return -radosCtx.stat(name, &size, &date); }, std::string("In BackendRados::lock, failed to librados::IoCtx::stat: ") + name + "/" + "lock" + "/" + clientId + "//"); if (!size) { // The object has a zero size: we probably created it by attempting the locking. - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException ([&radosCtx, &name]() { return -radosCtx.remove(name);}, + throwOnReturnedErrnoOrThrownStdException ([&radosCtx, &name]() { return -radosCtx.remove(name);}, std::string("In BackendRados::lock, failed to librados::IoCtx::remove: ") + name + "//"); throw cta::exception::NoSuchObject(std::string("In BackendRados::lockWatch(): " @@ -749,14 +749,14 @@ void BackendRados::lockBackoff(const std::string& name, uint64_t timeout_us, Loc TIMESTAMPEDPRINT(lockType==LockType::Shared?"Pre-lock (shared)":"Pre-lock (exclusive)"); RadosTimeoutLogger rtl; if (lockType==LockType::Shared) { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &radosLockExpirationTime, &timingMeasurements, &t, &timeoutTimer]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &radosLockExpirationTime, &timingMeasurements, &t, &timeoutTimer]() { rc = radosCtx.lock_shared(name, "lock", clientId, "", "", &radosLockExpirationTime, 0); timingMeasurements.addSuccess(t.secs(), timeoutTimer.secs()); return 0; }, "In BackendRados::lockBackoff(): failed radosCtx.lock_shared()"); rtl.logIfNeeded("In BackendRados::lockBackoff(): radosCtx.lock_shared()", name); } else { - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &radosLockExpirationTime, &timingMeasurements, &t, &timeoutTimer]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &radosCtx, &name, &clientId, &radosLockExpirationTime, &timingMeasurements, &t, &timeoutTimer]() { rc = radosCtx.lock_exclusive(name, "lock", clientId, "", &radosLockExpirationTime, 0); timingMeasurements.addSuccess(t.secs(), timeoutTimer.secs()); return 0; @@ -800,12 +800,12 @@ void BackendRados::lockBackoff(const std::string& name, uint64_t timeout_us, Loc // Get the size: uint64_t size; time_t date; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException ([&radosCtx, &name, &size, &date]() { return -radosCtx.stat(name, &size, &date); }, + throwOnReturnedErrnoOrThrownStdException ([&radosCtx, &name, &size, &date]() { return -radosCtx.stat(name, &size, &date); }, std::string("In BackendRados::lockBackoff, failed to librados::IoCtx::stat: ") + name + "/" + "lock" + "/" + clientId + "//"); if (!size) { // The object has a zero size: we probably created it by attempting the locking. - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException ([&radosCtx, &name]() { return -radosCtx.remove(name); }, + throwOnReturnedErrnoOrThrownStdException ([&radosCtx, &name]() { return -radosCtx.remove(name); }, std::string("In BackendRados::lockBackoff, failed to librados::IoCtx::remove: ") + name + "//"); throw cta::exception::NoSuchObject(std::string("In BackendRados::lockBackoff(): " @@ -871,7 +871,7 @@ m_backend(be), m_name(name), m_value(value), m_job(), m_jobFuture(m_job.get_futu m_radosTimeoutLogger.reset(); RadosTimeoutLogger rtl; int rc; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([this, &rc, &aioc, &wop]() { + throwOnReturnedErrnoOrThrownStdException([this, &rc, &aioc, &wop]() { rc=m_backend.getRadosCtx().aio_operate(m_name, aioc, &wop); return 0; }, "In BackendRados::AsyncCreator::AsyncCreator(): failed m_backend.getRadosCtx().aio_operate()"); @@ -903,7 +903,7 @@ void BackendRados::AsyncCreator::createExclusiveCallback(librados::completion_t RadosTimeoutLogger rtl; int rc; librados::AioCompletion * aioc = librados::Rados::aio_create_completion(pThis, statCallback, nullptr); - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &ac, &aioc]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &ac, &aioc]() { rc=ac.m_backend.getRadosCtx().aio_stat(ac.m_name, aioc, &ac.m_size, &ac.m_time); return 0; }, "In BackendRados::AsyncCreator::createExclusiveCallback(): failed m_backend.getRadosCtx().aio_operate()"); @@ -940,7 +940,7 @@ void BackendRados::AsyncCreator::statCallback(librados::completion_t completion, ac.m_radosTimeoutLogger.reset(); RadosTimeoutLogger rtl; int rc; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &ac, &aioc, &wop]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &ac, &aioc, &wop]() { rc=ac.m_backend.getRadosCtx().aio_operate(ac.m_name, aioc, &wop); return 0; }, "In BackendRados::AsyncCreator::statCallback(): failed m_backend.getRadosCtx().aio_operate()"); @@ -976,7 +976,7 @@ void BackendRados::AsyncCreator::statCallback(librados::completion_t completion, RadosTimeoutLogger rtl; int rc; librados::AioCompletion * aioc = librados::Rados::aio_create_completion(pThis, statCallback, nullptr); - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &ac, &aioc]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &ac, &aioc]() { rc=ac.m_backend.getRadosCtx().aio_stat(ac.m_name, aioc, &ac.m_size, &ac.m_time); return 0; }, "In BackendRados::AsyncCreator::statCallback(): failed m_backend.getRadosCtx().aio_operate()"); @@ -1019,7 +1019,7 @@ BackendRados::AsyncUpdater::AsyncUpdater(BackendRados& be, const std::string& na RadosTimeoutLogger rtl; m_radosTimeoutLogger.reset(); int rc; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([this, &rc, &aioc]() { + throwOnReturnedErrnoOrThrownStdException([this, &rc, &aioc]() { rc=m_backend.getRadosCtx().aio_read(m_name, aioc, &m_radosBufferList, std::numeric_limits<int32_t>::max(), 0); return 0; }, std::string("In AsyncUpdater::AsyncUpdater::lock_lambda(): failed getRadosCtx().aio_read(): ")+m_name); @@ -1136,7 +1136,7 @@ void BackendRados::AsyncUpdater::UpdateJob::execute() { RadosTimeoutLogger rtl; au.m_radosTimeoutLogger.reset(); int rc; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &au, &aioc]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &au, &aioc]() { rc=au.m_backend.getRadosCtx().aio_write_full(au.m_name, aioc, au.m_radosBufferList); return 0; }, "In BackendRados::AsyncUpdater::UpdateJob::execute(): failed m_backend.getRadosCtx().aio_write_full()"); @@ -1169,7 +1169,7 @@ void BackendRados::AsyncUpdater::commitCallback(librados::completion_t completio au.m_radosTimeoutLogger.reset(); RadosTimeoutLogger rtl; int rc; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &au, &aioc]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &au, &aioc]() { rc=au.m_backend.getRadosCtx().aio_unlock(au.m_name, "lock", au.m_lockClient, aioc); return 0; }, "In BackendRados::AsyncUpdater::commitCallback(): failed to m_backend.getRadosCtx().aio_unlock()"); @@ -1234,7 +1234,7 @@ BackendRados::AsyncDeleter::AsyncDeleter(BackendRados& be, const std::string& na m_radosTimeoutLogger.reset(); RadosTimeoutLogger rtl; int rc; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([this, &rc, &aioc]() { + throwOnReturnedErrnoOrThrownStdException([this, &rc, &aioc]() { rc=m_backend.getRadosCtx().aio_remove(m_name, aioc); return 0; }, "In BackendRados::AsyncDeleter::AsyncDeleter(): failed m_backend.getRadosCtx().aio_remove()"); @@ -1300,7 +1300,7 @@ void BackendRados::AsyncLockfreeFetcher::AioReadPoster::execute() { RadosTimeoutLogger rtl; au.m_radosTimeoutLogger.reset(); int rc; - cta::exception::Errnum::throwOnReturnedErrnoOrThrownStdException([&rc, &au, &aioc]() { + throwOnReturnedErrnoOrThrownStdException([&rc, &au, &aioc]() { rc=au.m_backend.getRadosCtx().aio_read(au.m_name, aioc, &au.m_radosBufferList, std::numeric_limits<int32_t>::max(), 0); return 0; }, "In BackendRados::AsyncLockfreeFetcher::AioReadPoster::execute(): failed m_backend.getRadosCtx().aio_read()"); diff --git a/objectstore/BackendRados.hpp b/objectstore/BackendRados.hpp index e171d710b1..71a7865bfe 100644 --- a/objectstore/BackendRados.hpp +++ b/objectstore/BackendRados.hpp @@ -112,6 +112,21 @@ public: static const size_t c_backoffFraction; static const uint64_t c_maxWait; + // This method was originally part of cta::exception::Errnum. As it is only used in the BackupRados class, + // it has been moved here. + template <typename F> + static void throwOnReturnedErrnoOrThrownStdException(F f, std::string_view context = "") { + try { + exception::Errnum::throwOnReturnedErrno(f(), context); + } catch(exception::Errnum&) { + throw; // Let the exception of throwOnReturnedErrno pass through + } catch(std::error_code& ec) { + throw exception::Errnum(ec.value(), std::string(context) + " Got a std::error_code: " + ec.message()); + } catch(std::exception& ex) { + throw exception::Exception(std::string(context) + " Got a std::exception: " + ex.what()); + } + } + private: static std::string createUniqueClientId(); /** This function will lock or die (actually throw, that is) */ diff --git a/objectstore/BackendTest.cpp b/objectstore/BackendTest.cpp index db47f803da..65312c8142 100644 --- a/objectstore/BackendTest.cpp +++ b/objectstore/BackendTest.cpp @@ -222,6 +222,13 @@ TEST_P(BackendAbstractTest, ParametersInterface) { //std::cout << params->toStr() << std::endl; } +TEST(BackendAbstractTest, Errnum_throwers) { + ASSERT_NO_THROW(cta::objectstore::BackendRados::throwOnReturnedErrnoOrThrownStdException([](){ return 0; }, "Context")); + ASSERT_THROW(cta::objectstore::BackendRados::throwOnReturnedErrnoOrThrownStdException([](){ return ENOSPC; }, "Context"), cta::exception::Errnum); + ASSERT_THROW(cta::objectstore::BackendRados::throwOnReturnedErrnoOrThrownStdException([](){ throw std::error_code(ENOSPC, std::system_category()); return 0; }, "Context"), cta::exception::Errnum); + ASSERT_THROW(cta::objectstore::BackendRados::throwOnReturnedErrnoOrThrownStdException([](){ throw std::exception(); return 0; }, "Context"), cta::exception::Exception); +} + static cta::objectstore::BackendVFS osVFS(__LINE__, __FILE__); #ifdef TEST_RADOS static cta::log::DummyLogger dl("", ""); -- GitLab From 1b70cc1ee5d34ca61b1f990d694ab830f273cc73 Mon Sep 17 00:00:00 2001 From: Michael Davis <michael.davis@cern.ch> Date: Wed, 24 Jan 2024 13:55:26 +0100 Subject: [PATCH 3/6] Simplifies Errnum constructors --- common/exception/Errnum.cpp | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/common/exception/Errnum.cpp b/common/exception/Errnum.cpp index cb94c0f18d..133d3e5730 100644 --- a/common/exception/Errnum.cpp +++ b/common/exception/Errnum.cpp @@ -20,25 +20,16 @@ namespace cta::exception { -Errnum::Errnum(std::string_view what) : Exception() { - m_errnum = errno; - ErrnumConstructorBottomHalf(what); +Errnum::Errnum(int err, std::string_view what) : Exception(), + m_errnum(err), + m_strerror(utils::errnoToString(m_errnum)) { + std::stringstream what2; + what2 << what << (what.empty() ? "" : " "); + what2 << "Errno=" << m_errnum << ": " << m_strerror; + getMessage() << what2.str(); } -Errnum::Errnum(int err, std::string_view what) : Exception() { - m_errnum = err; - ErrnumConstructorBottomHalf(what); -} - -void Errnum::ErrnumConstructorBottomHalf(std::string_view what) { - m_strerror = utils::errnoToString(m_errnum); - std::stringstream w2; - if(what.size()) { - w2 << what << " "; - } - w2 << "Errno=" << m_errnum << ": " << m_strerror; - getMessage() << w2.str(); -} +Errnum::Errnum(std::string_view what) : Errnum(errno, what) { } void Errnum::throwOnReturnedErrno(int err, std::string_view context) { if(err) throw Errnum(err, context); -- GitLab From b7af4f460760af7b8089510a0888b2038d69ebc9 Mon Sep 17 00:00:00 2001 From: Michael Davis <michael.davis@cern.ch> Date: Wed, 24 Jan 2024 14:08:34 +0100 Subject: [PATCH 4/6] A few small fixes --- common/exception/Errnum.cpp | 6 +++--- common/exception/Errnum.hpp | 3 --- common/exception/XrootCl.hpp | 2 +- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/common/exception/Errnum.cpp b/common/exception/Errnum.cpp index 133d3e5730..847190b37e 100644 --- a/common/exception/Errnum.cpp +++ b/common/exception/Errnum.cpp @@ -22,10 +22,10 @@ namespace cta::exception { Errnum::Errnum(int err, std::string_view what) : Exception(), m_errnum(err), - m_strerror(utils::errnoToString(m_errnum)) { + m_strerror(utils::errnoToString(err)) { std::stringstream what2; - what2 << what << (what.empty() ? "" : " "); - what2 << "Errno=" << m_errnum << ": " << m_strerror; + what2 << what << (what.empty() ? "" : " ") + << "Errno=" << m_errnum << ": " << m_strerror; getMessage() << what2.str(); } diff --git a/common/exception/Errnum.hpp b/common/exception/Errnum.hpp index 3b871b9d15..16bffe5b2a 100644 --- a/common/exception/Errnum.hpp +++ b/common/exception/Errnum.hpp @@ -28,7 +28,6 @@ public: virtual ~Errnum() = default; int errorNumber() const { return m_errnum; } - std::string strError() const { return m_strerror; } static void throwOnReturnedErrno(int err, std::string_view context = ""); static void throwOnNonZero(int status, std::string_view context = ""); @@ -36,8 +35,6 @@ public: static void throwOnMinusOne(ssize_t ret, const std::string_view context = ""); private: - void ErrnumConstructorBottomHalf(std::string_view what); - int m_errnum; std::string m_strerror; }; diff --git a/common/exception/XrootCl.hpp b/common/exception/XrootCl.hpp index 8376985c0e..02f7c8da47 100644 --- a/common/exception/XrootCl.hpp +++ b/common/exception/XrootCl.hpp @@ -18,7 +18,7 @@ #pragma once #include <xrootd/XrdCl/XrdClXRootDResponses.hh> -#include "Exception.hpp" +#include "common/exception/Exception.hpp" namespace cta::exception { -- GitLab From ad34dd5ec028df14fb638645ea42d6b614cd7455 Mon Sep 17 00:00:00 2001 From: Michael Davis <michael.davis@cern.ch> Date: Wed, 24 Jan 2024 14:19:43 +0100 Subject: [PATCH 5/6] Updates Release Notes --- ReleaseNotes.md | 1 + 1 file changed, 1 insertion(+) diff --git a/ReleaseNotes.md b/ReleaseNotes.md index 281c414236..0e8227921e 100644 --- a/ReleaseNotes.md +++ b/ReleaseNotes.md @@ -16,6 +16,7 @@ - cta/CTA#547 - Fix repack backpressure mechanism not requeueing repack requests - cta/CTA#562 - CTA Frontend should reject files with owner_uid=0 - cta/CTA#572 - Do not set the 'queueTrimRequired' flag as true when 'doCleanup' is required +- cta/CTA#584 - Refactors exception::Errnum class ### Continuous Integration - cta/CTA#278 - Updating system tests for new EOS evict command -- GitLab From 581c01a6457b05203cff51d32ec5fe25269145b8 Mon Sep 17 00:00:00 2001 From: Michael Davis <michael.davis@cern.ch> Date: Wed, 24 Jan 2024 14:21:55 +0100 Subject: [PATCH 6/6] Renames what2 --- common/exception/Errnum.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/exception/Errnum.cpp b/common/exception/Errnum.cpp index 847190b37e..89f105682a 100644 --- a/common/exception/Errnum.cpp +++ b/common/exception/Errnum.cpp @@ -23,10 +23,10 @@ namespace cta::exception { Errnum::Errnum(int err, std::string_view what) : Exception(), m_errnum(err), m_strerror(utils::errnoToString(err)) { - std::stringstream what2; - what2 << what << (what.empty() ? "" : " ") - << "Errno=" << m_errnum << ": " << m_strerror; - getMessage() << what2.str(); + std::stringstream whatWithErrnum; + whatWithErrnum << what << (what.empty() ? "" : " ") + << "Errno=" << m_errnum << ": " << m_strerror; + getMessage() << whatWithErrnum.str(); } Errnum::Errnum(std::string_view what) : Errnum(errno, what) { } -- GitLab