From 612384b3d46d06eea6fd71ee6dc60471d0f9e3d1 Mon Sep 17 00:00:00 2001 From: Attila Molnar Date: Sun, 12 Apr 2015 16:20:13 +0200 Subject: [PATCH] Dispatch EventHandler events to dedicated virtual functions Remove enum EventType --- include/inspsocket.h | 25 ++++-- include/socket.h | 7 +- include/socketengine.h | 39 +++------ src/coremods/core_dns.cpp | 11 ++- src/inspsocket.cpp | 98 ++++++++++++---------- src/listensocket.cpp | 21 +---- src/modules/extra/m_pgsql.cpp | 21 ++--- src/modules/m_ident.cpp | 35 ++------ src/socketengine.cpp | 12 ++- src/socketengines/socketengine_epoll.cpp | 8 +- src/socketengines/socketengine_kqueue.cpp | 6 +- src/socketengines/socketengine_poll.cpp | 8 +- src/socketengines/socketengine_ports.cpp | 4 +- src/socketengines/socketengine_select.cpp | 6 +- src/threadengines/threadengine_pthread.cpp | 50 ++++++----- 15 files changed, 169 insertions(+), 182 deletions(-) diff --git a/include/inspsocket.h b/include/inspsocket.h index 1bcfbea09..8c93f884e 100644 --- a/include/inspsocket.h +++ b/include/inspsocket.h @@ -113,6 +113,12 @@ class CoreExport StreamSocket : public EventHandler size_t sendq_len; /** Error - if nonempty, the socket is dead, and this is the reason. */ std::string error; + + /** Check if the socket has an error set, if yes, call OnError + * @param err Error to pass to OnError() + */ + void CheckError(BufferedSocketError err); + protected: std::string recvq; public: @@ -120,15 +126,24 @@ class CoreExport StreamSocket : public EventHandler IOHook* GetIOHook() const; void AddIOHook(IOHook* hook); void DelIOHook(); - /** Handle event from socket engine. - * This will call OnDataReady if there is *new* data in recvq - */ - virtual void HandleEvent(EventType et, int errornum = 0); /** Dispatched from HandleEvent */ virtual void DoRead(); /** Dispatched from HandleEvent */ virtual void DoWrite(); + /** Called by the socket engine on a read event + */ + void OnEventHandlerRead() CXX11_OVERRIDE; + + /** Called by the socket engine on a write event + */ + void OnEventHandlerWrite() CXX11_OVERRIDE; + + /** Called by the socket engine on error + * @param errcode Error + */ + void OnEventHandlerError(int errcode) CXX11_OVERRIDE; + /** Sets the error message for this socket. Once set, the socket is dead. */ void SetError(const std::string& err) { if (error.empty()) error = err; } @@ -226,7 +241,7 @@ class CoreExport BufferedSocket : public StreamSocket virtual ~BufferedSocket(); protected: - virtual void DoWrite(); + void OnEventHandlerWrite() CXX11_OVERRIDE; BufferedSocketError BeginConnect(const irc::sockets::sockaddrs& dest, const irc::sockets::sockaddrs& bind, unsigned long timeout); BufferedSocketError BeginConnect(const std::string &ipaddr, int aport, unsigned long maxtime, const std::string &connectbindip); }; diff --git a/include/socket.h b/include/socket.h index c292b7010..9d69b5d22 100644 --- a/include/socket.h +++ b/include/socket.h @@ -150,16 +150,13 @@ class CoreExport ListenSocket : public EventHandler /** Create a new listening socket */ ListenSocket(ConfigTag* tag, const irc::sockets::sockaddrs& bind_to); - /** Handle an I/O event - */ - void HandleEvent(EventType et, int errornum = 0); /** Close the socket */ ~ListenSocket(); - /** Handles sockets internals crap of a connection, convenience wrapper really + /** Handles new connections, called by the socket engine */ - void AcceptInternal(); + void OnEventHandlerRead() CXX11_OVERRIDE; /** Inspects the bind block belonging to this socket to set the name of the IO hook * provider which this socket will use for incoming connections. diff --git a/include/socketengine.h b/include/socketengine.h index f30289913..ddc48f94d 100644 --- a/include/socketengine.h +++ b/include/socketengine.h @@ -37,23 +37,6 @@ #define IOV_MAX 1024 #endif -/** Types of event an EventHandler may receive. - * EVENT_READ is a readable file descriptor, - * and EVENT_WRITE is a writeable file descriptor. - * EVENT_ERROR can always occur, and indicates - * a write error or read error on the socket, - * e.g. EOF condition or broken pipe. - */ -enum EventType -{ - /** Read event */ - EVENT_READ = 0, - /** Write event */ - EVENT_WRITE = 1, - /** Error event */ - EVENT_ERROR = 2 -}; - /** * Event mask for SocketEngine events */ @@ -207,16 +190,20 @@ class CoreExport EventHandler : public classbase */ virtual ~EventHandler() {} - /** Process an I/O event. - * You MUST implement this function in your derived - * class, and it will be called whenever read or write - * events are received. - * @param et either one of EVENT_READ for read events, - * EVENT_WRITE for write events and EVENT_ERROR for - * error events. - * @param errornum The error code which goes with an EVENT_ERROR. + /** Called by the socket engine in case of a read event + */ + virtual void OnEventHandlerRead() = 0; + + /** Called by the socket engine in case of a write event. + * The default implementation does nothing. + */ + virtual void OnEventHandlerWrite(); + + /** Called by the socket engine in case of an error event. + * The default implementation does nothing. + * @param errornum Error code */ - virtual void HandleEvent(EventType et, int errornum = 0) = 0; + virtual void OnEventHandlerError(int errornum); friend class SocketEngine; }; diff --git a/src/coremods/core_dns.cpp b/src/coremods/core_dns.cpp index 47d5b3cab..de8dedd4a 100644 --- a/src/coremods/core_dns.cpp +++ b/src/coremods/core_dns.cpp @@ -555,14 +555,13 @@ class MyManager : public Manager, public Timer, public EventHandler } } - void HandleEvent(EventType et, int) + void OnEventHandlerError(int errcode) CXX11_OVERRIDE { - if (et == EVENT_ERROR) - { - ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "UDP socket got an error event"); - return; - } + ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "UDP socket got an error event"); + } + void OnEventHandlerRead() CXX11_OVERRIDE + { unsigned char buffer[524]; irc::sockets::sockaddrs from; socklen_t x = sizeof(from); diff --git a/src/inspsocket.cpp b/src/inspsocket.cpp index 2646e265f..fbeb60122 100644 --- a/src/inspsocket.cpp +++ b/src/inspsocket.cpp @@ -417,7 +417,7 @@ bool SocketTimeout::Tick(time_t) void BufferedSocket::OnConnected() { } void BufferedSocket::OnTimeout() { return; } -void BufferedSocket::DoWrite() +void BufferedSocket::OnEventHandlerWrite() { if (state == I_CONNECTING) { @@ -426,7 +426,7 @@ void BufferedSocket::DoWrite() if (!GetIOHook()) SocketEngine::ChangeEventMask(this, FD_WANT_FAST_READ | FD_WANT_EDGE_WRITE); } - this->StreamSocket::DoWrite(); + this->StreamSocket::OnEventHandlerWrite(); } BufferedSocket::~BufferedSocket() @@ -436,57 +436,67 @@ BufferedSocket::~BufferedSocket() delete Timeout; } -void StreamSocket::HandleEvent(EventType et, int errornum) +void StreamSocket::OnEventHandlerError(int errornum) { if (!error.empty()) return; + + if (errornum == 0) + SetError("Connection closed"); + else + SetError(SocketEngine::GetError(errornum)); + BufferedSocketError errcode = I_ERR_OTHER; - try { - switch (et) - { - case EVENT_ERROR: - { - if (errornum == 0) - SetError("Connection closed"); - else - SetError(SocketEngine::GetError(errornum)); - switch (errornum) - { - case ETIMEDOUT: - errcode = I_ERR_TIMEOUT; - break; - case ECONNREFUSED: - case 0: - errcode = I_ERR_CONNECT; - break; - case EADDRINUSE: - errcode = I_ERR_BIND; - break; - case EPIPE: - case EIO: - errcode = I_ERR_WRITE; - break; - } - break; - } - case EVENT_READ: - { - DoRead(); - break; - } - case EVENT_WRITE: - { - DoWrite(); - break; - } - } + switch (errornum) + { + case ETIMEDOUT: + errcode = I_ERR_TIMEOUT; + break; + case ECONNREFUSED: + case 0: + errcode = I_ERR_CONNECT; + break; + case EADDRINUSE: + errcode = I_ERR_BIND; + break; + case EPIPE: + case EIO: + errcode = I_ERR_WRITE; + break; + } + + // Log and call OnError() + CheckError(errcode); +} + +void StreamSocket::OnEventHandlerRead() +{ + if (!error.empty()) + return; + + try + { + DoRead(); } catch (CoreException& ex) { - ServerInstance->Logs->Log("SOCKET", LOG_DEFAULT, "Caught exception in socket processing on FD %d - '%s'", - fd, ex.GetReason().c_str()); + ServerInstance->Logs->Log("SOCKET", LOG_DEFAULT, "Caught exception in socket processing on FD %d - '%s'", fd, ex.GetReason().c_str()); SetError(ex.GetReason()); } + CheckError(I_ERR_OTHER); +} + +void StreamSocket::OnEventHandlerWrite() +{ + if (!error.empty()) + return; + + DoWrite(); + CheckError(I_ERR_OTHER); +} + +void StreamSocket::CheckError(BufferedSocketError errcode) +{ if (!error.empty()) { ServerInstance->Logs->Log("SOCKET", LOG_DEBUG, "Error on FD %d - '%s'", fd, error.c_str()); diff --git a/src/listensocket.cpp b/src/listensocket.cpp index c1339fb3d..fa43e6827 100644 --- a/src/listensocket.cpp +++ b/src/listensocket.cpp @@ -100,8 +100,7 @@ ListenSocket::~ListenSocket() } } -/* Just seperated into another func for tidiness really.. */ -void ListenSocket::AcceptInternal() +void ListenSocket::OnEventHandlerRead() { irc::sockets::sockaddrs client; irc::sockets::sockaddrs server; @@ -109,7 +108,7 @@ void ListenSocket::AcceptInternal() socklen_t length = sizeof(client); int incomingSockfd = SocketEngine::Accept(this, &client.sa, &length); - ServerInstance->Logs->Log("SOCKET", LOG_DEBUG, "HandleEvent for Listensocket %s nfd=%d", bind_desc.c_str(), incomingSockfd); + ServerInstance->Logs->Log("SOCKET", LOG_DEBUG, "Accepting connection on socket %s fd %d", bind_desc.c_str(), incomingSockfd); if (incomingSockfd < 0) { ServerInstance->stats.Refused++; @@ -179,22 +178,6 @@ void ListenSocket::AcceptInternal() } } -void ListenSocket::HandleEvent(EventType e, int err) -{ - switch (e) - { - case EVENT_ERROR: - ServerInstance->Logs->Log("SOCKET", LOG_DEFAULT, "ListenSocket::HandleEvent() received a socket engine error event! well shit! '%s'", strerror(err)); - break; - case EVENT_WRITE: - ServerInstance->Logs->Log("SOCKET", LOG_DEBUG, "*** BUG *** ListenSocket::HandleEvent() got a WRITE event!!!"); - break; - case EVENT_READ: - this->AcceptInternal(); - break; - } -} - bool ListenSocket::ResetIOHookProvider() { std::string provname = bind_tag->getString("ssl"); diff --git a/src/modules/extra/m_pgsql.cpp b/src/modules/extra/m_pgsql.cpp index 1e73c0143..ff8c1174c 100644 --- a/src/modules/extra/m_pgsql.cpp +++ b/src/modules/extra/m_pgsql.cpp @@ -178,18 +178,19 @@ class SQLConn : public SQLProvider, public EventHandler } } - void HandleEvent(EventType et, int errornum) + void OnEventHandlerRead() CXX11_OVERRIDE { - switch (et) - { - case EVENT_READ: - case EVENT_WRITE: - DoEvent(); - break; + DoEvent(); + } - case EVENT_ERROR: - DelayReconnect(); - } + void OnEventHandlerWrite() CXX11_OVERRIDE + { + DoEvent(); + } + + void OnEventHandlerError(int errornum) CXX11_OVERRIDE + { + DelayReconnect(); } std::string GetDSN() diff --git a/src/modules/m_ident.cpp b/src/modules/m_ident.cpp index 959e58a47..0e5aa43ae 100644 --- a/src/modules/m_ident.cpp +++ b/src/modules/m_ident.cpp @@ -140,9 +140,8 @@ class IdentRequestSocket : public EventHandler } } - void OnConnected() + void OnEventHandlerWrite() CXX11_OVERRIDE { - ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "OnConnected()"); SocketEngine::ChangeEventMask(this, FD_WANT_POLL_READ | FD_WANT_NO_WRITE); char req[32]; @@ -163,30 +162,6 @@ class IdentRequestSocket : public EventHandler done = true; } - void HandleEvent(EventType et, int errornum = 0) - { - switch (et) - { - case EVENT_READ: - /* fd readable event, received ident response */ - ReadResponse(); - break; - case EVENT_WRITE: - /* fd writeable event, successfully connected! */ - OnConnected(); - break; - case EVENT_ERROR: - /* fd error event, ohshi- */ - ServerInstance->Logs->Log(MODNAME, LOG_DEBUG, "EVENT_ERROR"); - /* We *must* Close() here immediately or we get a - * huge storm of EVENT_ERROR events! - */ - Close(); - done = true; - break; - } - } - void Close() { /* Remove ident socket from engine, and close it, but dont detatch it @@ -204,7 +179,7 @@ class IdentRequestSocket : public EventHandler return done; } - void ReadResponse() + void OnEventHandlerRead() CXX11_OVERRIDE { /* We don't really need to buffer for incomplete replies here, since IDENT replies are * extremely short - there is *no* sane reason it'd be in more than one packet @@ -264,6 +239,12 @@ class IdentRequestSocket : public EventHandler } } + void OnEventHandlerError(int errornum) CXX11_OVERRIDE + { + Close(); + done = true; + } + CullResult cull() CXX11_OVERRIDE { Close(); diff --git a/src/socketengine.cpp b/src/socketengine.cpp index eadfc73d3..4183488b7 100644 --- a/src/socketengine.cpp +++ b/src/socketengine.cpp @@ -53,6 +53,14 @@ void EventHandler::SetFd(int FD) this->fd = FD; } +void EventHandler::OnEventHandlerWrite() +{ +} + +void EventHandler::OnEventHandlerError(int errornum) +{ +} + void SocketEngine::ChangeEventMask(EventHandler* eh, int change) { int old_m = eh->event_mask; @@ -91,9 +99,9 @@ void SocketEngine::DispatchTrialWrites() int mask = eh->event_mask; eh->event_mask &= ~(FD_ADD_TRIAL_READ | FD_ADD_TRIAL_WRITE); if ((mask & (FD_ADD_TRIAL_READ | FD_READ_WILL_BLOCK)) == FD_ADD_TRIAL_READ) - eh->HandleEvent(EVENT_READ, 0); + eh->OnEventHandlerRead(); if ((mask & (FD_ADD_TRIAL_WRITE | FD_WRITE_WILL_BLOCK)) == FD_ADD_TRIAL_WRITE) - eh->HandleEvent(EVENT_WRITE, 0); + eh->OnEventHandlerWrite(); } } diff --git a/src/socketengines/socketengine_epoll.cpp b/src/socketengines/socketengine_epoll.cpp index 29404f416..8548e0824 100644 --- a/src/socketengines/socketengine_epoll.cpp +++ b/src/socketengines/socketengine_epoll.cpp @@ -182,7 +182,7 @@ int SocketEngine::DispatchEvents() if (ev.events & EPOLLHUP) { stats.ErrorEvents++; - eh->HandleEvent(EVENT_ERROR, 0); + eh->OnEventHandlerError(0); continue; } @@ -194,7 +194,7 @@ int SocketEngine::DispatchEvents() int errcode; if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &errcode, &codesize) < 0) errcode = errno; - eh->HandleEvent(EVENT_ERROR, errcode); + eh->OnEventHandlerError(errcode); continue; } @@ -215,7 +215,7 @@ int SocketEngine::DispatchEvents() if (ev.events & EPOLLIN) { stats.ReadEvents++; - eh->HandleEvent(EVENT_READ); + eh->OnEventHandlerRead(); if (eh != GetRef(fd)) // whoa! we got deleted, better not give out the write event continue; @@ -223,7 +223,7 @@ int SocketEngine::DispatchEvents() if (ev.events & EPOLLOUT) { stats.WriteEvents++; - eh->HandleEvent(EVENT_WRITE); + eh->OnEventHandlerWrite(); } } diff --git a/src/socketengines/socketengine_kqueue.cpp b/src/socketengines/socketengine_kqueue.cpp index 8cc8a33f9..922cb7f2d 100644 --- a/src/socketengines/socketengine_kqueue.cpp +++ b/src/socketengines/socketengine_kqueue.cpp @@ -194,7 +194,7 @@ int SocketEngine::DispatchEvents() if (kev.flags & EV_EOF) { stats.ErrorEvents++; - eh->HandleEvent(EVENT_ERROR, kev.fflags); + eh->OnEventHandlerError(kev.fflags); continue; } if (filter == EVFILT_WRITE) @@ -206,13 +206,13 @@ int SocketEngine::DispatchEvents() */ const int bits_to_clr = FD_WANT_SINGLE_WRITE | FD_WANT_FAST_WRITE | FD_WRITE_WILL_BLOCK; eh->SetEventMask(eh->GetEventMask() & ~bits_to_clr); - eh->HandleEvent(EVENT_WRITE); + eh->OnEventHandlerWrite(); } else if (filter == EVFILT_READ) { stats.ReadEvents++; eh->SetEventMask(eh->GetEventMask() & ~FD_READ_WILL_BLOCK); - eh->HandleEvent(EVENT_READ); + eh->OnEventHandlerRead(); } } diff --git a/src/socketengines/socketengine_poll.cpp b/src/socketengines/socketengine_poll.cpp index dc63fe0ed..5fd7e6235 100644 --- a/src/socketengines/socketengine_poll.cpp +++ b/src/socketengines/socketengine_poll.cpp @@ -185,7 +185,7 @@ int SocketEngine::DispatchEvents() if (revents & POLLHUP) { - eh->HandleEvent(EVENT_ERROR, 0); + eh->OnEventHandlerError(0); continue; } @@ -196,14 +196,14 @@ int SocketEngine::DispatchEvents() int errcode; if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &errcode, &codesize) < 0) errcode = errno; - eh->HandleEvent(EVENT_ERROR, errcode); + eh->OnEventHandlerError(errcode); continue; } if (revents & POLLIN) { eh->SetEventMask(eh->GetEventMask() & ~FD_READ_WILL_BLOCK); - eh->HandleEvent(EVENT_READ); + eh->OnEventHandlerRead(); if (eh != GetRef(fd)) // whoops, deleted out from under us continue; @@ -217,7 +217,7 @@ int SocketEngine::DispatchEvents() // The vector could've been resized, reference can be invalid by now; don't use it events[index].events = mask_to_poll(mask); - eh->HandleEvent(EVENT_WRITE); + eh->eh->OnEventHandlerWrite(); } } diff --git a/src/socketengines/socketengine_ports.cpp b/src/socketengines/socketengine_ports.cpp index 0d77954e4..d94d02664 100644 --- a/src/socketengines/socketengine_ports.cpp +++ b/src/socketengines/socketengine_ports.cpp @@ -160,14 +160,14 @@ int SocketEngine::DispatchEvents() if (portev_events & POLLRDNORM) { stats.ReadEvents++; - eh->HandleEvent(EVENT_READ); + eh->OnEventHandlerRead(); if (eh != GetRef(fd)) continue; } if (portev_events & POLLWRNORM) { stats.WriteEvents++; - eh->HandleEvent(EVENT_WRITE); + eh->OnEventHandlerWrite(); } } diff --git a/src/socketengines/socketengine_select.cpp b/src/socketengines/socketengine_select.cpp index f44346ad8..6dfbae88e 100644 --- a/src/socketengines/socketengine_select.cpp +++ b/src/socketengines/socketengine_select.cpp @@ -141,7 +141,7 @@ int SocketEngine::DispatchEvents() if (getsockopt(i, SOL_SOCKET, SO_ERROR, (char*)&errcode, &codesize) < 0) errcode = errno; - ev->HandleEvent(EVENT_ERROR, errcode); + ev->OnEventHandlerError(errcode); continue; } @@ -149,7 +149,7 @@ int SocketEngine::DispatchEvents() { stats.ReadEvents++; ev->SetEventMask(ev->GetEventMask() & ~FD_READ_WILL_BLOCK); - ev->HandleEvent(EVENT_READ); + ev->OnEventHandlerRead(); if (ev != GetRef(i)) continue; } @@ -160,7 +160,7 @@ int SocketEngine::DispatchEvents() int newmask = (ev->GetEventMask() & ~(FD_WRITE_WILL_BLOCK | FD_WANT_SINGLE_WRITE)); SocketEngine::OnSetEvent(ev, ev->GetEventMask(), newmask); ev->SetEventMask(newmask); - ev->HandleEvent(EVENT_WRITE); + ev->OnEventHandlerWrite(); } } diff --git a/src/threadengines/threadengine_pthread.cpp b/src/threadengines/threadengine_pthread.cpp index fcb4db444..6456d6df5 100644 --- a/src/threadengines/threadengine_pthread.cpp +++ b/src/threadengines/threadengine_pthread.cpp @@ -72,18 +72,21 @@ class ThreadSignalSocket : public EventHandler eventfd_write(fd, 1); } - void HandleEvent(EventType et, int errornum) + void OnEventHandlerRead() CXX11_OVERRIDE { - if (et == EVENT_READ) - { - eventfd_t dummy; - eventfd_read(fd, &dummy); - parent->OnNotify(); - } - else - { - ServerInstance->GlobalCulls.AddItem(this); - } + eventfd_t dummy; + eventfd_read(fd, &dummy); + parent->OnNotify(); + } + + void OnEventHandlerWrite() CXX11_OVERRIDE + { + ServerInstance->GlobalCulls.AddItem(this); + } + + void OnEventHandlerError(int errcode) CXX11_OVERRIDE + { + ThreadSignalSocket::OnEventHandlerWrite(); } }; @@ -122,18 +125,21 @@ class ThreadSignalSocket : public EventHandler write(send_fd, &dummy, 1); } - void HandleEvent(EventType et, int errornum) + void OnEventHandlerRead() CXX11_OVERRIDE + { + char dummy[128]; + read(fd, dummy, 128); + parent->OnNotify(); + } + + void OnEventHandlerWrite() CXX11_OVERRIDE + { + ServerInstance->GlobalCulls.AddItem(this); + } + + void OnEventHandlerError(int errcode) CXX11_OVERRIDE { - if (et == EVENT_READ) - { - char dummy[128]; - read(fd, dummy, 128); - parent->OnNotify(); - } - else - { - ServerInstance->GlobalCulls.AddItem(this); - } + ThreadSignalSocket::OnEventHandlerWrite(); } }; -- 2.39.5