From: Eduard Bagdasaryan Date: Thu, 20 Feb 2025 13:35:33 +0000 (+0000) Subject: NoNewGlobals for iocb_table (#1998) X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=28a2ea0dedcbb43566c18e1d71b0f5bbff2c8640;p=thirdparty%2Fsquid.git NoNewGlobals for iocb_table (#1998) During shutdown, iocb_table global was deleted, but the corresponding cleanup code ignored some IoCallback members, triggering misleading Ipc::UdsSender memory leak reports from Valgrind. This change uses NoNewGlobals design to address the above problem, but this code needs a lot more refactoring to address other old problems associated with Comm initialization. --- diff --git a/src/comm.cc b/src/comm.cc index 6ff2a7d8c8..e27a8fd5cf 100644 --- a/src/comm.cc +++ b/src/comm.cc @@ -1152,9 +1152,6 @@ comm_init(void) { assert(fd_table); - // make sure the IO pending callback table exists - Comm::CallbackTableInit(); - /* XXX account fd_table */ /* Keep a few file descriptors free so that we don't run out of FD's * after accepting a client but before it opens a socket or a file. @@ -1172,8 +1169,6 @@ comm_exit(void) { delete TheHalfClosed; TheHalfClosed = nullptr; - - Comm::CallbackTableDestruct(); } #if USE_DELAY_POOLS diff --git a/src/comm/IoCallback.cc b/src/comm/IoCallback.cc index e9d6ba837c..5e62dd8f42 100644 --- a/src/comm/IoCallback.cc +++ b/src/comm/IoCallback.cc @@ -16,29 +16,34 @@ #include "fde.h" #include "globals.h" -Comm::CbEntry *Comm::iocb_table; +namespace Comm +{ -void -Comm::CallbackTableInit() +// XXX: Add API to react to Squid_MaxFD changes. +/// Creates a new callback table using the current value of Squid_MaxFD. +/// \sa fde::Init() +static CbEntry * +MakeCallbackTable() { // XXX: convert this to a std::map<> ? - iocb_table = static_cast(xcalloc(Squid_MaxFD, sizeof(CbEntry))); + // XXX: Stop bypassing CbEntry-associated constructors! Refactor to use new() instead. + const auto iocb_table = static_cast(xcalloc(Squid_MaxFD, sizeof(CbEntry))); for (int pos = 0; pos < Squid_MaxFD; ++pos) { iocb_table[pos].fd = pos; iocb_table[pos].readcb.type = IOCB_READ; iocb_table[pos].writecb.type = IOCB_WRITE; } + return iocb_table; } -void -Comm::CallbackTableDestruct() +} // namespace Comm + +Comm::CbEntry & +Comm::ioCallbacks(const int fd) { - // release any Comm::Connections being held. - for (int pos = 0; pos < Squid_MaxFD; ++pos) { - iocb_table[pos].readcb.conn = nullptr; - iocb_table[pos].writecb.conn = nullptr; - } - safe_free(iocb_table); + static const auto table = MakeCallbackTable(); + assert(fd < Squid_MaxFD); + return table[fd]; } /** diff --git a/src/comm/IoCallback.h b/src/comm/IoCallback.h index a4268fb15c..3ec152d41d 100644 --- a/src/comm/IoCallback.h +++ b/src/comm/IoCallback.h @@ -58,7 +58,7 @@ private: void reset(); }; -/// Entry nodes for the IO callback table: iocb_table +/// Entry nodes for the IO callback table. /// Keyed off the FD which the event applies to. class CbEntry { @@ -70,13 +70,10 @@ public: /// Table of scheduled IO events which have yet to be processed ?? /// Callbacks which might be scheduled in future are stored in fd_table. -extern CbEntry *iocb_table; +CbEntry &ioCallbacks(int fd); -void CallbackTableInit(); -void CallbackTableDestruct(); - -#define COMMIO_FD_READCB(fd) (&Comm::iocb_table[(fd)].readcb) -#define COMMIO_FD_WRITECB(fd) (&Comm::iocb_table[(fd)].writecb) +#define COMMIO_FD_READCB(fd) (&(Comm::ioCallbacks(fd).readcb)) +#define COMMIO_FD_WRITECB(fd) (&(Comm::ioCallbacks(fd).writecb)) } // namespace Comm diff --git a/src/comm/Write.cc b/src/comm/Write.cc index 2b9d8228e0..514947936b 100644 --- a/src/comm/Write.cc +++ b/src/comm/Write.cc @@ -49,7 +49,7 @@ Comm::Write(const Comm::ConnectionPointer &conn, const char *buf, int size, Asyn /** Write to FD. * This function is used by the lowest level of IO loop which only has access to FD numbers. - * We have to use the comm iocb_table to map FD numbers to waiting data and Comm::Connections. + * We have to use the Comm::ioCallbacks() to map FD numbers to waiting data and Comm::Connections. * Once the write has been concluded we schedule the waiting call with success/fail results. */ void diff --git a/src/tests/stub_libcomm.cc b/src/tests/stub_libcomm.cc index 6dd2bf9b61..fa41f03448 100644 --- a/src/tests/stub_libcomm.cc +++ b/src/tests/stub_libcomm.cc @@ -36,22 +36,19 @@ CBDATA_NAMESPACED_CLASS_INIT(Comm, ConnOpener); bool Comm::ConnOpener::doneAll() const STUB_RETVAL(false) void Comm::ConnOpener::start() STUB void Comm::ConnOpener::swanSong() STUB -Comm::ConnOpener::ConnOpener(const Comm::ConnectionPointer &, const AsyncCall::Pointer &, time_t) : AsyncJob("STUB Comm::ConnOpener") STUB - Comm::ConnOpener::~ConnOpener() STUB - void Comm::ConnOpener::setHost(const char *) STUB - const char * Comm::ConnOpener::getHost() const STUB_RETVAL(nullptr) +Comm::ConnOpener::ConnOpener(const Comm::ConnectionPointer &, const AsyncCall::Pointer &, time_t) : AsyncJob("STUB Comm::ConnOpener") {STUB} +Comm::ConnOpener::~ConnOpener() STUB +void Comm::ConnOpener::setHost(const char *) STUB +const char * Comm::ConnOpener::getHost() const STUB_RETVAL(nullptr) #include "comm/forward.h" - bool Comm::IsConnOpen(const Comm::ConnectionPointer &) STUB_RETVAL(false) +bool Comm::IsConnOpen(const Comm::ConnectionPointer &) STUB_RETVAL(false) #include "comm/IoCallback.h" - void Comm::IoCallback::setCallback(iocb_type, AsyncCall::Pointer &, char *, FREE *, int) STUB - void Comm::IoCallback::selectOrQueueWrite() STUB - void Comm::IoCallback::cancel(const char *) STUB - void Comm::IoCallback::finish(Comm::Flag, int) STUB - Comm::CbEntry *Comm::iocb_table = nullptr; -void Comm::CallbackTableInit() STUB -void Comm::CallbackTableDestruct() STUB +void Comm::IoCallback::setCallback(iocb_type, AsyncCall::Pointer &, char *, FREE *, int) STUB +void Comm::IoCallback::selectOrQueueWrite() STUB +void Comm::IoCallback::cancel(const char *) STUB +void Comm::IoCallback::finish(Comm::Flag, int) STUB #include "comm/Loops.h" void Comm::SelectLoopInit(void) STUB