From d468535df3c20f2bbe47a5deae7935cdfa61acba Mon Sep 17 00:00:00 2001 From: elexis Date: Tue, 17 Sep 2019 14:18:46 +0000 Subject: [PATCH] Split NetFileTransfer message parsing into one function per case, refs #5212. Avoid redundant map lookups by caching the iterator. Remove c-style casts by using type specific printf parameters, and static_cast elsewhere. Use references while at it, and typedef -> using. Differential Revision: https://code.wildfiregames.com/D1567 Comments by: Vladislav Tested on: clang 8.0.1, gcc 9.1.0 This was SVN commit r22916. --- source/network/NetClient.cpp | 2 +- source/network/NetFileTransfer.cpp | 200 +++++++++++++++-------------- source/network/NetFileTransfer.h | 15 ++- source/network/NetServer.cpp | 2 +- 4 files changed, 119 insertions(+), 100 deletions(-) diff --git a/source/network/NetClient.cpp b/source/network/NetClient.cpp index 118c356984..6dbe42eb88 100644 --- a/source/network/NetClient.cpp +++ b/source/network/NetClient.cpp @@ -391,7 +391,7 @@ bool CNetClient::HandleMessage(CNetMessage* message) { // Handle non-FSM messages first - Status status = m_Session->GetFileTransferer().HandleMessageReceive(message); + Status status = m_Session->GetFileTransferer().HandleMessageReceive(*message); if (status == INFO::OK) return true; if (status != INFO::SKIPPED) diff --git a/source/network/NetFileTransfer.cpp b/source/network/NetFileTransfer.cpp index f39a131661..8591166b1a 100644 --- a/source/network/NetFileTransfer.cpp +++ b/source/network/NetFileTransfer.cpp @@ -1,4 +1,4 @@ -/* Copyright (C) 2016 Wildfire Games. +/* Copyright (C) 2019 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -24,107 +24,119 @@ #include "network/NetSession.h" #include "ps/CLogger.h" -Status CNetFileTransferer::HandleMessageReceive(const CNetMessage* message) +Status CNetFileTransferer::HandleMessageReceive(const CNetMessage& message) { - if (message->GetType() == NMT_FILE_TRANSFER_RESPONSE) + switch (message.GetType()) { - CFileTransferResponseMessage* respMessage = (CFileTransferResponseMessage*)message; + case NMT_FILE_TRANSFER_RESPONSE: + return OnFileTransferResponse(static_cast(message)); - if (m_FileReceiveTasks.find(respMessage->m_RequestID) == m_FileReceiveTasks.end()) - { - LOGERROR("Net transfer: Unsolicited file transfer response (id=%d)", (int)respMessage->m_RequestID); - return ERR::FAIL; - } + case NMT_FILE_TRANSFER_DATA: + return OnFileTransferData(static_cast(message)); - if (respMessage->m_Length == 0 || respMessage->m_Length > MAX_FILE_TRANSFER_SIZE) - { - LOGERROR("Net transfer: Invalid size for file transfer response (length=%d)", (int)respMessage->m_Length); - return ERR::FAIL; - } + case NMT_FILE_TRANSFER_ACK: + return OnFileTransferAck(static_cast(message)); - shared_ptr task = m_FileReceiveTasks[respMessage->m_RequestID]; - - task->m_Length = respMessage->m_Length; - task->m_Buffer.reserve(respMessage->m_Length); - - LOGMESSAGERENDER("Downloading data over network (%d KB) - please wait...", (int)(task->m_Length/1024)); - m_LastProgressReportTime = timer_Time(); - - return INFO::OK; + default: + return INFO::SKIPPED; } - else if (message->GetType() == NMT_FILE_TRANSFER_DATA) - { - CFileTransferDataMessage* dataMessage = (CFileTransferDataMessage*)message; - - if (m_FileReceiveTasks.find(dataMessage->m_RequestID) == m_FileReceiveTasks.end()) - { - LOGERROR("Net transfer: Unsolicited file transfer data (id=%d)", (int)dataMessage->m_RequestID); - return ERR::FAIL; - } - - shared_ptr task = m_FileReceiveTasks[dataMessage->m_RequestID]; - - task->m_Buffer += dataMessage->m_Data; - - if (task->m_Buffer.size() > task->m_Length) - { - LOGERROR("Net transfer: Invalid size for file transfer data (length=%d actual=%d)", (int)task->m_Length, (int)task->m_Buffer.size()); - return ERR::FAIL; - } - - CFileTransferAckMessage ackMessage; - ackMessage.m_RequestID = task->m_RequestID; - ackMessage.m_NumPackets = 1; // TODO: would be nice to send a single ack for multiple packets at once - m_Session->SendMessage(&ackMessage); - - if (task->m_Buffer.size() == task->m_Length) - { - LOGMESSAGERENDER("Download completed"); - - task->OnComplete(); - m_FileReceiveTasks.erase(dataMessage->m_RequestID); - return INFO::OK; - } - - // TODO: should report progress using proper GUI - - // Report the download status occassionally - double t = timer_Time(); - if (t > m_LastProgressReportTime + 0.5) - { - LOGMESSAGERENDER("Downloading data: %.1f%% of %d KB", 100.f*task->m_Buffer.size()/task->m_Length, (int)(task->m_Length/1024)); - m_LastProgressReportTime = t; - } - - return INFO::OK; - } - else if (message->GetType() == NMT_FILE_TRANSFER_ACK) - { - CFileTransferAckMessage* ackMessage = (CFileTransferAckMessage*)message; - - if (m_FileSendTasks.find(ackMessage->m_RequestID) == m_FileSendTasks.end()) - { - LOGERROR("Net transfer: Unsolicited file transfer ack (id=%d)", (int)ackMessage->m_RequestID); - return ERR::FAIL; - } - - CNetFileSendTask& task = m_FileSendTasks[ackMessage->m_RequestID]; - - if (ackMessage->m_NumPackets > task.packetsInFlight) - { - LOGERROR("Net transfer: Invalid num packets for file transfer ack (num=%d inflight=%d)", - (int)ackMessage->m_NumPackets, (int)task.packetsInFlight); - return ERR::FAIL; - } - - task.packetsInFlight -= ackMessage->m_NumPackets; - - return INFO::OK; - } - - return INFO::SKIPPED; } +Status CNetFileTransferer::OnFileTransferResponse(const CFileTransferResponseMessage& message) +{ + const FileReceiveTasksMap::iterator it = m_FileReceiveTasks.find(message.m_RequestID); + if (it == m_FileReceiveTasks.end()) + { + LOGERROR("Net transfer: Unsolicited file transfer response (id=%lu)", message.m_RequestID); + return ERR::FAIL; + } + + if (message.m_Length == 0 || message.m_Length > MAX_FILE_TRANSFER_SIZE) + { + LOGERROR("Net transfer: Invalid size for file transfer response (length=%lu)", message.m_Length); + return ERR::FAIL; + } + + CNetFileReceiveTask& task = *it->second; + + task.m_Length = message.m_Length; + task.m_Buffer.reserve(message.m_Length); + + LOGMESSAGERENDER("Downloading data over network (%lu KB) - please wait...", task.m_Length / 1024); + m_LastProgressReportTime = timer_Time(); + + return INFO::OK; +} + +Status CNetFileTransferer::OnFileTransferData(const CFileTransferDataMessage& message) +{ + FileReceiveTasksMap::iterator it = m_FileReceiveTasks.find(message.m_RequestID); + if (it == m_FileReceiveTasks.end()) + { + LOGERROR("Net transfer: Unsolicited file transfer data (id=%lu)", message.m_RequestID); + return ERR::FAIL; + } + + CNetFileReceiveTask& task = *it->second; + + task.m_Buffer += message.m_Data; + + if (task.m_Buffer.size() > task.m_Length) + { + LOGERROR("Net transfer: Invalid size for file transfer data (length=%lu actual=%zu)", task.m_Length, task.m_Buffer.size()); + return ERR::FAIL; + } + + CFileTransferAckMessage ackMessage; + ackMessage.m_RequestID = task.m_RequestID; + ackMessage.m_NumPackets = 1; // TODO: would be nice to send a single ack for multiple packets at once + m_Session->SendMessage(&ackMessage); + + if (task.m_Buffer.size() == task.m_Length) + { + LOGMESSAGERENDER("Download completed"); + + task.OnComplete(); + m_FileReceiveTasks.erase(message.m_RequestID); + return INFO::OK; + } + + // TODO: should report progress using proper GUI + + // Report the download status occassionally + double t = timer_Time(); + if (t > m_LastProgressReportTime + 0.5) + { + LOGMESSAGERENDER("Downloading data: %.1f%% of %lu KB", 100.f * task.m_Buffer.size() / task.m_Length, task.m_Length / 1024); + m_LastProgressReportTime = t; + } + + return INFO::OK; +} + +Status CNetFileTransferer::OnFileTransferAck(const CFileTransferAckMessage& message) +{ + FileSendTasksMap::iterator it = m_FileSendTasks.find(message.m_RequestID); + if (it == m_FileSendTasks.end()) + { + LOGERROR("Net transfer: Unsolicited file transfer ack (id=%lu)", message.m_RequestID); + return ERR::FAIL; + } + + CNetFileSendTask& task = it->second; + + if (message.m_NumPackets > task.packetsInFlight) + { + LOGERROR("Net transfer: Invalid num packets for file transfer ack (num=%lu inflight=%lu)", + message.m_NumPackets, task.packetsInFlight); + return ERR::FAIL; + } + + task.packetsInFlight -= message.m_NumPackets; + + return INFO::OK; + +} void CNetFileTransferer::StartTask(const shared_ptr& task) { diff --git a/source/network/NetFileTransfer.h b/source/network/NetFileTransfer.h index 9b803816ad..3c8444d477 100644 --- a/source/network/NetFileTransfer.h +++ b/source/network/NetFileTransfer.h @@ -1,4 +1,4 @@ -/* Copyright (C) 2016 Wildfire Games. +/* Copyright (C) 2019 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -21,6 +21,9 @@ #include class CNetMessage; +class CFileTransferResponseMessage; +class CFileTransferDataMessage; +class CFileTransferAckMessage; class CNetClientSession; class CNetServerSession; class INetSession; @@ -84,7 +87,7 @@ public: * Returns INFO::OK if the message is handled successfully, * or ERR::FAIL if handled unsuccessfully. */ - Status HandleMessageReceive(const CNetMessage* message); + Status HandleMessageReceive(const CNetMessage& message); /** * Registers a file-receiving task. @@ -105,6 +108,10 @@ public: void Poll(); private: + Status OnFileTransferResponse(const CFileTransferResponseMessage& message); + Status OnFileTransferData(const CFileTransferDataMessage& message); + Status OnFileTransferAck(const CFileTransferAckMessage& message); + /** * Asynchronous file-sending task. */ @@ -121,10 +128,10 @@ private: u32 m_NextRequestID; - typedef std::map> FileReceiveTasksMap; + using FileReceiveTasksMap = std::map >; FileReceiveTasksMap m_FileReceiveTasks; - typedef std::map FileSendTasksMap; + using FileSendTasksMap = std::map; FileSendTasksMap m_FileSendTasks; double m_LastProgressReportTime; diff --git a/source/network/NetServer.cpp b/source/network/NetServer.cpp index 3aa50b770d..c4943bf0b3 100644 --- a/source/network/NetServer.cpp +++ b/source/network/NetServer.cpp @@ -621,7 +621,7 @@ void CNetServerWorker::CheckClientConnections() void CNetServerWorker::HandleMessageReceive(const CNetMessage* message, CNetServerSession* session) { // Handle non-FSM messages first - Status status = session->GetFileTransferer().HandleMessageReceive(message); + Status status = session->GetFileTransferer().HandleMessageReceive(*message); if (status != INFO::SKIPPED) return;