From: Amos Jeffries Date: Sat, 17 Oct 2020 05:33:11 +0000 (+0000) Subject: Cleanup helper.h classes (#719) X-Git-Tag: 4.15-20210522-snapshot~55 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=017a2d1;p=thirdparty%2Fsquid.git Cleanup helper.h classes (#719) Polish up the classes in helper.h to use proper constructors as the caller API for creation + initialization. Use C++11 initialization for default values. * no "virtual" in helper class destructor declaration could create memory leaks in future (poorly) refactored code; the gained protection is probably worth adding the (currently unused) virtual table to the helper class; resolves Coverity issue CID 1461141 (VIRTUAL_DTOR) * missing Comm::Connection timers initialization on helper startup * multiple initialization of values used for state accounting * initialization of booleans to non-0 integer values * initialization of char using numeric values * missing mgr:filedescriptors description for helper sockets --- diff --git a/src/helper.cc b/src/helper.cc index 0a52614db4..4bb301c113 100644 --- a/src/helper.cc +++ b/src/helper.cc @@ -59,16 +59,6 @@ CBDATA_CLASS_INIT(helper_stateful_server); InstanceIdDefinitions(HelperServerBase, "Hlpr"); -void -HelperServerBase::initStats() -{ - stats.uses=0; - stats.replies=0; - stats.pending=0; - stats.releases=0; - stats.timedout = 0; -} - void HelperServerBase::closePipesSafely(const char *id_name) { @@ -136,6 +126,23 @@ HelperServerBase::dropQueued() } } +HelperServerBase::HelperServerBase(const char *desc, Ip::Address &ip, int aPid, void *aIpc, int rfd, int wfd) : + pid(aPid), + addr(ip), + readPipe(new Comm::Connection), + writePipe(new Comm::Connection), + hIpc(aIpc), + rbuf(static_cast(memAllocBuf(ReadBufSize, &rbuf_sz))) +{ + readPipe->fd = rfd; + readPipe->noteStart(); + fd_note(readPipe->fd, desc); + + writePipe->fd = wfd; + writePipe->noteStart(); + fd_note(writePipe->fd, desc); +} + HelperServerBase::~HelperServerBase() { if (rbuf) { @@ -144,6 +151,12 @@ HelperServerBase::~HelperServerBase() } } +helper_server::helper_server(helper *hlp, int aPid, void *aIpc, int rfd, int wfd) : + HelperServerBase(hlp->cmdline->key, hlp->addr, aPid, aIpc, rfd, wfd), + parent(cbdataReference(hlp)) +{ +} + helper_server::~helper_server() { wqueue->clean(); @@ -174,6 +187,12 @@ helper_server::dropQueued() requestsIndex.clear(); } +helper_stateful_server::helper_stateful_server(statefulhelper *hlp, int aPid, void *aIpc, int rfd, int wfd) : + HelperServerBase(hlp->cmdline->key, hlp->addr, aPid, aIpc, rfd, wfd), + parent(cbdataReference(hlp)) +{ +} + helper_stateful_server::~helper_stateful_server() { /* TODO: walk the local queue of requests and carry them all out */ @@ -201,7 +220,6 @@ helperOpenServers(helper * hlp) char *procname; const char *args[HELPER_MAX_ARGS+1]; // save space for a NULL terminator char fd_note_buf[FD_DESC_SZ]; - helper_server *srv; int nargs = 0; int k; pid_t pid; @@ -265,22 +283,7 @@ helperOpenServers(helper * hlp) ++ hlp->childs.n_running; ++ hlp->childs.n_active; - srv = new helper_server; - srv->hIpc = hIpc; - srv->pid = pid; - srv->initStats(); - srv->addr = hlp->addr; - srv->readPipe = new Comm::Connection; - srv->readPipe->fd = rfd; - srv->writePipe = new Comm::Connection; - srv->writePipe->fd = wfd; - srv->rbuf = (char *)memAllocBuf(ReadBufSize, &srv->rbuf_sz); - srv->wqueue = new MemBuf; - srv->roffset = 0; - srv->nextRequestId = 0; - srv->replyXaction = NULL; - srv->ignoreToEom = false; - srv->parent = cbdataReference(hlp); + auto *srv = new helper_server(hlp, pid, hIpc, rfd, wfd); dlinkAddTail(srv, &srv->link, &hlp->servers); if (rfd == wfd) { @@ -392,20 +395,7 @@ helperStatefulOpenServers(statefulhelper * hlp) ++ hlp->childs.n_running; ++ hlp->childs.n_active; - helper_stateful_server *srv = new helper_stateful_server; - srv->hIpc = hIpc; - srv->pid = pid; - srv->initStats(); - srv->addr = hlp->addr; - srv->readPipe = new Comm::Connection; - srv->readPipe->fd = rfd; - srv->writePipe = new Comm::Connection; - srv->writePipe->fd = wfd; - srv->rbuf = (char *)memAllocBuf(ReadBufSize, &srv->rbuf_sz); - srv->roffset = 0; - srv->parent = cbdataReference(hlp); - srv->reservationStart = 0; - + auto *srv = new helper_stateful_server(hlp, pid, hIpc, rfd, wfd); dlinkAddTail(srv, &srv->link, &hlp->servers); if (rfd == wfd) { diff --git a/src/helper.h b/src/helper.h index e517b44df8..a1f72d8ad6 100644 --- a/src/helper.h +++ b/src/helper.h @@ -29,6 +29,9 @@ #include #include +class CommTimeoutCbParams; +class HelperServerBase; +class MemBuf; class Packable; class wordlist; @@ -44,7 +47,6 @@ public: }; } -class HelperServerBase; /** * Managers a set of individual helper processes with a common queue of requests. * @@ -65,21 +67,8 @@ class helper CBDATA_CLASS(helper); public: - inline helper(const char *name) : - cmdline(NULL), - id_name(name), - ipc_type(0), - droppedRequests(0), - overloadStart(0), - last_queue_warn(0), - last_restart(0), - timeout(0), - retryTimedOut(false), - retryBrokenHelper(false), - eom('\n') { - memset(&stats, 0, sizeof(stats)); - } - ~helper(); + explicit helper(const char *name) : id_name(name) {} + virtual ~helper(); /// \returns next request in the queue, or nil. Helper::Xaction *nextRequest(); @@ -103,29 +92,29 @@ public: void handleKilledServer(HelperServerBase *srv, bool &needsNewServers); public: - wordlist *cmdline; + wordlist *cmdline = nullptr; dlink_list servers; std::queue queue; - const char *id_name; - Helper::ChildConfig childs; ///< Configuration settings for number running. - int ipc_type; + const char *id_name = nullptr; + Helper::ChildConfig childs; ///< configuration settings for number running + int ipc_type = 0; Ip::Address addr; - unsigned int droppedRequests; ///< requests not sent during helper overload - time_t overloadStart; ///< when the helper became overloaded (zero if it is not) - time_t last_queue_warn; - time_t last_restart; - time_t timeout; ///< Requests timeout - bool retryTimedOut; ///< Whether the timed-out requests must retried - bool retryBrokenHelper; ///< Whether the requests must retried on BH replies - SBuf onTimedOutResponse; ///< The response to use when helper response timedout - char eom; ///< The char which marks the end of (response) message, normally '\n' + unsigned int droppedRequests = 0; ///< requests not sent during helper overload + time_t overloadStart = 0; ///< when the helper became overloaded (zero if it is not) + time_t last_queue_warn = 0; + time_t last_restart = 0; + time_t timeout = 0; ///< requests timeout + bool retryTimedOut = false; ///< whether the timed-out requests must retried + bool retryBrokenHelper = false; ///< whether the requests must retried on BH replies + SBuf onTimedOutResponse; ///< the response to use when helper response timedout + char eom = '\n'; ///< the char which marks the end of (response) message struct _stats { - int requests; - int replies; - int timedout; - int queue_size; - int avg_svc_time; + int requests = 0; + int replies = 0; + int timedout = 0; + int queue_size = 0; + int avg_svc_time = 0; } stats; protected: @@ -144,8 +133,8 @@ class statefulhelper : public helper public: typedef std::unordered_map Reservations; - inline statefulhelper(const char *name) : helper(name) {} - inline ~statefulhelper() {} + explicit statefulhelper(const char *name) : helper(name) {} + virtual ~statefulhelper() = default; public: /// reserve the given server @@ -163,7 +152,7 @@ private: void submit(const char *buf, HLPCB * callback, void *data, const Helper::ReservationId & reservation); bool trySubmit(const char *buf, HLPCB * callback, void *data, const Helper::ReservationId & reservation); - ///< reserved servers indexed by reservation IDs + /// reserved servers indexed by reservation IDs Reservations reservations; }; @@ -171,7 +160,9 @@ private: class HelperServerBase: public CbdataParent { public: + HelperServerBase(const char *desc, Ip::Address &, int aPid, void *aIpc, int rfd, int wfd); virtual ~HelperServerBase(); + /** Closes pipes to the helper safely. * Handles the case where the read and write pipes are the same FD. * @@ -201,15 +192,15 @@ public: /// Helper program identifier; does not change when contents do, /// including during assignment const InstanceId index; - int pid; + int pid = 0; Ip::Address addr; Comm::ConnectionPointer readPipe; Comm::ConnectionPointer writePipe; - void *hIpc; + void *hIpc = nullptr; - char *rbuf; - size_t rbuf_sz; - size_t roffset; + char *rbuf = nullptr; + size_t rbuf_sz = 0; + size_t roffset = 0; struct timeval dispatch_time; struct timeval answer_time; @@ -217,27 +208,23 @@ public: dlink_node link; struct _helper_flags { - bool writing; - bool closing; - bool shutdown; + bool writing = false; + bool closing = false; + bool shutdown = false; } flags; typedef std::list Requests; Requests requests; ///< requests in order of submission/expiration struct { - uint64_t uses; //< requests sent to this helper - uint64_t replies; //< replies received from this helper - uint64_t pending; //< queued lookups waiting to be sent to this helper - uint64_t releases; //< times release() has been called on this helper (if stateful) - uint64_t timedout; //< requests which timed-out + uint64_t uses = 0; ///< requests sent to this helper + uint64_t replies = 0; ///< replies received from this helper + uint64_t pending = 0; ///< queued lookups waiting to be sent to this helper + uint64_t releases = 0; ///< times release() has been called on this helper (if stateful) + uint64_t timedout = 0; ///< requests which timed-out } stats; - void initStats(); }; -class MemBuf; -class CommTimeoutCbParams; - // TODO: Rename to StatelessHelperServer and rename HelperServerBase to HelperServer. /// represents a single "stateless helper" process class helper_server : public HelperServerBase @@ -245,27 +232,29 @@ class helper_server : public HelperServerBase CBDATA_CHILD(helper_server); public: - uint64_t nextRequestId; + helper_server(helper *hlp, int pid, void *hIpc, int rfd, int wfd); + virtual ~helper_server(); + + uint64_t nextRequestId = 0; - MemBuf *wqueue; - MemBuf *writebuf; + MemBuf *wqueue = nullptr; + MemBuf *writebuf = nullptr; - helper *parent; + helper *parent = nullptr; /// The helper request Xaction object for the current reply . /// A helper reply may be distributed to more than one of the retrieved /// packets from helper. This member stores the Xaction object as long as /// the end-of-message for current reply is not retrieved. - Helper::Xaction *replyXaction; + Helper::Xaction *replyXaction = nullptr; /// Whether to ignore current message, because it is timed-out or other reason - bool ignoreToEom; + bool ignoreToEom = false; // STL says storing std::list iterators is safe when changing the list typedef std::map RequestIndex; RequestIndex requestsIndex; ///< maps request IDs to requests - virtual ~helper_server(); /// Search in queue for the request with requestId, return the related /// Xaction object and remove it from queue. /// If concurrency is disabled then the requestId is ignored and the @@ -295,7 +284,9 @@ class helper_stateful_server : public HelperServerBase CBDATA_CHILD(helper_stateful_server); public: + helper_stateful_server(statefulhelper *hlp, int pid, void *hIpc, int rfd, int wfd); virtual ~helper_stateful_server(); + void reserve(); void clearReservation(); @@ -306,14 +297,14 @@ public: /// close handler to handle exited server processes static void HelperServerClosed(helper_stateful_server *srv); - statefulhelper *parent; + statefulhelper *parent = nullptr; // Reservations temporary lock the server for an exclusive "client" use. The // client keeps the reservation ID as a proof of her reservation. If a // reservation expires, and the server is reserved for another client, then // the reservation ID presented by the late client will not match ours. Helper::ReservationId reservationId; ///< "confirmation ID" of the last - time_t reservationStart; ///< when the last `reservation` was made + time_t reservationStart = 0; ///< when the last reservation was made }; /* helper.c */ @@ -325,4 +316,3 @@ void helperShutdown(helper * hlp); void helperStatefulShutdown(statefulhelper * hlp); #endif /* SQUID_HELPER_H */ -