From: Francesco Chemolli <5175948+kinkie@users.noreply.github.com> Date: Tue, 25 Jun 2024 18:18:12 +0000 (+0000) Subject: Scaffolding for YAML-formatted cache manager reports (#1784) X-Git-Tag: SQUID_7_0_1~101 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=99572608ffe1d6567abd876d27ceeb51174a2237;p=thirdparty%2Fsquid.git Scaffolding for YAML-formatted cache manager reports (#1784) The only YAML-compliant report currently using new code is mgr:pconn. Non-YAML non-aggregated reports in non-SMP instances are now framed using "by kid0 {...}" wrappers (for `squid -N`) or "by kid1 {...}" wrappers (for instances having one worker process and no rock diskers). This change makes YAML and non-YAML report framing more similar, but may affect existing report parsing automation. Also fixed Content-Type value computation for SMP reports: They were missing ";charset=utf-8" suffix. Broken since 2014 commit 8088f8d0, possibly due to code duplication created by 2010 commit 8822ebee. Moved FunActionCreator and ClassActionCreator classes into src/mgr/Registration.cc because no other code needs to know about them. This is especially valuable for function-based actions because they do not need to know about Mgr::FunAction (or even Mgr::Action). --- diff --git a/src/CacheManager.h b/src/CacheManager.h index 81bccb06dd..e3b085dbcc 100644 --- a/src/CacheManager.h +++ b/src/CacheManager.h @@ -37,12 +37,9 @@ public: /// initial URL path characters that identify cache manager requests static const SBuf &WellKnownUrlPathPrefix(); - void registerProfile(char const * action, char const * desc, - OBJH * handler, - int pw_req_flag, int atomic); - void registerProfile(char const * action, char const * desc, - Mgr::ClassActionCreationHandler *handler, - int pw_req_flag, int atomic); + /// remembers the given profile while ignoring attempts to register a same-name duplicate + void registerProfile(const Mgr::ActionProfilePointer &); + Mgr::ActionProfilePointer findAction(char const * action) const; Mgr::Action::Pointer createNamedAction(const char *actionName); Mgr::Action::Pointer createRequestedAction(const Mgr::ActionParams &); @@ -66,8 +63,6 @@ protected: int CheckPassword(const Mgr::Command &cmd); char *PasswdGet(Mgr::ActionPasswordList *, const char *); - void registerProfile(const Mgr::ActionProfilePointer &profile); - Menu menu_; }; diff --git a/src/Makefile.am b/src/Makefile.am index 1ce047eed4..3dca80df76 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2564,7 +2564,6 @@ tests_testCacheManager_SOURCES = \ tests/stub_libdiskio.cc \ tests/stub_liberror.cc \ tests/stub_libsecurity.cc \ - tests/stub_libstore.cc \ tests/stub_main_cc.cc \ mem_node.cc \ mime.cc \ diff --git a/src/cache_manager.cc b/src/cache_manager.cc index 08445a517a..8c6eb59873 100644 --- a/src/cache_manager.cc +++ b/src/cache_manager.cc @@ -44,24 +44,6 @@ /// \ingroup CacheManagerInternal #define MGR_PASSWD_SZ 128 -/// creates Action using supplied Action::Create method and command -class ClassActionCreator: public Mgr::ActionCreator -{ -public: - typedef Mgr::Action::Pointer Handler(const Mgr::Command::Pointer &cmd); - -public: - ClassActionCreator(Handler *aHandler): handler(aHandler) {} - - Mgr::Action::Pointer create(const Mgr::Command::Pointer &cmd) const override { - return handler(cmd); - } - -private: - Handler *handler; -}; - -/// Registers new profiles, ignoring attempts to register a duplicate void CacheManager::registerProfile(const Mgr::ActionProfile::Pointer &profile) { @@ -74,37 +56,6 @@ CacheManager::registerProfile(const Mgr::ActionProfile::Pointer &profile) } } -/** - \ingroup CacheManagerAPI - * Registers a C-style action, which is implemented as a pointer to a function - * taking as argument a pointer to a StoreEntry and returning void. - * Implemented via CacheManagerActionLegacy. - */ -void -CacheManager::registerProfile(char const * action, char const * desc, OBJH * handler, int pw_req_flag, int atomic) -{ - debugs(16, 3, "registering legacy " << action); - const Mgr::ActionProfile::Pointer profile = new Mgr::ActionProfile(action, - desc, pw_req_flag, atomic, new Mgr::FunActionCreator(handler)); - registerProfile(profile); -} - -/** - * \ingroup CacheManagerAPI - * Registers a C++-style action, via a pointer to a subclass of - * a CacheManagerAction object, whose run() method will be invoked when - * CacheManager identifies that the user has requested the action. - */ -void -CacheManager::registerProfile(char const * action, char const * desc, - ClassActionCreator::Handler *handler, - int pw_req_flag, int atomic) -{ - const Mgr::ActionProfile::Pointer profile = new Mgr::ActionProfile(action, - desc, pw_req_flag, atomic, new ClassActionCreator(handler)); - registerProfile(profile); -} - /** \ingroup CacheManagerInternal * Locates an action in the actions registry ActionsList. diff --git a/src/mgr/Action.cc b/src/mgr/Action.cc index 2c8bf63bf5..8fae91b0e9 100644 --- a/src/mgr/Action.cc +++ b/src/mgr/Action.cc @@ -45,12 +45,31 @@ Mgr::Action::atomic() const return command().profile->isAtomic; } +Mgr::Format +Mgr::Action::format() const +{ + return command().profile->format; +} + const char* Mgr::Action::name() const { return command().profile->name; } +const char * +Mgr::Action::contentType() const +{ + switch (format()) { + case Format::yaml: + return "application/yaml;charset=utf-8"; + case Format::informal: + return "text/plain;charset=utf-8"; + } + assert(!"unreachable code"); + return ""; +} + StoreEntry* Mgr::Action::createStoreEntry() const { @@ -120,3 +139,26 @@ Mgr::Action::fillEntry(StoreEntry* entry, bool writeHttpHeader) entry->complete(); } +void +Mgr::OpenKidSection(StoreEntry * const entry, const Format format) +{ + switch (format) { + case Format::yaml: + return storeAppendPrintf(entry, "---\nkid: %d\n", KidIdentifier); + case Format::informal: + return storeAppendPrintf(entry, "by kid%d {\n", KidIdentifier); + } + // unreachable code +} + +void +Mgr::CloseKidSection(StoreEntry * const entry, const Format format) +{ + switch (format) { + case Format::yaml: + return storeAppendPrintf(entry, "...\n"); + case Format::informal: + return storeAppendPrintf(entry, "} by kid%d\n\n", KidIdentifier); + } + // unreachable code +} diff --git a/src/mgr/Action.h b/src/mgr/Action.h index bf09b0e151..92b69ff826 100644 --- a/src/mgr/Action.h +++ b/src/mgr/Action.h @@ -12,6 +12,7 @@ #define SQUID_SRC_MGR_ACTION_H #include "ipc/forward.h" +#include "mgr/ActionFeatures.h" #include "mgr/forward.h" class StoreEntry; @@ -63,14 +64,17 @@ public: /// combined data should be written at the end of the coordinated response virtual bool aggregatable() const { return true; } // most kid classes are + /// action report syntax + virtual Format format() const; + bool atomic() const; ///< dump() call writes everything before returning const char *name() const; ///< label as seen in the cache manager menu const Command &command() const; ///< the cause of this action StoreEntry *createStoreEntry() const; ///< creates store entry from params - ///< Content-Type: header value for this report - virtual const char *contentType() const {return "text/plain;charset=utf-8";} + /// HTTP Content-Type header value for this Action report + const char *contentType() const; protected: /// calculate and keep local action-specific information @@ -90,6 +94,14 @@ private: Action &operator= (const Action &); // not implemented }; +/// starts writing a portion of the report specific to the current process +/// \sa CloseKidSection() +void OpenKidSection(StoreEntry *, Format); + +/// finishes writing a portion of the report specific to the current process +/// \sa OpenKidSection() +void CloseKidSection(StoreEntry *, Format); + } // namespace Mgr #endif /* SQUID_SRC_MGR_ACTION_H */ diff --git a/src/mgr/ActionFeatures.h b/src/mgr/ActionFeatures.h new file mode 100644 index 0000000000..f70035da72 --- /dev/null +++ b/src/mgr/ActionFeatures.h @@ -0,0 +1,35 @@ +/* + * Copyright (C) 1996-2024 The Squid Software Foundation and contributors + * + * Squid software is distributed under GPLv2+ license and includes + * contributions from numerous individuals and organizations. + * Please see the COPYING and CONTRIBUTORS files for details. + */ + +#ifndef SQUID_SRC_MGR_ACTIONFEATURES_H +#define SQUID_SRC_MGR_ACTIONFEATURES_H + +namespace Mgr +{ + +// Scoped enumeration declarations below solve two problems with ActionProfile +// constructor, RegisterAction(), and related function calls, making argument +// lists readable and safe: +// 1. They eliminate dangerous guessing of f(..., 0, 1, false) meaning by +// converting each anonymous constant into a named one (e.g., Atomic::no). +// 2. They prevent accidental argument reordering by prohibiting implicit value +// casts (e.g., both f(1, false) and f(false, 1) would otherwise compile). + +/// whether default cachemgr_passwd configuration denies the Action +enum class Protected { no, yes }; + +/// whether Action::dump() writes the entire report before returning +enum class Atomic { no, yes }; + +/// whether Action report uses valid YAML or unspecified/legacy formatting +enum class Format { informal, yaml }; + +} // namespace Mgr + +#endif /* SQUID_SRC_MGR_ACTIONFEATURES_H */ + diff --git a/src/mgr/ActionProfile.h b/src/mgr/ActionProfile.h index 349fba6e94..802696f5d5 100644 --- a/src/mgr/ActionProfile.h +++ b/src/mgr/ActionProfile.h @@ -12,6 +12,7 @@ #define SQUID_SRC_MGR_ACTIONPROFILE_H #include "mgr/ActionCreator.h" +#include "mgr/ActionFeatures.h" #include "mgr/forward.h" namespace Mgr @@ -24,9 +25,15 @@ public: typedef RefCount Pointer; public: - ActionProfile(const char* aName, const char* aDesc, bool aPwReq, - bool anAtomic, const ActionCreatorPointer &aCreator): - name(aName), desc(aDesc), isPwReq(aPwReq), isAtomic(anAtomic), + ActionProfile(const char* aName, const char* aDesc, + ActionCreatorPointer aCreator, + const Protected aProtected, + const Atomic anAtomic, + const Format aFormat): + name(aName), desc(aDesc), + isPwReq(aProtected == Protected::yes), + isAtomic(anAtomic == Atomic::yes), + format(aFormat), creator(aCreator) { } @@ -35,6 +42,7 @@ public: const char *desc; ///< action description to build an action menu list bool isPwReq; ///< whether password is required to perform the action bool isAtomic; ///< whether action dumps everything in one dump() call + Format format; ///< action report syntax ActionCreatorPointer creator; ///< creates Action objects with this profile }; diff --git a/src/mgr/FunAction.cc b/src/mgr/FunAction.cc index 84aeb76a31..a77fc3b64a 100644 --- a/src/mgr/FunAction.cc +++ b/src/mgr/FunAction.cc @@ -49,10 +49,12 @@ Mgr::FunAction::dump(StoreEntry* entry) { debugs(16, 5, MYNAME); Must(entry != nullptr); - if (UsingSmp()) - storeAppendPrintf(entry, "by kid%d {\n", KidIdentifier); + + OpenKidSection(entry, format()); + handler(entry); - if (atomic() && UsingSmp()) - storeAppendPrintf(entry, "} by kid%d\n\n", KidIdentifier); -} + if (atomic()) + CloseKidSection(entry, format()); + // non-atomic() actions must call CloseKidSection() when they are done +} diff --git a/src/mgr/FunAction.h b/src/mgr/FunAction.h index 1ea9aa7d9a..4844863a48 100644 --- a/src/mgr/FunAction.h +++ b/src/mgr/FunAction.h @@ -40,21 +40,6 @@ private: OBJH *handler; ///< legacy function that collects and dumps info }; -/// creates FunAction using ActionCreator API -class FunActionCreator: public ActionCreator -{ -public: - explicit FunActionCreator(OBJH *aHandler): handler(aHandler) {} - - /* ActionCreator API */ - Action::Pointer create(const CommandPointer &cmd) const override { - return FunAction::Create(cmd, handler); - } - -private: - OBJH *handler; ///< legacy function to pass to the FunAction wrapper -}; - } // namespace Mgr #endif /* SQUID_SRC_MGR_FUNACTION_H */ diff --git a/src/mgr/InfoAction.cc b/src/mgr/InfoAction.cc index 5baf4557a2..56ff7f6a87 100644 --- a/src/mgr/InfoAction.cc +++ b/src/mgr/InfoAction.cc @@ -143,11 +143,10 @@ Mgr::InfoAction::dump(StoreEntry* entry) Must(entry != nullptr); #if XMALLOC_STATISTICS - if (UsingSmp()) - storeAppendPrintf(entry, "by kid%d {\n", KidIdentifier); + // TODO: Move these stats into a new dedicated report. + Mgr::OpenKidSection(entry, Mgr::Format::informal); DumpMallocStatistics(entry); - if (UsingSmp()) - storeAppendPrintf(entry, "} by kid%d\n\n", KidIdentifier); + Mgr::CloseKidSection(entry, Mgr::Format::informal); #endif if (IamPrimaryProcess()) DumpInfo(data, entry); diff --git a/src/mgr/Inquirer.cc b/src/mgr/Inquirer.cc index 6db57a9fa7..ab2892e758 100644 --- a/src/mgr/Inquirer.cc +++ b/src/mgr/Inquirer.cc @@ -88,7 +88,7 @@ Mgr::Inquirer::start() replyBuf.reset(reply->pack()); } else { std::unique_ptr reply(new HttpReply); - reply->setHeaders(Http::scOkay, nullptr, "text/plain", -1, squid_curtime, squid_curtime); + reply->setHeaders(Http::scOkay, nullptr, aggrAction->contentType(), -1, squid_curtime, squid_curtime); CacheManager::PutCommonResponseHeaders(*reply, originOrNil); reply->header.putStr(Http::HdrType::CONNECTION, "close"); // until we chunk response replyBuf.reset(reply->pack()); diff --git a/src/mgr/Makefile.am b/src/mgr/Makefile.am index 0771ce3699..4da7ac6103 100644 --- a/src/mgr/Makefile.am +++ b/src/mgr/Makefile.am @@ -13,6 +13,7 @@ libmgr_la_SOURCES = \ Action.cc \ Action.h \ ActionCreator.h \ + ActionFeatures.h \ ActionParams.cc \ ActionParams.h \ ActionPasswordList.cc \ diff --git a/src/mgr/Registration.cc b/src/mgr/Registration.cc index c59b06b0a9..fb12506efd 100644 --- a/src/mgr/Registration.cc +++ b/src/mgr/Registration.cc @@ -10,23 +10,71 @@ #include "squid.h" #include "CacheManager.h" +#include "mgr/FunAction.h" #include "mgr/Registration.h" +namespace Mgr { + +/// creates FunAction using ActionCreator API +class FunActionCreator: public ActionCreator +{ +public: + explicit FunActionCreator(OBJH * const aHandler): handler(aHandler) {} + + /* ActionCreator API */ + Action::Pointer create(const CommandPointer &cmd) const override { + return FunAction::Create(cmd, handler); + } + +private: + OBJH *handler; ///< legacy function to pass to the FunAction wrapper +}; + +/// creates Action using supplied Action::Create method and command +class ClassActionCreator: public ActionCreator +{ +public: + using Handler = ClassActionCreationHandler; + +public: + ClassActionCreator(Handler * const aHandler): handler(aHandler) {} + + /* ActionCreator API */ + Action::Pointer create(const Command::Pointer &cmd) const override { + return handler(cmd); + } + +private: + Handler *handler; ///< configured Action object creator +}; + +} // namespace Mgr + void Mgr::RegisterAction(char const * action, char const * desc, OBJH * handler, - int pw_req_flag, int atomic) + const Protected protection, + const Atomic atomicity, + const Format format) { - CacheManager::GetInstance()->registerProfile(action, desc, handler, - pw_req_flag, atomic); + debugs(16, 3, "function-based " << action); + const auto profile = ActionProfile::Pointer::Make(action, + desc, new FunActionCreator(handler), + protection, atomicity, format); + CacheManager::GetInstance()->registerProfile(profile); } void Mgr::RegisterAction(char const * action, char const * desc, ClassActionCreationHandler *handler, - int pw_req_flag, int atomic) + const Protected protection, + const Atomic atomicity, + const Format format) { - CacheManager::GetInstance()->registerProfile(action, desc, handler, - pw_req_flag, atomic); + debugs(16, 3, "class-based " << action); + const auto profile = ActionProfile::Pointer::Make(action, + desc, new ClassActionCreator(handler), + protection, atomicity, format); + CacheManager::GetInstance()->registerProfile(profile); } diff --git a/src/mgr/Registration.h b/src/mgr/Registration.h index 027c21d309..40c97aabbd 100644 --- a/src/mgr/Registration.h +++ b/src/mgr/Registration.h @@ -11,18 +11,47 @@ #ifndef SQUID_SRC_MGR_REGISTRATION_H #define SQUID_SRC_MGR_REGISTRATION_H +#include "mgr/ActionFeatures.h" #include "mgr/forward.h" namespace Mgr { +/// Creates a function-based action profile and adds it to the cache manager +/// collection (once across all calls with the same action name). void RegisterAction(char const * action, char const * desc, OBJH * handler, - int pw_req_flag, int atomic); + Protected, Atomic, Format); +/// wrapper for legacy Format-unaware function-based action registration code +inline void +RegisterAction(const char * const action, const char * const desc, + OBJH * handler, + int pw_req_flag, int atomic) +{ + return RegisterAction(action, desc, handler, + (pw_req_flag ? Protected::yes : Protected::no), + (atomic ? Atomic::yes : Atomic::no), + Format::informal); +} + +/// Creates a class-based action profile and adds it to the cache manager +/// collection (once across all calls with the same action name). void RegisterAction(char const * action, char const * desc, ClassActionCreationHandler *handler, - int pw_req_flag, int atomic); + Protected, Atomic, Format); + +/// wrapper for legacy Format-unaware class-based action registration code +inline void +RegisterAction(const char * const action, const char * const desc, + ClassActionCreationHandler *handler, + int pw_req_flag, int atomic) +{ + return RegisterAction(action, desc, handler, + (pw_req_flag ? Protected::yes : Protected::no), + (atomic ? Atomic::yes : Atomic::no), + Format::informal); +} } // namespace Mgr diff --git a/src/pconn.cc b/src/pconn.cc index 3c1e951116..45adde1a3d 100644 --- a/src/pconn.cc +++ b/src/pconn.cc @@ -575,7 +575,8 @@ PconnModule::registerWithCacheManager(void) { Mgr::RegisterAction("pconn", "Persistent Connection Utilization Histograms", - DumpWrapper, 0, 1); + DumpWrapper, Mgr::Protected::no, Mgr::Atomic::yes, + Mgr::Format::yaml); } void diff --git a/src/stat.cc b/src/stat.cc index 2e8e6a451c..a49efeeada 100644 --- a/src/stat.cc +++ b/src/stat.cc @@ -327,8 +327,7 @@ statObjects(void *data) StoreEntry *e; if (state->theSearch->isDone()) { - if (UsingSmp()) - storeAppendPrintf(state->sentry, "} by kid%d\n\n", KidIdentifier); + Mgr::CloseKidSection(state->sentry, Mgr::Format::informal); state->sentry->complete(); state->sentry->unlock("statObjects+isDone"); delete state; diff --git a/src/tests/stub_cache_manager.cc b/src/tests/stub_cache_manager.cc index 6acb56f09e..04dbf7f35b 100644 --- a/src/tests/stub_cache_manager.cc +++ b/src/tests/stub_cache_manager.cc @@ -22,8 +22,8 @@ void CacheManager::start(const Comm::ConnectionPointer &, HttpRequest *, StoreEn } static CacheManager* instance = nullptr; CacheManager* CacheManager::GetInstance() STUB_RETVAL(instance) -void Mgr::RegisterAction(char const*, char const*, OBJH, int, int) {} -void Mgr::RegisterAction(char const *, char const *, Mgr::ClassActionCreationHandler *, int, int) {} +void Mgr::RegisterAction(char const *, char const *, OBJH *, Protected, Atomic, Format) {} +void Mgr::RegisterAction(char const *, char const *, ClassActionCreationHandler *, Protected, Atomic, Format) {} Mgr::Action::Pointer CacheManager::createRequestedAction(const Mgr::ActionParams &) STUB_RETVAL(nullptr) void CacheManager::PutCommonResponseHeaders(HttpReply &, const char *) STUB diff --git a/src/tests/stub_libmgr.cc b/src/tests/stub_libmgr.cc index 8177995bd7..65af4fe6bf 100644 --- a/src/tests/stub_libmgr.cc +++ b/src/tests/stub_libmgr.cc @@ -29,6 +29,7 @@ void Mgr::Action::fillEntry(StoreEntry *, bool) STUB void Mgr::Action::add(const Action &) STUB void Mgr::Action::respond(const Request &) STUB void Mgr::Action::sendResponse(const Ipc::RequestId) STUB +Mgr::Format Mgr::Action::format() const STUB_RETVAL(Mgr::Format::informal) bool Mgr::Action::atomic() const STUB_RETVAL(false) const char * Mgr::Action::name() const STUB_RETVAL(nullptr) static Mgr::Command static_Command; @@ -181,8 +182,8 @@ void Mgr::QueryParams::Parse(Parser::Tokenizer &, QueryParams &) STUB Mgr::QueryParam::Pointer Mgr::QueryParams::CreateParam(QueryParam::Type) STUB_RETVAL(Mgr::QueryParam::Pointer(nullptr)) #include "mgr/Registration.h" -//void Mgr::RegisterAction(char const *, char const *, OBJH *, int, int); -//void Mgr::RegisterAction(char const *, char const *, ClassActionCreationHandler *, int, int); +//void Mgr::RegisterAction(char const *, char const *, OBJH *, Protected, Atomic, Format); +//void Mgr::RegisterAction(char const *, char const *, ClassActionCreationHandler *, Protected, Atomic, Format); #include "mgr/Request.h" //Mgr::Request::Request(int, unsigned int, int, const Mgr::ActionParams &) STUB diff --git a/src/tests/testCacheManager.cc b/src/tests/testCacheManager.cc index 554ecb7f61..84f8a51eae 100644 --- a/src/tests/testCacheManager.cc +++ b/src/tests/testCacheManager.cc @@ -11,6 +11,7 @@ #include "CacheManager.h" #include "compat/cppunit.h" #include "mgr/Action.h" +#include "mgr/Registration.h" #include "Store.h" #include "unitTestMain.h" @@ -99,7 +100,7 @@ TestCacheManager::testRegister() CacheManager *manager=CacheManager::GetInstance(); CPPUNIT_ASSERT(manager != nullptr); - manager->registerProfile("sample", "my sample", &dummy_action, false, false); + Mgr::RegisterAction("sample", "my sample", &dummy_action, Mgr::Protected::no, Mgr::Atomic::no, Mgr::Format::informal); Mgr::Action::Pointer action = manager->createNamedAction("sample"); CPPUNIT_ASSERT(action != nullptr); @@ -108,9 +109,12 @@ TestCacheManager::testRegister() CPPUNIT_ASSERT(profile->creator != nullptr); CPPUNIT_ASSERT_EQUAL(false, profile->isPwReq); CPPUNIT_ASSERT_EQUAL(false, profile->isAtomic); + CPPUNIT_ASSERT_EQUAL(Mgr::Format::informal, profile->format); + CPPUNIT_ASSERT_EQUAL(Mgr::Format::informal, action->format()); CPPUNIT_ASSERT_EQUAL(String("sample"), String(action->name())); StoreEntry *sentry=new StoreEntry(); + sentry->createMemObject(); sentry->flags=0x25; //arbitrary test value action->run(sentry, false); CPPUNIT_ASSERT_EQUAL(1,(int)sentry->flags);