From 43ff3a29d5d5b6f6a747a58914447087da4ca525 Mon Sep 17 00:00:00 2001 From: Karol Grydziuszko Date: Tue, 23 Nov 2021 10:49:00 +0100 Subject: [PATCH 1/2] Added better memory management and encapsulation in LogCollector and part of socket servers. Signed-off-by: Karol Grydziuszko Change-Id: I646283cd1515afbb1177ce5f8a04888a1bbd2add --- frameworks/native/dgram_socket_server.cpp | 32 ++--- .../native/hilog_input_socket_server.cpp | 58 +++++++--- .../native/include/dgram_socket_server.h | 10 +- .../include/hilog_input_socket_server.h | 40 +++++-- frameworks/native/include/socket_server.h | 6 +- frameworks/native/socket_server.cpp | 12 +- services/hilogd/include/log_collector.h | 11 +- services/hilogd/log_collector.cpp | 109 ++++++++---------- services/hilogd/main.cpp | 19 ++- 9 files changed, 161 insertions(+), 136 deletions(-) diff --git a/frameworks/native/dgram_socket_server.cpp b/frameworks/native/dgram_socket_server.cpp index 04db0d6..67fa6d3 100644 --- a/frameworks/native/dgram_socket_server.cpp +++ b/frameworks/native/dgram_socket_server.cpp @@ -20,33 +20,25 @@ namespace OHOS { namespace HiviewDFX { -int DgramSocketServer::RecvPacket(char **data, int *length, struct ucred *cred) +int DgramSocketServer::RecvPacket(std::vector& buffer, struct ucred *cred) { - int ret = 0; uint16_t packetLen = 0; - - ret = Recv(&packetLen, sizeof(packetLen)); - if (ret < 0) { - return ret; + if (auto status = Recv(&packetLen, sizeof(packetLen)); status < 0) { + return status; } - if (packetLen > maxLength) { - Recv(&packetLen, sizeof(packetLen), 0); - return 0; - } - *length = packetLen; - *data = new (std::nothrow) char[packetLen + 1]; - - if (*data == nullptr) { - Recv(&packetLen, sizeof(packetLen), 0); + if (packetLen > maxPacketLength) { + Recv(&packetLen, sizeof(packetLen), 0); // skip too long packet return 0; } + buffer.resize(packetLen + 1); std::array control = {0}; struct msghdr msgh = {0}; + int ret = 0; if (cred != nullptr) { struct iovec iov; - iov.iov_base = *data; + iov.iov_base = buffer.data(); iov.iov_len = packetLen; msgh.msg_iov = &iov; msgh.msg_iovlen = 1; @@ -60,24 +52,20 @@ int DgramSocketServer::RecvPacket(char **data, int *length, struct ucred *cred) ret = RecvMsg(&msgh); } else { - ret = Recv(*data, packetLen, 0); + ret = Recv(buffer.data(), packetLen, 0); } if (ret <= 0) { - delete [] *data; - *data = nullptr; return ret; } else if (cred != nullptr) { struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msgh); struct ucred *receivedUcred = (struct ucred*)CMSG_DATA(cmsg); if (receivedUcred == nullptr) { - delete [] *data; - *data = nullptr; return 0; } *cred = *receivedUcred; } - (*data)[ret - 1] = 0; + buffer[ret - 1] = 0; return ret; } diff --git a/frameworks/native/hilog_input_socket_server.cpp b/frameworks/native/hilog_input_socket_server.cpp index a4549dd..cf84d45 100644 --- a/frameworks/native/hilog_input_socket_server.cpp +++ b/frameworks/native/hilog_input_socket_server.cpp @@ -19,38 +19,62 @@ namespace OHOS { namespace HiviewDFX { -int HilogInputSocketServer::RunServingThread() +HilogInputSocketServer::~HilogInputSocketServer() { - auto thread = std::thread(&HilogInputSocketServer::ServingThread, this); - thread.detach(); - return 0; + StopServingThread(); } -int HilogInputSocketServer::ServingThread() +HilogInputSocketServer::ServerThreadState HilogInputSocketServer::RunServingThread() +{ + if (m_serverThread.get_id() != std::thread().get_id()) { + return ServerThreadState::ALREADY_STARTED; + } + m_serverThread = std::thread(&HilogInputSocketServer::ServingThread, this); + m_stopServer.store(false); + if (m_serverThread.get_id() != std::thread().get_id()) { + return ServerThreadState::JUST_STARTED; + } + return ServerThreadState::CAN_NOT_START; +} + +void HilogInputSocketServer::StopServingThread() +{ + if (m_serverThread.get_id() == std::thread().get_id()) { + return; + } + std::thread tmp; + std::swap(m_serverThread, tmp); + if (tmp.joinable()) { + m_stopServer.store(true); + tmp.join(); + } +} + +void HilogInputSocketServer::ServingThread() { prctl(PR_SET_NAME, "hilogd.server"); int ret; - int length; - char *data = nullptr; + std::vector data; #ifndef __RECV_MSG_WITH_UCRED_ - while ((ret = RecvPacket(&data, &length)) >= 0) { + while ((ret = RecvPacket(data)) >= 0) { if (ret > 0) { - handlePacket(data, length); - delete [] data; - data = nullptr; + m_packetHandler(data); + } + if (m_stopServer.load()) { + break; } } #else - struct ucred cred; - while ((ret = RecvPacket(&data, &length, &cred)) >= 0) { + ucred cred; + while ((ret = RecvPacket(data, &cred)) >= 0) { if (ret > 0) { - handlePacket(cred, data, length); - delete [] data; - data = nullptr; + m_packetHandler(cred, data); + } + if (m_stopServer.load()) { + break; } } #endif - return ret; } } // namespace HiviewDFX } // namespace OHOS diff --git a/frameworks/native/include/dgram_socket_server.h b/frameworks/native/include/dgram_socket_server.h index f2486b7..dd654fd 100644 --- a/frameworks/native/include/dgram_socket_server.h +++ b/frameworks/native/include/dgram_socket_server.h @@ -17,17 +17,17 @@ #define DGRAM_SOCKET_SERVER_H #include "socket_server.h" +#include namespace OHOS { namespace HiviewDFX { class DgramSocketServer : public SocketServer { public: - DgramSocketServer(const std::string& serverPath, uint16_t maxLength) - : SocketServer(serverPath, SOCK_DGRAM), maxLength(maxLength) {} - ~DgramSocketServer() = default; - int RecvPacket(char **data, int *length, struct ucred *cred = nullptr); + DgramSocketServer(const std::string& socketName, uint16_t maxLength) + : SocketServer(socketName, SOCK_DGRAM), maxPacketLength(maxLength) {} + int RecvPacket(std::vector& buffer, struct ucred *cred = nullptr); private: - uint16_t maxLength; + uint16_t maxPacketLength; }; } // namespace HiviewDFX } // namespace OHOS diff --git a/frameworks/native/include/hilog_input_socket_server.h b/frameworks/native/include/hilog_input_socket_server.h index a63d419..c8f7200 100644 --- a/frameworks/native/include/hilog_input_socket_server.h +++ b/frameworks/native/include/hilog_input_socket_server.h @@ -17,6 +17,10 @@ #define HILOG_INPUT_SOCKET_SERVER_H #include "dgram_socket_server.h" +#include +#include +#include +#include namespace OHOS { namespace HiviewDFX { @@ -24,22 +28,34 @@ namespace HiviewDFX { class HilogInputSocketServer : public DgramSocketServer { public: + #ifndef __RECV_MSG_WITH_UCRED_ - explicit HilogInputSocketServer(int (*handlePacket)(char*, unsigned int)) - : DgramSocketServer(INPUT_SOCKET_NAME, MAX_SOCKET_PACKET_LEN), handlePacket(handlePacket){} + using HandlingFunc = std::function& data)>; #else - explicit HilogInputSocketServer(int (*handlePacket)(struct ucred cred, char*, unsigned int)) - : DgramSocketServer(INPUT_SOCKET_NAME, MAX_SOCKET_PACKET_LEN), handlePacket(handlePacket){} + using HandlingFunc = std::function& data)>; #endif - ~HilogInputSocketServer() = default; - int RunServingThread(); + enum class ServerThreadState { + JUST_STARTED, + ALREADY_STARTED, + CAN_NOT_START + }; + + explicit HilogInputSocketServer(HandlingFunc _packetHandler) + : DgramSocketServer(INPUT_SOCKET_NAME, MAX_SOCKET_PACKET_LEN) + , m_packetHandler(_packetHandler) + , m_stopServer(false) + {} + + ~HilogInputSocketServer(); + + ServerThreadState RunServingThread(); + void StopServingThread(); private: -#ifndef __RECV_MSG_WITH_UCRED_ - int (*handlePacket)(char *data, unsigned int dataLen); -#else - int (*handlePacket)(struct ucred cred, char *data, unsigned int dataLen); -#endif - int ServingThread(); + void ServingThread(); + + HandlingFunc m_packetHandler = nullptr; + std::thread m_serverThread; + std::atomic_bool m_stopServer; }; } // namespace HiviewDFX } // namespace OHOS diff --git a/frameworks/native/include/socket_server.h b/frameworks/native/include/socket_server.h index 3df2528..2b98a34 100644 --- a/frameworks/native/include/socket_server.h +++ b/frameworks/native/include/socket_server.h @@ -29,8 +29,8 @@ namespace OHOS { namespace HiviewDFX { class SocketServer { public: - SocketServer(const std::string& serverPath, uint32_t socketType); - ~SocketServer(); + SocketServer(const std::string& _socketName, uint32_t _socketType); + virtual ~SocketServer(); int Init(); int Recv(void *buffer, unsigned int bufferLen, int flags = MSG_PEEK); int RecvMsg(struct msghdr *hdr, int flags = 0); @@ -40,7 +40,7 @@ private: int socketHandler; uint32_t socketType; sockaddr_un serverAddr; - std::string sockName; + std::string socketName; }; } // namespace HiviewDFX } // namespace OHOS diff --git a/frameworks/native/socket_server.cpp b/frameworks/native/socket_server.cpp index d5cc8e7..9ffb16e 100644 --- a/frameworks/native/socket_server.cpp +++ b/frameworks/native/socket_server.cpp @@ -32,13 +32,13 @@ extern "C" { #endif namespace OHOS { namespace HiviewDFX { -SocketServer::SocketServer(const std::string& serverPath, uint32_t socketType) - : socketHandler(0), socketType(socketType), sockName(serverPath) +SocketServer::SocketServer(const std::string& _socketName, uint32_t socketType) + : socketHandler(0), socketType(socketType), socketName(_socketName) { serverAddr.sun_family = AF_UNIX; std::string sockPath(SOCKET_FILE_DIR); - sockPath += sockName; + sockPath += socketName; if (strcpy_s(serverAddr.sun_path, sizeof(serverAddr.sun_path), sockPath.c_str()) != EOK) { return; } @@ -47,11 +47,11 @@ SocketServer::SocketServer(const std::string& serverPath, uint32_t socketType) int SocketServer::Init() { - if (sockName.length()) { + if (socketName.length()) { #ifdef HILOG_USE_MUSL - socketHandler = GetControlSocket(sockName.c_str()); + socketHandler = GetControlSocket(socketName.c_str()); #else - socketHandler = GetExistingSocketServer(sockName.c_str(), socketType); + socketHandler = GetExistingSocketServer(socketName.c_str(), socketType); #endif if (socketHandler >= 0) { return socketHandler; diff --git a/services/hilogd/include/log_collector.h b/services/hilogd/include/log_collector.h index 3109a7d..7988904 100644 --- a/services/hilogd/include/log_collector.h +++ b/services/hilogd/include/log_collector.h @@ -24,17 +24,16 @@ namespace HiviewDFX { class LogCollector { public: LogCollector(HilogBuffer* buffer); - void operator ()(); - static int FlowCtrlDataRecv(HilogMsg *msg, int ret); - static size_t InsertLogToBuffer(const HilogMsg& msg); + void InsertDropInfo(const HilogMsg &msg, int droppedCount); + size_t InsertLogToBuffer(const HilogMsg& msg); #ifndef __RECV_MSG_WITH_UCRED_ - static int onDataRecv(char *data, unsigned int dataLen); + void onDataRecv(std::vector& data); #else - static int onDataRecv(ucred cred, char *data, unsigned int dataLen); + void onDataRecv(const ucred& cred, std::vector& data); #endif ~LogCollector() = default; private: - static HilogBuffer* hilogBuffer; + HilogBuffer* m_hilogBuffer = nullptr; }; } // namespace HiviewDFX } // namespace OHOS diff --git a/services/hilogd/log_collector.cpp b/services/hilogd/log_collector.cpp index 8ee8fd3..cc03dde 100644 --- a/services/hilogd/log_collector.cpp +++ b/services/hilogd/log_collector.cpp @@ -21,82 +21,69 @@ #include #include #include +#include namespace OHOS { namespace HiviewDFX { using namespace std; -HilogBuffer* LogCollector::hilogBuffer = nullptr; -int LogCollector::FlowCtrlDataRecv(HilogMsg *msg, int ret) + +void LogCollector::InsertDropInfo(const HilogMsg &msg, int droppedCount) { - string dropLog = to_string(ret) + " line(s) dropped!"; - char tag[] = "LOGLIMITD"; - int len = sizeof(HilogMsg) + sizeof(tag) + dropLog.size() + 1; - HilogMsg *dropMsg = (HilogMsg*)malloc(len); + string dropLog = to_string(droppedCount) + " line(s) dropped!"; + constexpr auto tag = "LOGLIMITD"sv; + std::vector buffer(sizeof(HilogMsg) + tag.size() + dropLog.size() + 1, '\0'); + HilogMsg *dropMsg = reinterpret_cast(buffer.data()); if (dropMsg != nullptr) { - dropMsg->len = len; - dropMsg->version = msg->version; - dropMsg->type = msg->type; - dropMsg->level = msg->level; - dropMsg->tag_len = sizeof(tag) + 1; - dropMsg->tv_sec = msg->tv_sec; - dropMsg->tv_nsec = msg->tv_nsec; - dropMsg->pid = msg->pid; - dropMsg->tid = msg->tid; - dropMsg->domain = msg->domain; - if (memcpy_s(dropMsg->tag, len - sizeof(HilogMsg), tag, sizeof(tag))) { - } - if (memcpy_s(dropMsg->tag + sizeof(tag), len - sizeof(HilogMsg) - sizeof(tag), - dropLog.c_str(), dropLog.size() + 1)) { - } + dropMsg->len = buffer.size(); + dropMsg->version = msg.version; + dropMsg->type = msg.type; + dropMsg->level = msg.level; + dropMsg->tag_len = tag.size(); + dropMsg->tv_sec = msg.tv_sec; + dropMsg->tv_nsec = msg.tv_nsec; + dropMsg->pid = msg.pid; + dropMsg->tid = msg.tid; + dropMsg->domain = msg.domain; + + auto remainSize = buffer.size() - sizeof(HilogMsg); + memcpy_s(dropMsg->tag, remainSize, tag.data(), tag.size()); + + remainSize -= tag.size(); + auto logTextPtr = dropMsg->tag + tag.size(); // log text is behind tag data + memcpy_s(logTextPtr, remainSize, dropLog.data(), dropLog.size() + 1); + InsertLogToBuffer(*dropMsg); - free(dropMsg); } - return 0; } #ifndef __RECV_MSG_WITH_UCRED_ -int LogCollector::onDataRecv(char *data, unsigned int dataLen) -{ - HilogMsg *msg = (HilogMsg *)data; - /* Domain flow control */ - int ret = FlowCtrlDomain(msg); - if (ret < 0) { - return 0; - } else if (ret > 0) { /* if >0 !Need print how many lines was dopped */ - FlowCtrlDataRecv(msg, ret); - } - InsertLogToBuffer(*msg); - return 0; -} +void LogCollector::onDataRecv(std::vector& data) #else -int LogCollector::onDataRecv(ucred cred, char *data, unsigned int dataLen) +void LogCollector::onDataRecv(const ucred& cred, std::vector& data) +#endif { - HilogMsg *msg = (HilogMsg *)data; - msg->pid = cred.pid; - /* Domain flow control */ + if (data.size() < sizeof(HilogMsg)) { + std::cerr << "Internal error - received data less than HilogMsg size"; + return; + } + + HilogMsg *msg = reinterpret_cast(data.data()); + #ifdef __RECV_MSG_WITH_UCRED_ + msg->pid = cred.pid; + #endif + // Domain flow control int ret = FlowCtrlDomain(msg); if (ret < 0) { - return 0; + // dropping message + return; } else if (ret > 0) { /* if >0 !Need print how many lines was dopped */ - FlowCtrlDataRecv(msg, ret); + // store info how many was dropped + InsertDropInfo(*msg, ret); } InsertLogToBuffer(*msg); - return 0; } -#endif -LogCollector::LogCollector(HilogBuffer* buffer) +LogCollector::LogCollector(HilogBuffer* buffer) : m_hilogBuffer(buffer) { - hilogBuffer = buffer; -} - -void LogCollector::operator()() -{ - HilogInputSocketServer server(onDataRecv); - if (server.Init() < 0) { - cout << "Failed to init control server socket ! error=" << strerror(errno) << std::endl; - } else { - server.RunServingThread(); - } } size_t LogCollector::InsertLogToBuffer(const HilogMsg& msg) @@ -104,19 +91,19 @@ size_t LogCollector::InsertLogToBuffer(const HilogMsg& msg) if (msg.type >= LOG_TYPE_MAX) { return ERR_LOG_TYPE_INVALID; } - size_t result = hilogBuffer->Insert(msg); + size_t result = m_hilogBuffer->Insert(msg); if (result <= 0) { return result; } - hilogBuffer->logReaderListMutex.lock_shared(); - auto it = hilogBuffer->logReaderList.begin(); - while (it != hilogBuffer->logReaderList.end()) { + m_hilogBuffer->logReaderListMutex.lock_shared(); + auto it = m_hilogBuffer->logReaderList.begin(); + while (it != m_hilogBuffer->logReaderList.end()) { if ((*it).lock()->GetType() != TYPE_CONTROL) { (*it).lock()->NotifyForNewData(); } ++it; } - hilogBuffer->logReaderListMutex.unlock_shared(); + m_hilogBuffer->logReaderListMutex.unlock_shared(); return result; } } // namespace HiviewDFX diff --git a/services/hilogd/main.cpp b/services/hilogd/main.cpp index 4d7b441..2cfbc65 100644 --- a/services/hilogd/main.cpp +++ b/services/hilogd/main.cpp @@ -70,9 +70,20 @@ int HilogdEntry(int argc, char* argv[]) InitDomainFlowCtrl(); // Start log_collector - LogCollector logCollector(&hilogBuffer); - HilogInputSocketServer server(logCollector.onDataRecv); - if (server.Init() < 0) { + #ifndef __RECV_MSG_WITH_UCRED_ + auto onDataReceive = [&hilogBuffer](std::vector& data) { + static LogCollector logCollector(&hilogBuffer); + logCollector.onDataRecv(data); + }; + #else + auto onDataReceive = [&hilogBuffer](const ucred& cred, std::vector& data) { + static LogCollector logCollector(&hilogBuffer); + logCollector.onDataRecv(cred, data); + }; + #endif + + HilogInputSocketServer incomingLogsServer(onDataReceive); + if (incomingLogsServer.Init() < 0) { #ifdef DEBUG cout << "Failed to init input server socket ! error=" << strerror(errno) << std::endl; #endif @@ -83,7 +94,7 @@ int HilogdEntry(int argc, char* argv[]) #ifdef DEBUG cout << "Begin to listen !\n"; #endif - server.RunServingThread(); + incomingLogsServer.RunServingThread(); } std::thread startupCheckThread([&hilogBuffer]() { -- Gitee From 78e0fd3466503ef3aea66bbe67cf8cc90540a713 Mon Sep 17 00:00:00 2001 From: Karol Grydziuszko Date: Tue, 23 Nov 2021 13:13:15 +0100 Subject: [PATCH 2/2] Fixed CodeCheck warnings. Signed-off-by: Karol Grydziuszko Change-Id: Ief4c0bf9b208dfe2ce16d18e10920b72d7702a60 --- .../native/include/hilog_input_socket_server.h | 1 - services/hilogd/log_collector.cpp | 16 ++++++++++------ services/hilogd/main.cpp | 6 +++--- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/frameworks/native/include/hilog_input_socket_server.h b/frameworks/native/include/hilog_input_socket_server.h index c8f7200..7a451f3 100644 --- a/frameworks/native/include/hilog_input_socket_server.h +++ b/frameworks/native/include/hilog_input_socket_server.h @@ -28,7 +28,6 @@ namespace HiviewDFX { class HilogInputSocketServer : public DgramSocketServer { public: - #ifndef __RECV_MSG_WITH_UCRED_ using HandlingFunc = std::function& data)>; #else diff --git a/services/hilogd/log_collector.cpp b/services/hilogd/log_collector.cpp index cc03dde..dfb974a 100644 --- a/services/hilogd/log_collector.cpp +++ b/services/hilogd/log_collector.cpp @@ -46,11 +46,15 @@ void LogCollector::InsertDropInfo(const HilogMsg &msg, int droppedCount) dropMsg->domain = msg.domain; auto remainSize = buffer.size() - sizeof(HilogMsg); - memcpy_s(dropMsg->tag, remainSize, tag.data(), tag.size()); + if (memcpy_s(dropMsg->tag, remainSize, tag.data(), tag.size())) { + std::cerr << "Can't copy tag info of Drop Info message\n"; + } remainSize -= tag.size(); auto logTextPtr = dropMsg->tag + tag.size(); // log text is behind tag data - memcpy_s(logTextPtr, remainSize, dropLog.data(), dropLog.size() + 1); + if (memcpy_s(logTextPtr, remainSize, dropLog.data(), dropLog.size() + 1)) { + std::cerr << "Can't copy log text info of Drop Info message\n"; + } InsertLogToBuffer(*dropMsg); } @@ -67,10 +71,10 @@ void LogCollector::onDataRecv(const ucred& cred, std::vector& data) } HilogMsg *msg = reinterpret_cast(data.data()); - #ifdef __RECV_MSG_WITH_UCRED_ - msg->pid = cred.pid; - #endif - // Domain flow control +#ifdef __RECV_MSG_WITH_UCRED_ + msg->pid = cred.pid; +#endif + // Domain flow control int ret = FlowCtrlDomain(msg); if (ret < 0) { // dropping message diff --git a/services/hilogd/main.cpp b/services/hilogd/main.cpp index 2cfbc65..e95b4dd 100644 --- a/services/hilogd/main.cpp +++ b/services/hilogd/main.cpp @@ -70,17 +70,17 @@ int HilogdEntry(int argc, char* argv[]) InitDomainFlowCtrl(); // Start log_collector - #ifndef __RECV_MSG_WITH_UCRED_ +#ifndef __RECV_MSG_WITH_UCRED_ auto onDataReceive = [&hilogBuffer](std::vector& data) { static LogCollector logCollector(&hilogBuffer); logCollector.onDataRecv(data); }; - #else +#else auto onDataReceive = [&hilogBuffer](const ucred& cred, std::vector& data) { static LogCollector logCollector(&hilogBuffer); logCollector.onDataRecv(cred, data); }; - #endif +#endif HilogInputSocketServer incomingLogsServer(onDataReceive); if (incomingLogsServer.Init() < 0) { -- Gitee