From 4c218615874e3002e1dd4d1d047821115027fac0 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Fri, 12 Feb 2021 13:33:16 +0000 Subject: [PATCH] Protect SMP kids from unsolicited IPC answers (#771) When an SMP kid restarts, it recreates its IPC socket to flush any old messages, but it might still receive IPC messages intended for its previous OS process because the sender may write the message _after_ kid's IPC socket is flushed. Squid IPC communication is connectionless, so the sender cannot easily detect the reopening of the recipient socket to prevent this race condition. Some notifications are desirable across kid restarts, so properly switching to connection-oriented IPC communication would only complicate things further. This change protects kids from, for the lack of a better term, an "unsolicited" answer: An answer to a question the recipient did not ask. When allowed to reach regular message-handling code, unsolicited answers result in misleading diagnostics, may trigger assertions, and might even result in a bad recipient state. For example, a kid might think it has been successfully registered with Coordinator while its registration attempt was actually dropped due to a Coordinator restart. Our protection targets one specific use case: Responses that were born (or became) "unsolicited" due to a recipient restart. Other problematic cases may not require any protection, may require a very different protection mechanism (e.g. cryptography), may deal with requests rather than responses, or even cannot be reliably detected. For example: * messages sent by a malicious attacker * requests sent by a misconfigured Squid instance * requests sent by a previous Squid instance * messages sent by a no-longer-running kid process * messages sent by buggy Squid code ---- Also marked a few out-of-scope problems/improvements, including a bug: Improved handling of Coordinator and Strand exceptions exposed and partially addressed an old problem: When configured to listen on an inaccessible port, Squid used to kill the Coordinator job, resulting in subsequent kid registration timeouts. Squid now correctly keeps the Coordinator job running, logging a detailed report: WARNING: Ignoring problematic IPC message message type: 5 problem: check failed: fd >= 0 exception location: TypedMsgHdr.cc(198) putFd Still, the affected kids do not report the port opening problem and continue running, listening on the problem-free ports (if any). Depending on the exception timing, similar behavior was possible before these changes. The correct action here is to send the port opening error to the kid process without throwing the putFd() exception, but we should decide how Squid should handle such inaccessible configured ports first. --- src/DiskIO/IpcIo/IpcIoFile.cc | 5 +-- src/base/Makefile.am | 1 + src/base/TypeTraits.h | 41 ++++++++++++++++++++ src/ip/Address.h | 3 +- src/ipc/Coordinator.cc | 8 ++-- src/ipc/Forwarder.cc | 7 ++-- src/ipc/Forwarder.h | 8 ++-- src/ipc/Inquirer.cc | 6 +-- src/ipc/Inquirer.h | 6 +-- src/ipc/Makefile.am | 4 ++ src/ipc/Port.cc | 22 ++++++++++- src/ipc/Port.h | 4 +- src/ipc/QuestionerId.cc | 51 +++++++++++++++++++++++++ src/ipc/QuestionerId.h | 72 +++++++++++++++++++++++++++++++++++ src/ipc/Request.h | 32 ++++++++++------ src/ipc/RequestId.cc | 28 ++++++++++++++ src/ipc/RequestId.h | 64 +++++++++++++++++++++++++++++++ src/ipc/Response.h | 28 +++++++------- src/ipc/SharedListen.cc | 51 ++++++++++++++----------- src/ipc/SharedListen.h | 13 +++++-- src/ipc/Strand.cc | 13 ++++--- src/ipc/StrandCoord.cc | 9 +++-- src/ipc/StrandCoord.h | 10 ++++- src/ipc/StrandSearch.cc | 8 +++- src/ipc/StrandSearch.h | 4 +- src/ipc/TypedMsgHdr.h | 26 ++++++++++--- src/ipc/forward.h | 8 ++-- src/mgr/Action.cc | 2 +- src/mgr/Action.h | 2 +- src/mgr/Filler.cc | 3 +- src/mgr/Filler.h | 5 ++- src/mgr/Forwarder.cc | 5 ++- src/mgr/FunAction.cc | 1 + src/mgr/InfoAction.cc | 1 + src/mgr/Request.cc | 13 ++----- src/mgr/Request.h | 5 +-- src/mgr/Response.cc | 11 ++---- src/mgr/Response.h | 6 +-- src/snmp/Forwarder.cc | 4 +- src/snmp/Forwarder.h | 2 +- src/snmp/Request.cc | 12 +----- src/snmp/Request.h | 5 +-- src/snmp/Response.cc | 17 ++------- src/snmp/Response.h | 7 +--- src/tests/stub_libmgr.cc | 4 +- 45 files changed, 471 insertions(+), 166 deletions(-) create mode 100644 src/base/TypeTraits.h create mode 100644 src/ipc/QuestionerId.cc create mode 100644 src/ipc/QuestionerId.h create mode 100644 src/ipc/RequestId.cc create mode 100644 src/ipc/RequestId.h diff --git a/src/DiskIO/IpcIo/IpcIoFile.cc b/src/DiskIO/IpcIo/IpcIoFile.cc index 90c8d9cb35..67b9498537 100644 --- a/src/DiskIO/IpcIo/IpcIoFile.cc +++ b/src/DiskIO/IpcIo/IpcIoFile.cc @@ -150,10 +150,7 @@ IpcIoFile::open(int flags, mode_t mode, RefCount callback) return; } - Ipc::StrandSearchRequest request; - request.requestorId = KidIdentifier; - request.tag = dbName; - + const Ipc::StrandSearchRequest request(dbName); Ipc::TypedMsgHdr msg; request.pack(msg); Ipc::SendMessage(Ipc::Port::CoordinatorAddr(), msg); diff --git a/src/base/Makefile.am b/src/base/Makefile.am index 4792522f8d..1366df5b75 100644 --- a/src/base/Makefile.am +++ b/src/base/Makefile.am @@ -49,5 +49,6 @@ libbase_la_SOURCES = \ Subscription.h \ TextException.cc \ TextException.h \ + TypeTraits.h \ YesNoNone.h \ forward.h diff --git a/src/base/TypeTraits.h b/src/base/TypeTraits.h new file mode 100644 index 0000000000..719fceecbc --- /dev/null +++ b/src/base/TypeTraits.h @@ -0,0 +1,41 @@ +/* + * Copyright (C) 1996-2020 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_BASE_TYPETRAITS_H +#define SQUID_SRC_BASE_TYPETRAITS_H + +namespace TypeTraits_ { // a hack to prevent "unintended ADL" + +// TODO: Extract reusable paradigms into other mixins (e.g., NonCopyable). +/// convenience base for any class with pure virtual method(s) +class Interface +{ +public: + // ensures proper destruction via pointers to base interface classes + virtual ~Interface() = default; + + // prohibits copy/move assignment to prevent accidental object slicing + Interface &operator=(const Interface &) = delete; + Interface &operator=(Interface &&) = delete; + +protected: // prevents accidental creation of Interface instances + + // allows default-construction in kids + constexpr Interface() = default; + + // allows copy/move construction for kids convenience + Interface(const Interface &) = default; + Interface(Interface &&) = default; +}; + +} // namespace TypeTraits_ + +using Interface = TypeTraits_::Interface; + +#endif /* SQUID_SRC_BASE_TYPETRAITS_H */ + diff --git a/src/ip/Address.h b/src/ip/Address.h index 7555e28ff5..d40f7f855d 100644 --- a/src/ip/Address.h +++ b/src/ip/Address.h @@ -41,7 +41,7 @@ class Address { public: - /** @name Constructors and Destructor */ + /** @name Constructors */ /*@{*/ Address() { setEmpty(); } Address(const struct in_addr &); @@ -51,7 +51,6 @@ public: Address(const struct hostent &); Address(const struct addrinfo &); Address(const char*); - ~Address() {} /*@}*/ /** @name Assignment Operators */ diff --git a/src/ipc/Coordinator.cc b/src/ipc/Coordinator.cc index 012e6fb8e2..6202cd175b 100644 --- a/src/ipc/Coordinator.cc +++ b/src/ipc/Coordinator.cc @@ -107,7 +107,7 @@ void Ipc::Coordinator::receive(const TypedMsgHdr& message) case mtCacheMgrResponse: { debugs(54, 6, HERE << "Cache manager response"); const Mgr::Response resp(message); - handleCacheMgrResponse(resp); + handleCacheMgrResponse(Mine(resp)); } break; @@ -122,13 +122,13 @@ void Ipc::Coordinator::receive(const TypedMsgHdr& message) case mtSnmpResponse: { debugs(54, 6, HERE << "SNMP response"); const Snmp::Response resp(message); - handleSnmpResponse(resp); + handleSnmpResponse(Mine(resp)); } break; #endif default: - debugs(54, DBG_IMPORTANT, "WARNING: Ignoring IPC message with an unknown type: " << message.rawType()); + Port::receive(message); break; } } @@ -222,7 +222,7 @@ Ipc::Coordinator::notifySearcher(const Ipc::StrandSearchRequest &request, { debugs(54, 3, HERE << "tell kid" << request.requestorId << " that " << request.tag << " is kid" << strand.kidId); - const StrandMessage response(strand); + const StrandMessage response(strand, request.qid); TypedMsgHdr message; response.pack(mtStrandReady, message); SendMessage(MakeAddr(strandAddrLabel, request.requestorId), message); diff --git a/src/ipc/Forwarder.cc b/src/ipc/Forwarder.cc index 70ec73a7b8..46df988763 100644 --- a/src/ipc/Forwarder.cc +++ b/src/ipc/Forwarder.cc @@ -16,12 +16,13 @@ #include "HttpRequest.h" #include "ipc/Forwarder.h" #include "ipc/Port.h" +#include "ipc/RequestId.h" #include "ipc/TypedMsgHdr.h" CBDATA_NAMESPACED_CLASS_INIT(Ipc, Forwarder); Ipc::Forwarder::RequestsMap Ipc::Forwarder::TheRequestsMap; -unsigned int Ipc::Forwarder::LastRequestId = 0; +Ipc::RequestId::Index Ipc::Forwarder::LastRequestId = 0; Ipc::Forwarder::Forwarder(Request::Pointer aRequest, double aTimeout): AsyncJob("Ipc::Forwarder"), @@ -149,7 +150,7 @@ Ipc::Forwarder::callException(const std::exception& e) /// returns and forgets the right Forwarder callback for the request AsyncCall::Pointer -Ipc::Forwarder::DequeueRequest(unsigned int requestId) +Ipc::Forwarder::DequeueRequest(const RequestId::Index requestId) { debugs(54, 3, HERE); Must(requestId != 0); @@ -172,7 +173,7 @@ Ipc::Forwarder::removeTimeoutEvent() } void -Ipc::Forwarder::HandleRemoteAck(unsigned int requestId) +Ipc::Forwarder::HandleRemoteAck(const RequestId requestId) { debugs(54, 3, HERE); Must(requestId != 0); diff --git a/src/ipc/Forwarder.h b/src/ipc/Forwarder.h index 69c957b773..7179619a17 100644 --- a/src/ipc/Forwarder.h +++ b/src/ipc/Forwarder.h @@ -35,7 +35,7 @@ public: virtual ~Forwarder(); /// finds and calls the right Forwarder upon Coordinator's response - static void HandleRemoteAck(unsigned int requestId); + static void HandleRemoteAck(RequestId); /* has-to-be-public AsyncJob API */ virtual void callException(const std::exception& e); @@ -59,17 +59,17 @@ private: void handleRemoteAck(); - static AsyncCall::Pointer DequeueRequest(unsigned int requestId); + static AsyncCall::Pointer DequeueRequest(RequestId::Index); protected: Request::Pointer request; const double timeout; ///< response wait timeout in seconds /// maps request->id to Forwarder::handleRemoteAck callback - typedef std::map RequestsMap; + typedef std::map RequestsMap; static RequestsMap TheRequestsMap; ///< pending Coordinator requests - static unsigned int LastRequestId; ///< last requestId used + static RequestId::Index LastRequestId; ///< last requestId used }; } // namespace Ipc diff --git a/src/ipc/Inquirer.cc b/src/ipc/Inquirer.cc index 6ce807ce3f..481710c47a 100644 --- a/src/ipc/Inquirer.cc +++ b/src/ipc/Inquirer.cc @@ -21,7 +21,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Ipc, Inquirer); Ipc::Inquirer::RequestsMap Ipc::Inquirer::TheRequestsMap; -unsigned int Ipc::Inquirer::LastRequestId = 0; +Ipc::RequestId::Index Ipc::Inquirer::LastRequestId = 0; /// compare Ipc::StrandCoord using kidId, for std::sort() below static bool @@ -139,7 +139,7 @@ Ipc::Inquirer::callException(const std::exception& e) /// returns and forgets the right Inquirer callback for strand request AsyncCall::Pointer -Ipc::Inquirer::DequeueRequest(unsigned int requestId) +Ipc::Inquirer::DequeueRequest(const RequestId::Index requestId) { debugs(54, 3, HERE << " requestId " << requestId); Must(requestId != 0); @@ -206,7 +206,7 @@ Ipc::Inquirer::status() const { static MemBuf buf; buf.reset(); - buf.appendf(" [request->requestId %u]", request->requestId); + buf.appendf(" [requestId %u]", request->requestId.index()); buf.terminate(); return buf.content(); } diff --git a/src/ipc/Inquirer.h b/src/ipc/Inquirer.h index 527252aa60..d97ea884a5 100644 --- a/src/ipc/Inquirer.h +++ b/src/ipc/Inquirer.h @@ -64,7 +64,7 @@ private: void handleRemoteAck(Response::Pointer response); - static AsyncCall::Pointer DequeueRequest(unsigned int requestId); + static AsyncCall::Pointer DequeueRequest(RequestId::Index); static void RequestTimedOut(void* param); void requestTimedOut(); @@ -79,10 +79,10 @@ protected: const double timeout; ///< number of seconds to wait for strand response /// maps request->id to Inquirer::handleRemoteAck callback - typedef std::map RequestsMap; + typedef std::map RequestsMap; static RequestsMap TheRequestsMap; ///< pending strand requests - static unsigned int LastRequestId; ///< last requestId used + static RequestId::Index LastRequestId; ///< last requestId used }; } // namespace Ipc diff --git a/src/ipc/Makefile.am b/src/ipc/Makefile.am index e84da0db72..dd9ec392ff 100644 --- a/src/ipc/Makefile.am +++ b/src/ipc/Makefile.am @@ -28,11 +28,15 @@ libipc_la_SOURCES = \ Messages.h \ Port.cc \ Port.h \ + QuestionerId.cc \ + QuestionerId.h \ Queue.cc \ Queue.h \ ReadWriteLock.cc \ ReadWriteLock.h \ Request.h \ + RequestId.cc \ + RequestId.h \ Response.h \ SharedListen.cc \ SharedListen.h \ diff --git a/src/ipc/Port.cc b/src/ipc/Port.cc index f3d1a3853f..3e9f06b436 100644 --- a/src/ipc/Port.cc +++ b/src/ipc/Port.cc @@ -14,6 +14,7 @@ #include "comm/Read.h" #include "CommCalls.h" #include "ipc/Port.h" +#include "sbuf/Stream.h" #include "tools.h" #include "util.h" @@ -73,6 +74,25 @@ Ipc::Port::CoordinatorAddr() return coordinatorAddr; } +void +Ipc::Port::receive(const TypedMsgHdr &message) +{ + throw TextException(ToSBuf("bad IPC message type: ", message.rawType()), Here()); +} + +/// receive() but ignore any errors +void +Ipc::Port::receiveOrIgnore(const TypedMsgHdr &message) +{ + try { + receive(message); + } catch (...) { + debugs(54, DBG_IMPORTANT, "WARNING: Ignoring IPC message" << + Debug::Extra << "message type: " << message.rawType() << + Debug::Extra << "problem: " << CurrentException); + } +} + void Ipc::Port::noteRead(const CommIoCbParams& params) { debugs(54, 6, HERE << params.conn << " flag " << params.flag << @@ -80,7 +100,7 @@ void Ipc::Port::noteRead(const CommIoCbParams& params) if (params.flag == Comm::OK) { assert(params.buf == buf.raw()); debugs(54, 6, "message type: " << buf.rawType()); - receive(buf); + receiveOrIgnore(buf); } // TODO: if there was a fatal error on our socket, close the socket before // trying to listen again and print a level-1 error message. diff --git a/src/ipc/Port.h b/src/ipc/Port.h index df9e9b9064..9f8e6ae9d5 100644 --- a/src/ipc/Port.h +++ b/src/ipc/Port.h @@ -36,10 +36,12 @@ protected: void doListen(); /// handle IPC message just read - virtual void receive(const TypedMsgHdr& message) = 0; + /// kids must call parent method when they do not recognize the message type + virtual void receive(const TypedMsgHdr &) = 0; private: void noteRead(const CommIoCbParams ¶ms); // Comm callback API + void receiveOrIgnore(const TypedMsgHdr& ); private: TypedMsgHdr buf; ///< msghdr struct filled by Comm diff --git a/src/ipc/QuestionerId.cc b/src/ipc/QuestionerId.cc new file mode 100644 index 0000000000..c530bb9f18 --- /dev/null +++ b/src/ipc/QuestionerId.cc @@ -0,0 +1,51 @@ +/* + * Copyright (C) 1996-2020 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. + */ + +#include "squid.h" +#include "base/TextException.h" +#include "ipc/QuestionerId.h" +#include "ipc/TypedMsgHdr.h" +#include "sbuf/Stream.h" + +#include + +Ipc::QuestionerId +Ipc::MyQuestionerId() +{ + static const QuestionerId qid(getpid()); + return qid; +} + +void +Ipc::QuestionerId::pack(TypedMsgHdr &hdrMsg) const +{ + hdrMsg.putPod(pid); +} + +void +Ipc::QuestionerId::unpack(const TypedMsgHdr &hdrMsg) +{ + hdrMsg.getPod(pid); +} + +void +Ipc::QuestionerId::rejectAnswerIfStale() const +{ + const auto myPid = MyQuestionerId().pid; + if (myPid != pid) { + throw TextException(ToSBuf("received answer to an IPC question asked by process ", pid, + Debug::Extra, "my process PID: ", myPid), Here()); + } +} + +void +Ipc::QuestionerId::print(std::ostream &os) const +{ + os << pid; +} + diff --git a/src/ipc/QuestionerId.h b/src/ipc/QuestionerId.h new file mode 100644 index 0000000000..ee0efff033 --- /dev/null +++ b/src/ipc/QuestionerId.h @@ -0,0 +1,72 @@ +/* + * Copyright (C) 1996-2020 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_IPC_QUESTIONERID_H +#define SQUID_SRC_IPC_QUESTIONERID_H + +#include "ipc/forward.h" + +#include + +namespace Ipc +{ + +/// Identifies a kid process sending IPC messages that require an answer. +/// Must be unique across all kids with pending questions. +class QuestionerId +{ +public: + /// to-be-determined ID + QuestionerId() = default; + + /// for sending the ID of the asking process + void pack(TypedMsgHdr &) const; + + /// for receiving the ID of the asking process + void unpack(const TypedMsgHdr &); + + /// does nothing but throws if the questioner was not the current process + void rejectAnswerIfStale() const; + + /// reports the stored opaque ID value (for debugging) + void print(std::ostream &) const; + +private: + /// for MyQuestionerId() convenience + explicit QuestionerId(const pid_t aPid): pid(aPid) {} + friend QuestionerId MyQuestionerId(); + + /// OS process ID of the asking kid. If the kid restarts, it is assumed + /// not to wrap back to the old value until the answer is received. + pid_t pid = -1; +}; + +/// the questioner ID of the current/calling process +QuestionerId MyQuestionerId(); + +/// Convenience wrapper for rejecting (freshly parsed) stale answers. +/// All answers are assumed to have a "QuestionerId intendedRecepient()" member. +template +const Answer & +Mine(const Answer &answer) +{ + answer.intendedRecepient().rejectAnswerIfStale(); + return answer; +} + +inline std::ostream & +operator <<(std::ostream &os, const QuestionerId &qid) +{ + qid.print(os); + return os; +} + +} // namespace Ipc; + +#endif /* SQUID_SRC_IPC_QUESTIONERID_H */ + diff --git a/src/ipc/Request.h b/src/ipc/Request.h index 31006364f9..089c190c5d 100644 --- a/src/ipc/Request.h +++ b/src/ipc/Request.h @@ -12,31 +12,41 @@ #define SQUID_IPC_REQUEST_H #include "base/RefCount.h" -#include "ipc/forward.h" +#include "base/TypeTraits.h" +#include "ipc/RequestId.h" namespace Ipc { +// TODO: Request and Response ought to have their own un/pack() methods instead +// of duplicating their functionality in derived classes. To avoid dependency +// loops between libipc and libmgr/libsnmp, fixing that requires extracting +// src/ipc/Coordinator and its friends into a new src/coordinator/ library. + /// IPC request -class Request: public RefCountable +class Request: public RefCountable, public Interface { public: typedef RefCount Pointer; public: - Request(int aRequestorId, unsigned int aRequestId): - requestorId(aRequestorId), requestId(aRequestId) {} - virtual void pack(TypedMsgHdr& msg) const = 0; ///< prepare for sendmsg() virtual Pointer clone() const = 0; ///< returns a copy of this -private: - Request(const Request&); // not implemented - Request& operator= (const Request&); // not implemented - public: - int requestorId; ///< kidId of the requestor; used for response destination - unsigned int requestId; ///< unique for sender; matches request w/ response + int requestorId = 0; ///< kidId of the requestor; used for response destination + RequestId requestId; ///< matches the request[or] with the response + +protected: + /// sender's constructor + Request(const int aRequestorId, const RequestId aRequestId): + requestorId(aRequestorId), + requestId(aRequestId) + { + } + + /// recipient's constructor + Request() = default; }; } // namespace Ipc diff --git a/src/ipc/RequestId.cc b/src/ipc/RequestId.cc new file mode 100644 index 0000000000..c27cb4f0a4 --- /dev/null +++ b/src/ipc/RequestId.cc @@ -0,0 +1,28 @@ +/* + * Copyright (C) 1996-2020 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. + */ + +#include "squid.h" +#include "base/TextException.h" +#include "Debug.h" +#include "ipc/RequestId.h" + +#include + +Ipc::RequestId::RequestId(const Index anIndex): + qid_(anIndex ? MyQuestionerId() : QuestionerId()), + index_(anIndex) +{ +} + +std::ostream & +Ipc::operator <<(std::ostream &os, const RequestId &requestId) +{ + os << requestId.index() << '@' << requestId.questioner(); + return os; +} + diff --git a/src/ipc/RequestId.h b/src/ipc/RequestId.h new file mode 100644 index 0000000000..64270341b5 --- /dev/null +++ b/src/ipc/RequestId.h @@ -0,0 +1,64 @@ +/* + * Copyright (C) 1996-2020 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_IPC_REQUESTID_H +#define SQUID_IPC_REQUESTID_H + +#include "ipc/forward.h" +#include "ipc/QuestionerId.h" + +#include + +namespace Ipc +{ + +/// uniquely identifies an IPC request among same-type concurrent IPC requests +/// submitted by a single Squid instance +class RequestId +{ +public: + /// A simple ID for correlating IPC responses with pending requests. + /// Value 0 has a special meaning of "unset/unknown", but otherwise opaque. + typedef unsigned int Index; + + /// Request sender's constructor. + /// For performance and clarity sake, default constructor is preferred to 0 index. + explicit RequestId(Index); + + /// request recipient's constructor + RequestId() = default; + + /// Make the ID unset/unknown. + /// Optimization: leaves the questioner field alone. + void reset() { index_ = 0; } + + /// Make the ID set/known with the given (by the questioner) index. + /// For performance and clarity sake, reset(void) is preferred to reset(0). + void reset(const Index anIndex) { *this = RequestId(anIndex); } + + QuestionerId questioner() const { return qid_; } + Index index() const { return index_; } + + // these conversion operators allow our users to treat us as an Index + operator Index() const { return index_; } + RequestId &operator =(const Index anIndex) { anIndex ? reset(anIndex) : reset(); return *this; } + +private: + /// the sender of the request + QuestionerId qid_; + + /// request ID; unique within pending same-qid_ questions of the same kind + Index index_ = 0; +}; + +std::ostream &operator <<(std::ostream &, const RequestId &); + +} // namespace Ipc; + +#endif /* SQUID_IPC_REQUESTID_H */ + diff --git a/src/ipc/Response.h b/src/ipc/Response.h index 76ba916507..ce278167df 100644 --- a/src/ipc/Response.h +++ b/src/ipc/Response.h @@ -12,38 +12,36 @@ #define SQUID_IPC_RESPONSE_H #include "base/RefCount.h" +#include "base/TypeTraits.h" #include "ipc/forward.h" +#include "ipc/QuestionerId.h" namespace Ipc { /// A response to Ipc::Request. -class Response: public RefCountable +class Response: public RefCountable, public Interface { public: typedef RefCount Pointer; public: - explicit Response(unsigned int aRequestId): - requestId(aRequestId) {} - virtual void pack(TypedMsgHdr& msg) const = 0; ///< prepare for sendmsg() virtual Pointer clone() const = 0; ///< returns a copy of this -private: - Response(const Response&); // not implemented - Response& operator= (const Response&); // not implemented + /// for Mine() tests + QuestionerId intendedRecepient() const { return requestId.questioner(); } public: - unsigned int requestId; ///< ID of request we are responding to -}; + RequestId requestId; ///< the ID of the request we are responding to -inline -std::ostream& operator << (std::ostream &os, const Response& response) -{ - os << "[response.requestId %u]" << response.requestId << '}'; - return os; -} +protected: + /// sender's constructor + explicit Response(const RequestId aRequestId): requestId(aRequestId) {} + + /// recipient's constructor + Response() = default; +}; } // namespace Ipc diff --git a/src/ipc/SharedListen.cc b/src/ipc/SharedListen.cc index 60e9e35801..d873bf26ce 100644 --- a/src/ipc/SharedListen.cc +++ b/src/ipc/SharedListen.cc @@ -33,25 +33,26 @@ public: }; /// maps ID assigned at request time to the response callback -typedef std::map SharedListenRequestMap; +typedef std::map SharedListenRequestMap; static SharedListenRequestMap TheSharedListenRequestMap; /// accumulates delayed requests until they are ready to be sent, in FIFO order typedef std::list DelayedSharedListenRequests; static DelayedSharedListenRequests TheDelayedRequests; -static int +// TODO: Encapsulate "Pending Request Map" logic shared by all RequestId users. +/// registers the given request in the collection of pending requests +/// \returns the registration key +static Ipc::RequestId::Index AddToMap(const PendingOpenRequest &por) { - // find unused ID using linear search; there should not be many entries - for (int id = 0; true; ++id) { - if (TheSharedListenRequestMap.find(id) == TheSharedListenRequestMap.end()) { - TheSharedListenRequestMap[id] = por; - return id; - } - } - assert(false); // not reached - return -1; + static Ipc::RequestId::Index LastIndex = 0; + // TODO: Switch Ipc::RequestId::Index to uint64_t and drop these 0 checks. + if (++LastIndex == 0) // don't use zero value as an ID + ++LastIndex; + assert(TheSharedListenRequestMap.find(LastIndex) == TheSharedListenRequestMap.end()); + TheSharedListenRequestMap[LastIndex] = por; + return LastIndex; } bool @@ -68,7 +69,10 @@ Ipc::OpenListenerParams::operator <(const OpenListenerParams &p) const return addr.compareWhole(p.addr) < 0; } -Ipc::SharedListenRequest::SharedListenRequest(): requestorId(-1), mapId(-1) +Ipc::SharedListenRequest::SharedListenRequest(const OpenListenerParams &aParams, const RequestId aMapId): + requestorId(KidIdentifier), + params(aParams), + mapId(aMapId) { // caller will then set public data members } @@ -76,22 +80,25 @@ Ipc::SharedListenRequest::SharedListenRequest(): requestorId(-1), mapId(-1) Ipc::SharedListenRequest::SharedListenRequest(const TypedMsgHdr &hdrMsg) { hdrMsg.checkType(mtSharedListenRequest); + // XXX: our handlerSubscription is not a POD! hdrMsg.getPod(*this); } void Ipc::SharedListenRequest::pack(TypedMsgHdr &hdrMsg) const { hdrMsg.setType(mtSharedListenRequest); + // XXX: our handlerSubscription is not a POD! hdrMsg.putPod(*this); } -Ipc::SharedListenResponse::SharedListenResponse(int aFd, int anErrNo, int aMapId): +Ipc::SharedListenResponse::SharedListenResponse(const int aFd, const int anErrNo, const RequestId aMapId): fd(aFd), errNo(anErrNo), mapId(aMapId) { } Ipc::SharedListenResponse::SharedListenResponse(const TypedMsgHdr &hdrMsg): - fd(-1), errNo(0), mapId(-1) + fd(-1), + errNo(0) { hdrMsg.checkType(mtSharedListenResponse); hdrMsg.getPod(*this); @@ -103,16 +110,14 @@ void Ipc::SharedListenResponse::pack(TypedMsgHdr &hdrMsg) const { hdrMsg.setType(mtSharedListenResponse); hdrMsg.putPod(*this); + // XXX: When we respond with an error, putFd() throws due to the negative fd hdrMsg.putFd(fd); } static void SendSharedListenRequest(const PendingOpenRequest &por) { - Ipc::SharedListenRequest request; - request.requestorId = KidIdentifier; - request.params = por.params; - request.mapId = AddToMap(por); + const Ipc::SharedListenRequest request(por.params, Ipc::RequestId(AddToMap(por))); debugs(54, 3, "getting listening FD for " << request.params.addr << " mapId=" << request.mapId); @@ -160,10 +165,12 @@ void Ipc::SharedListenJoined(const SharedListenResponse &response) TheSharedListenRequestMap.size() << " active + " << TheDelayedRequests.size() << " delayed requests"); - Must(TheSharedListenRequestMap.find(response.mapId) != TheSharedListenRequestMap.end()); - PendingOpenRequest por = TheSharedListenRequestMap[response.mapId]; - Must(por.callback != NULL); - TheSharedListenRequestMap.erase(response.mapId); + Must(response.mapId); + const auto pori = TheSharedListenRequestMap.find(response.mapId.index()); + Must(pori != TheSharedListenRequestMap.end()); + auto por = pori->second; + Must(por.callback); + TheSharedListenRequestMap.erase(pori); StartListeningCb *cbd = dynamic_cast(por.callback->getDialer()); assert(cbd && cbd->conn != NULL); diff --git a/src/ipc/SharedListen.h b/src/ipc/SharedListen.h index 13a0700496..a6a7059b16 100644 --- a/src/ipc/SharedListen.h +++ b/src/ipc/SharedListen.h @@ -13,6 +13,8 @@ #include "base/AsyncCall.h" #include "base/Subscription.h" +#include "ipc/QuestionerId.h" +#include "ipc/RequestId.h" namespace Ipc { @@ -45,7 +47,7 @@ class TypedMsgHdr; class SharedListenRequest { public: - SharedListenRequest(); ///< from OpenSharedListen() which then sets public data + SharedListenRequest(const OpenListenerParams &, RequestId aMapId); ///< sender's constructor explicit SharedListenRequest(const TypedMsgHdr &hdrMsg); ///< from recvmsg() void pack(TypedMsgHdr &hdrMsg) const; ///< prepare for sendmsg() @@ -54,21 +56,24 @@ public: OpenListenerParams params; ///< actual comm_open_sharedListen() parameters - int mapId; ///< to map future response to the requestor's callback + RequestId mapId; ///< to map future response to the requestor's callback }; /// a response to SharedListenRequest class SharedListenResponse { public: - SharedListenResponse(int fd, int errNo, int mapId); + SharedListenResponse(int fd, int errNo, RequestId aMapId); ///< sender's constructor explicit SharedListenResponse(const TypedMsgHdr &hdrMsg); ///< from recvmsg() void pack(TypedMsgHdr &hdrMsg) const; ///< prepare for sendmsg() + /// for Mine() tests + QuestionerId intendedRecepient() const { return mapId.questioner(); } + public: int fd; ///< opened listening socket or -1 int errNo; ///< errno value from comm_open_sharedListen() call - int mapId; ///< to map future response to the requestor's callback + RequestId mapId; ///< to map future response to the requestor's callback }; /// prepare and send SharedListenRequest to Coordinator diff --git a/src/ipc/Strand.cc b/src/ipc/Strand.cc index 6d844e9323..240a2aa21c 100644 --- a/src/ipc/Strand.cc +++ b/src/ipc/Strand.cc @@ -18,6 +18,7 @@ #include "globals.h" #include "ipc/Kids.h" #include "ipc/Messages.h" +#include "ipc/QuestionerId.h" #include "ipc/SharedListen.h" #include "ipc/Strand.h" #include "ipc/StrandCoord.h" @@ -62,16 +63,16 @@ void Ipc::Strand::receive(const TypedMsgHdr &message) switch (message.rawType()) { case mtStrandRegistered: - handleRegistrationResponse(StrandMessage(message)); + handleRegistrationResponse(Mine(StrandMessage(message))); break; case mtSharedListenResponse: - SharedListenJoined(SharedListenResponse(message)); + SharedListenJoined(Mine(SharedListenResponse(message))); break; #if HAVE_DISKIO_MODULE_IPCIO case mtStrandReady: - IpcIoFile::HandleOpenResponse(StrandMessage(message)); + IpcIoFile::HandleOpenResponse(Mine(StrandMessage(message))); break; case mtIpcIoNotification: @@ -87,7 +88,7 @@ void Ipc::Strand::receive(const TypedMsgHdr &message) case mtCacheMgrResponse: { const Mgr::Response resp(message); - handleCacheMgrResponse(resp); + handleCacheMgrResponse(Mine(resp)); } break; @@ -104,13 +105,13 @@ void Ipc::Strand::receive(const TypedMsgHdr &message) case mtSnmpResponse: { const Snmp::Response resp(message); - handleSnmpResponse(resp); + handleSnmpResponse(Mine(resp)); } break; #endif default: - debugs(54, DBG_IMPORTANT, "WARNING: Ignoring IPC message with an unknown type: " << message.rawType()); + Port::receive(message); break; } } diff --git a/src/ipc/StrandCoord.cc b/src/ipc/StrandCoord.cc index fc35b0fb86..df299cf075 100644 --- a/src/ipc/StrandCoord.cc +++ b/src/ipc/StrandCoord.cc @@ -38,14 +38,16 @@ void Ipc::StrandCoord::pack(TypedMsgHdr &hdrMsg) const hdrMsg.putString(tag); } -Ipc::StrandMessage::StrandMessage(const StrandCoord &aStrand): - strand(aStrand) +Ipc::StrandMessage::StrandMessage(const StrandCoord &aStrand, const QuestionerId aQid): + strand(aStrand), + qid(aQid) { } Ipc::StrandMessage::StrandMessage(const TypedMsgHdr &hdrMsg) { strand.unpack(hdrMsg); + qid.unpack(hdrMsg); } void @@ -53,13 +55,14 @@ Ipc::StrandMessage::pack(const MessageType messageType, TypedMsgHdr &hdrMsg) con { hdrMsg.setType(messageType); strand.pack(hdrMsg); + qid.pack(hdrMsg); } void Ipc::StrandMessage::NotifyCoordinator(const MessageType msgType, const char *tag) { static const auto pid = getpid(); - StrandMessage message(StrandCoord(KidIdentifier, pid)); + StrandMessage message(StrandCoord(KidIdentifier, pid), MyQuestionerId()); if (tag) message.strand.tag = tag; TypedMsgHdr hdr; diff --git a/src/ipc/StrandCoord.h b/src/ipc/StrandCoord.h index 2758d7777c..1778f0d55a 100644 --- a/src/ipc/StrandCoord.h +++ b/src/ipc/StrandCoord.h @@ -11,6 +11,7 @@ #include "ipc/forward.h" #include "ipc/Messages.h" +#include "ipc/QuestionerId.h" #include "SquidString.h" namespace Ipc @@ -37,15 +38,22 @@ public: class StrandMessage { public: - explicit StrandMessage(const StrandCoord &); + explicit StrandMessage(const StrandCoord &, QuestionerId); explicit StrandMessage(const TypedMsgHdr &); void pack(MessageType, TypedMsgHdr &) const; /// creates and sends StrandMessage to Coordinator static void NotifyCoordinator(MessageType, const char *tag); + /// for Mine() tests + QuestionerId intendedRecepient() const { return qid; } + public: StrandCoord strand; ///< messageType-specific coordinates (e.g., sender) + + /// For IPC requests/questions: The sender of this request. + /// For IPC responses/answers: The sender of the corresponding request. + QuestionerId qid; }; } // namespace Ipc; diff --git a/src/ipc/StrandSearch.cc b/src/ipc/StrandSearch.cc index 13f9406ebb..f75fddaada 100644 --- a/src/ipc/StrandSearch.cc +++ b/src/ipc/StrandSearch.cc @@ -9,11 +9,15 @@ /* DEBUG: section 54 Interprocess Communication */ #include "squid.h" +#include "globals.h" #include "ipc/Messages.h" #include "ipc/StrandSearch.h" #include "ipc/TypedMsgHdr.h" -Ipc::StrandSearchRequest::StrandSearchRequest(): requestorId(-1) +Ipc::StrandSearchRequest::StrandSearchRequest(const String &aTag): + requestorId(KidIdentifier), + tag(aTag), + qid(MyQuestionerId()) { } @@ -23,6 +27,7 @@ Ipc::StrandSearchRequest::StrandSearchRequest(const TypedMsgHdr &hdrMsg): hdrMsg.checkType(mtFindStrand); hdrMsg.getPod(requestorId); hdrMsg.getString(tag); + qid.unpack(hdrMsg); } void Ipc::StrandSearchRequest::pack(TypedMsgHdr &hdrMsg) const @@ -30,5 +35,6 @@ void Ipc::StrandSearchRequest::pack(TypedMsgHdr &hdrMsg) const hdrMsg.setType(mtFindStrand); hdrMsg.putPod(requestorId); hdrMsg.putString(tag); + qid.pack(hdrMsg); } diff --git a/src/ipc/StrandSearch.h b/src/ipc/StrandSearch.h index 3c3cd9d8cf..ad4cb44953 100644 --- a/src/ipc/StrandSearch.h +++ b/src/ipc/StrandSearch.h @@ -10,6 +10,7 @@ #define SQUID_IPC_STRAND_SEARCH_H #include "ipc/forward.h" +#include "ipc/QuestionerId.h" #include "ipc/StrandCoord.h" #include "SquidString.h" @@ -20,13 +21,14 @@ namespace Ipc class StrandSearchRequest { public: - StrandSearchRequest(); + explicit StrandSearchRequest(const String &aTag); ///< sender's constructor explicit StrandSearchRequest(const TypedMsgHdr &hdrMsg); ///< from recvmsg() void pack(TypedMsgHdr &hdrMsg) const; ///< prepare for sendmsg() public: int requestorId; ///< sender-provided return address String tag; ///< set when looking for a matching StrandCoord::tag + QuestionerId qid; ///< the sender of the request }; } // namespace Ipc; diff --git a/src/ipc/TypedMsgHdr.h b/src/ipc/TypedMsgHdr.h index 5fc7ac443f..4953fa4e42 100644 --- a/src/ipc/TypedMsgHdr.h +++ b/src/ipc/TypedMsgHdr.h @@ -48,11 +48,9 @@ public: /// \returns 0 if no message kind has been received or set int rawType() const { return msg_iov ? data.type_ : 0; } - /* access for Plain Old Data (POD)-based message parts */ - template - void getPod(Pod &pod) const { getFixed(&pod, sizeof(pod)); } ///< load POD - template - void putPod(const Pod &pod) { putFixed(&pod, sizeof(pod)); } ///< store POD + /* access for TriviallyCopyable (a.k.a. Plain Old Data or POD) message parts */ + template void getPod(Pod &pod) const; ///< load POD + template void putPod(const Pod &pod); ///< store POD /* access to message parts for selected commonly-used part types */ void getString(String &s) const; ///< load variable-length string @@ -113,5 +111,23 @@ private: } // namespace Ipc +template +void +Ipc::TypedMsgHdr::getPod(Pod &pod) const +{ + // TODO: Enable after fixing Ipc::SharedListenRequest::SharedListenRequest() + //static_assert(std::is_trivially_copyable::value, "getPod() used for a POD"); + getFixed(&pod, sizeof(pod)); +} + +template +void +Ipc::TypedMsgHdr::putPod(const Pod &pod) +{ + // TODO: Enable after fixing Ipc::SharedListenRequest::pack() + //static_assert(std::is_trivially_copyable::value, "putPod() used for a POD"); + putFixed(&pod, sizeof(pod)); +} + #endif /* SQUID_IPC_TYPED_MSG_HDR_H */ diff --git a/src/ipc/forward.h b/src/ipc/forward.h index 8fff7804a1..cee2b9478c 100644 --- a/src/ipc/forward.h +++ b/src/ipc/forward.h @@ -14,13 +14,15 @@ namespace Ipc { -class TypedMsgHdr; -class StrandCoord; -class StrandMessage; class Forwarder; class Inquirer; +class QuestionerId; class Request; +class RequestId; class Response; +class StrandCoord; +class StrandMessage; +class TypedMsgHdr; } // namespace Ipc diff --git a/src/mgr/Action.cc b/src/mgr/Action.cc index 343dceb49b..2ca5eef1e6 100644 --- a/src/mgr/Action.cc +++ b/src/mgr/Action.cc @@ -79,7 +79,7 @@ Mgr::Action::respond(const Request &request) } void -Mgr::Action::sendResponse(unsigned int requestId) +Mgr::Action::sendResponse(const Ipc::RequestId requestId) { Response response(requestId, this); Ipc::TypedMsgHdr message; diff --git a/src/mgr/Action.h b/src/mgr/Action.h index 0efcd12c5e..6ccee5b645 100644 --- a/src/mgr/Action.h +++ b/src/mgr/Action.h @@ -55,7 +55,7 @@ public: virtual void unpack(const Ipc::TypedMsgHdr &) {} /// notify Coordinator that this action is done with local processing - void sendResponse(unsigned int requestId); + void sendResponse(Ipc::RequestId); /* Action properties */ diff --git a/src/mgr/Filler.cc b/src/mgr/Filler.cc index bf50e47df2..4d4a2b0bce 100644 --- a/src/mgr/Filler.cc +++ b/src/mgr/Filler.cc @@ -11,6 +11,7 @@ #include "squid.h" #include "base/TextException.h" #include "comm/Connection.h" +#include "ipc/RequestId.h" #include "mgr/Filler.h" #include "mgr/Response.h" #include "Store.h" @@ -18,7 +19,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Mgr, Filler); Mgr::Filler::Filler(const Action::Pointer &anAction, const Comm::ConnectionPointer &conn, - unsigned int aRequestId): + const Ipc::RequestId aRequestId): StoreToCommWriter(conn, anAction->createStoreEntry()), action(anAction), requestId(aRequestId) diff --git a/src/mgr/Filler.h b/src/mgr/Filler.h index 26b5f01e15..f9f3d3ea1a 100644 --- a/src/mgr/Filler.h +++ b/src/mgr/Filler.h @@ -12,6 +12,7 @@ #define SQUID_MGR_FILLER_H #include "comm/forward.h" +#include "ipc/forward.h" #include "mgr/Action.h" #include "mgr/StoreToCommWriter.h" @@ -24,7 +25,7 @@ class Filler: public StoreToCommWriter CBDATA_CLASS(Filler); public: - Filler(const Action::Pointer &anAction, const Comm::ConnectionPointer &conn, unsigned int aRequestId); + Filler(const Action::Pointer &, const Comm::ConnectionPointer &, Ipc::RequestId); protected: /* AsyncJob API */ @@ -33,7 +34,7 @@ protected: private: Action::Pointer action; ///< action that will run() and sendResponse() - unsigned int requestId; ///< the ID of the Request we are responding to + Ipc::RequestId requestId; ///< the ID of the Request we are responding to }; } // namespace Mgr diff --git a/src/mgr/Forwarder.cc b/src/mgr/Forwarder.cc index 0486d01ce6..aefbae3c8e 100644 --- a/src/mgr/Forwarder.cc +++ b/src/mgr/Forwarder.cc @@ -28,7 +28,10 @@ CBDATA_NAMESPACED_CLASS_INIT(Mgr, Forwarder); Mgr::Forwarder::Forwarder(const Comm::ConnectionPointer &aConn, const ActionParams &aParams, HttpRequest* aRequest, StoreEntry* anEntry, const AccessLogEntryPointer &anAle): - Ipc::Forwarder(new Request(KidIdentifier, 0, aConn, aParams), 10), + // TODO: Add virtual Forwarder::makeRequest() to avoid prematurely creating + // this dummy request with a dummy ID that are finalized by Ipc::Forwarder. + // Same for Snmp::Forwarder. + Ipc::Forwarder(new Request(KidIdentifier, Ipc::RequestId(/*XXX*/), aConn, aParams), 10), httpRequest(aRequest), entry(anEntry), conn(aConn), ale(anAle) { debugs(16, 5, HERE << conn); diff --git a/src/mgr/FunAction.cc b/src/mgr/FunAction.cc index ccba1e6e72..a4ca76dd7a 100644 --- a/src/mgr/FunAction.cc +++ b/src/mgr/FunAction.cc @@ -12,6 +12,7 @@ #include "base/TextException.h" #include "comm/Connection.h" #include "globals.h" +#include "ipc/RequestId.h" #include "ipc/UdsOp.h" #include "mgr/Command.h" #include "mgr/Filler.h" diff --git a/src/mgr/InfoAction.cc b/src/mgr/InfoAction.cc index 49fe61d118..87968b4ea5 100644 --- a/src/mgr/InfoAction.cc +++ b/src/mgr/InfoAction.cc @@ -14,6 +14,7 @@ #include "globals.h" #include "HttpReply.h" #include "ipc/Messages.h" +#include "ipc/RequestId.h" #include "ipc/TypedMsgHdr.h" #include "ipc/UdsOp.h" #include "mgr/Filler.h" diff --git a/src/mgr/Request.cc b/src/mgr/Request.cc index 8b9dfe9be6..d1d5fc53cf 100644 --- a/src/mgr/Request.cc +++ b/src/mgr/Request.cc @@ -16,7 +16,9 @@ #include "mgr/ActionParams.h" #include "mgr/Request.h" -Mgr::Request::Request(int aRequestorId, unsigned int aRequestId, const Comm::ConnectionPointer &aConn, +Mgr::Request::Request(const int aRequestorId, + const Ipc::RequestId aRequestId, + const Comm::ConnectionPointer &aConn, const ActionParams &aParams): Ipc::Request(aRequestorId, aRequestId), conn(aConn), @@ -25,14 +27,7 @@ Mgr::Request::Request(int aRequestorId, unsigned int aRequestId, const Comm::Con Must(requestorId > 0); } -Mgr::Request::Request(const Request& request): - Ipc::Request(request.requestorId, request.requestId), - conn(request.conn), params(request.params) -{ -} - -Mgr::Request::Request(const Ipc::TypedMsgHdr& msg): - Ipc::Request(0, 0) +Mgr::Request::Request(const Ipc::TypedMsgHdr &msg) { msg.checkType(Ipc::mtCacheMgrRequest); msg.getPod(requestorId); diff --git a/src/mgr/Request.h b/src/mgr/Request.h index fce310cc36..9399e37232 100644 --- a/src/mgr/Request.h +++ b/src/mgr/Request.h @@ -22,7 +22,7 @@ namespace Mgr class Request: public Ipc::Request { public: - Request(int aRequestorId, unsigned int aRequestId, const Comm::ConnectionPointer &aConn, + Request(int aRequestorId, Ipc::RequestId, const Comm::ConnectionPointer &aConn, const ActionParams &aParams); explicit Request(const Ipc::TypedMsgHdr& msg); ///< from recvmsg() @@ -30,9 +30,6 @@ public: virtual void pack(Ipc::TypedMsgHdr& msg) const; virtual Pointer clone() const; -private: - Request(const Request& request); - public: Comm::ConnectionPointer conn; ///< HTTP client connection descriptor diff --git a/src/mgr/Response.cc b/src/mgr/Response.cc index 8d40a37d70..37cab587a5 100644 --- a/src/mgr/Response.cc +++ b/src/mgr/Response.cc @@ -12,24 +12,19 @@ #include "base/TextException.h" #include "CacheManager.h" #include "ipc/Messages.h" +#include "ipc/RequestId.h" #include "ipc/TypedMsgHdr.h" #include "mgr/ActionCreator.h" #include "mgr/ActionProfile.h" #include "mgr/Response.h" -Mgr::Response::Response(unsigned int aRequestId, Action::Pointer anAction): +Mgr::Response::Response(const Ipc::RequestId aRequestId, const Action::Pointer anAction): Ipc::Response(aRequestId), action(anAction) { Must(!action || action->name()); // if there is an action, it must be named } -Mgr::Response::Response(const Response& response): - Ipc::Response(response.requestId), action(response.action) -{ -} - -Mgr::Response::Response(const Ipc::TypedMsgHdr& msg): - Ipc::Response(0) +Mgr::Response::Response(const Ipc::TypedMsgHdr &msg) { msg.checkType(Ipc::mtCacheMgrResponse); msg.getPod(requestId); diff --git a/src/mgr/Response.h b/src/mgr/Response.h index e95a6d5b45..13e4c189bc 100644 --- a/src/mgr/Response.h +++ b/src/mgr/Response.h @@ -23,7 +23,8 @@ namespace Mgr class Response: public Ipc::Response { public: - Response(unsigned int aRequestId, Action::Pointer anAction = NULL); + /// sender's constructor + Response(Ipc::RequestId, Action::Pointer anAction = nullptr); explicit Response(const Ipc::TypedMsgHdr& msg); ///< from recvmsg() @@ -34,9 +35,6 @@ public: bool hasAction() const; ///< whether response contain action object const Action& getAction() const; ///< returns action object -private: - Response(const Response& response); - public: Action::Pointer action; ///< action relating to response }; diff --git a/src/snmp/Forwarder.cc b/src/snmp/Forwarder.cc index 51b2932272..6fd3bd8a06 100644 --- a/src/snmp/Forwarder.cc +++ b/src/snmp/Forwarder.cc @@ -23,7 +23,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Snmp, Forwarder); Snmp::Forwarder::Forwarder(const Pdu& aPdu, const Session& aSession, int aFd, const Ip::Address& anAddress): - Ipc::Forwarder(new Request(KidIdentifier, 0, aPdu, aSession, aFd, anAddress), 2), + Ipc::Forwarder(new Request(KidIdentifier, Ipc::RequestId(), aPdu, aSession, aFd, anAddress), 2), fd(aFd) { debugs(49, 5, HERE << "FD " << aFd); @@ -88,7 +88,7 @@ Snmp::Forwarder::sendError(int error) } void -Snmp::SendResponse(unsigned int requestId, const Pdu& pdu) +Snmp::SendResponse(const Ipc::RequestId requestId, const Pdu &pdu) { debugs(49, 5, HERE); // snmpAgentResponse() can modify arg diff --git a/src/snmp/Forwarder.h b/src/snmp/Forwarder.h index a8c7560af6..0eecd6e1ac 100644 --- a/src/snmp/Forwarder.h +++ b/src/snmp/Forwarder.h @@ -47,7 +47,7 @@ private: AsyncCall::Pointer closer; ///< comm_close handler for the connection }; -void SendResponse(unsigned int requestId, const Pdu& pdu); +void SendResponse(Ipc::RequestId, const Pdu &); } // namespace Snmp diff --git a/src/snmp/Request.cc b/src/snmp/Request.cc index e5247a971e..750e491cf8 100644 --- a/src/snmp/Request.cc +++ b/src/snmp/Request.cc @@ -13,7 +13,7 @@ #include "ipc/TypedMsgHdr.h" #include "snmp/Request.h" -Snmp::Request::Request(int aRequestorId, unsigned int aRequestId, +Snmp::Request::Request(const int aRequestorId, const Ipc::RequestId aRequestId, const Pdu& aPdu, const Session& aSession, int aFd, const Ip::Address& anAddress): Ipc::Request(aRequestorId, aRequestId), @@ -21,15 +21,7 @@ Snmp::Request::Request(int aRequestorId, unsigned int aRequestId, { } -Snmp::Request::Request(const Request& request): - Ipc::Request(request.requestorId, request.requestId), - pdu(request.pdu), session(request.session), - fd(request.fd), address(request.address) -{ -} - -Snmp::Request::Request(const Ipc::TypedMsgHdr& msg): - Ipc::Request(0, 0) +Snmp::Request::Request(const Ipc::TypedMsgHdr &msg) { msg.checkType(Ipc::mtSnmpRequest); msg.getPod(requestorId); diff --git a/src/snmp/Request.h b/src/snmp/Request.h index 739dda6819..df9adbe605 100644 --- a/src/snmp/Request.h +++ b/src/snmp/Request.h @@ -24,7 +24,7 @@ namespace Snmp class Request: public Ipc::Request { public: - Request(int aRequestorId, unsigned int aRequestId, const Pdu& aPdu, + Request(int aRequestorId, Ipc::RequestId aRequestId, const Pdu& aPdu, const Session& aSession, int aFd, const Ip::Address& anAddress); explicit Request(const Ipc::TypedMsgHdr& msg); ///< from recvmsg() @@ -32,9 +32,6 @@ public: virtual void pack(Ipc::TypedMsgHdr& msg) const; virtual Pointer clone() const; -private: - Request(const Request& request); - public: Pdu pdu; ///< SNMP protocol data unit Session session; ///< SNMP session diff --git a/src/snmp/Response.cc b/src/snmp/Response.cc index 65495d45c8..c7f522945b 100644 --- a/src/snmp/Response.cc +++ b/src/snmp/Response.cc @@ -11,27 +11,16 @@ #include "squid.h" #include "base/TextException.h" #include "ipc/Messages.h" +#include "ipc/RequestId.h" #include "ipc/TypedMsgHdr.h" #include "snmp/Response.h" -std::ostream& Snmp::operator << (std::ostream& os, const Response& response) -{ - os << "response: {requestId: " << response.requestId << '}'; - return os; -} - -Snmp::Response::Response(unsigned int aRequestId): +Snmp::Response::Response(const Ipc::RequestId aRequestId): Ipc::Response(aRequestId), pdu() { } -Snmp::Response::Response(const Response& response): - Ipc::Response(response.requestId), pdu(response.pdu) -{ -} - -Snmp::Response::Response(const Ipc::TypedMsgHdr& msg): - Ipc::Response(0) +Snmp::Response::Response(const Ipc::TypedMsgHdr &msg) { msg.checkType(Ipc::mtSnmpResponse); msg.getPod(requestId); diff --git a/src/snmp/Response.h b/src/snmp/Response.h index 7f787011c8..0845a2d17b 100644 --- a/src/snmp/Response.h +++ b/src/snmp/Response.h @@ -23,21 +23,16 @@ namespace Snmp class Response: public Ipc::Response { public: - Response(unsigned int aRequestId); + explicit Response(Ipc::RequestId); ///< sender's constructor explicit Response(const Ipc::TypedMsgHdr& msg); ///< from recvmsg() /* Ipc::Response API */ virtual void pack(Ipc::TypedMsgHdr& msg) const; virtual Ipc::Response::Pointer clone() const; -private: - Response(const Response& response); - public: Pdu pdu; ///< SNMP protocol data unit }; -std::ostream& operator << (std::ostream& os, const Response& response); - } // namespace Snmp #endif /* SQUID_SNMPX_RESPONSE_H */ diff --git a/src/tests/stub_libmgr.cc b/src/tests/stub_libmgr.cc index 742da1ea54..b2a1335b78 100644 --- a/src/tests/stub_libmgr.cc +++ b/src/tests/stub_libmgr.cc @@ -12,6 +12,8 @@ #define STUB_API "lmgr/libmgr.la" #include "tests/STUB.h" +#include "ipc/RequestId.h" + // NP: used by Command.h instantiations #include "mgr/ActionProfile.h" @@ -26,7 +28,7 @@ void Mgr::Action::run(StoreEntry *entry, bool writeHttpHeader) STUB void Mgr::Action::fillEntry(StoreEntry *entry, bool writeHttpHeader) STUB void Mgr::Action::add(const Action &action) STUB void Mgr::Action::respond(const Request &request) STUB -void Mgr::Action::sendResponse(unsigned int requestId) STUB +void Mgr::Action::sendResponse(const Ipc::RequestId) STUB bool Mgr::Action::atomic() const STUB_RETVAL(false) const char * Mgr::Action::name() const STUB_RETVAL(NULL) static Mgr::Command static_Command; -- 2.47.2