Hi Anders, Ack, code review only
Regards, Ravi -----Original Message----- From: Anders Widell [mailto:[email protected]] Sent: Monday, March 19, 2018 8:38 PM To: Ravi Sekhar Reddy Konda <[email protected]> Cc: [email protected]; Anders Widell <[email protected]> Subject: [PATCH 1/1] dtm: Fix the osaflog --flush command, and revert osaflog protocol [#2812] Fix the remaining review comment for ticket [#2731]: revert back to a text-based protocol between osaflog command and osaftransportd. Also fix the osaflog --flush command, that stopped working after ticket [#2731]. --- src/dtm/common/osaflog_protocol.h | 7 ----- src/dtm/tools/osaflog.cc | 55 ++++++++++-------------------------- src/dtm/transport/log_server.cc | 59 ++++++++++++++++++++------------------- src/dtm/transport/log_server.h | 3 +- 4 files changed, 47 insertions(+), 77 deletions(-) diff --git a/src/dtm/common/osaflog_protocol.h b/src/dtm/common/osaflog_protocol.h index db914e00a..61e9f6f39 100644 --- a/src/dtm/common/osaflog_protocol.h +++ b/src/dtm/common/osaflog_protocol.h @@ -24,13 +24,6 @@ namespace Osaflog { -enum Command { kFlush, kMaxbackups, kMaxfilesize, kFailure }; -struct Message { - char marker[4]; - Command command; // Command Enum - size_t value; // Value based on the command -}; - static constexpr const char* kServerSocketPath = PKGLOCALSTATEDIR "/osaf_log.sock"; diff --git a/src/dtm/tools/osaflog.cc b/src/dtm/tools/osaflog.cc index cf1e6b43c..1de0a85d6 100644 --- a/src/dtm/tools/osaflog.cc +++ b/src/dtm/tools/osaflog.cc @@ -158,9 +158,9 @@ void PrintUsage(const char* program_name) { program_name); } - -bool SendCommand(Osaflog::Message message, - Osaflog::Command command) { +bool SendCommand(const std::string& command) { + std::string request{std::string{"?"} + command}; + std::string expected_reply{std::string{"!"} + command}; auto sock = std::unique_ptr<base::UnixServerSocket>(CreateSocket()); if (!sock) { @@ -172,13 +172,12 @@ bool SendCommand(Osaflog::Message message, socklen_t addrlen = base::UnixSocket::SetAddress(Osaflog::kServerSocketPath, &osaftransportd_addr); - ssize_t result = sock->SendTo(&message, sizeof(message), + ssize_t result = sock->SendTo(request.data(), request.size(), &osaftransportd_addr, addrlen); if (result < 0) { perror("Failed to send message to osaftransportd"); return false; - } else if (static_cast<size_t>(result) != - (sizeof(Osaflog::Message))) { + } else if (static_cast<size_t>(result) != request.size()) { fprintf(stderr, "Failed to send message to osaftransportd\n"); return false; } @@ -214,50 +213,26 @@ bool SendCommand(Osaflog::Message message, if (result < 0) { perror("Failed to receive reply from osaftransportd"); return false; - } else if (static_cast<size_t>(result) != - (sizeof(Osaflog::Message) )) { - Osaflog::Message result_message; - memset(&result_message, 0, sizeof(result_message)); - memcpy(&result_message, buf, result); - if (result_message.command != command) { - fprintf(stderr, "Received unexpected reply from osaftransportd\n"); - return false; - } + } else if (static_cast<size_t>(result) != expected_reply.size() || + memcmp(buf, expected_reply.data(), result) != 0) { + fprintf(stderr, "ERROR: osaftransportf replied '%s'\n", + std::string{buf, static_cast<size_t>(result)}.c_str()); + return false; } return true; } bool MaxTraceFileSize(size_t max_file_size) { - Osaflog::Message message; - - memset(&message, 0, sizeof(message)); - message.marker[0] = '?'; - message.command = Osaflog::kMaxfilesize; - message.value = max_file_size; - - return SendCommand(message, Osaflog::kMaxfilesize); + return SendCommand(std::string{"max-file-size "} + + std::to_string(max_file_size)); } -bool NoOfBackupFiles(size_t number_of_files) { - Osaflog::Message message; - - memset(&message, 0, sizeof(message)); - message.marker[0] = '?'; - message.command = Osaflog::kMaxbackups; - message.value = number_of_files; - - return SendCommand(message, Osaflog::kMaxbackups); +bool NoOfBackupFiles(size_t max_backups) { + return SendCommand(std::string{"max-backups "} + +std::to_string(max_backups)); } bool Flush() { - Osaflog::Message message; - - memset(&message, 0, sizeof(message)); - message.marker[0] = '?'; - message.command = Osaflog::kFlush; - message.value = 0; - - return SendCommand(message, Osaflog::kFlush); + return SendCommand(std::string{"flush"}); } base::UnixServerSocket* CreateSocket() { diff --git a/src/dtm/transport/log_server.cc b/src/dtm/transport/log_server.cc index 44fbe140a..76519cf35 100644 --- a/src/dtm/transport/log_server.cc +++ b/src/dtm/transport/log_server.cc @@ -16,16 +16,16 @@ * */ +#include "dtm/transport/log_server.h" #include <signal.h> #include <syslog.h> +#include <cstdlib> #include <cstring> #include "base/osaf_poll.h" #include "base/time.h" -#include "dtm/transport/log_server.h" #include "dtm/common/osaflog_protocol.h" #include "osaf/configmake.h" - const Osaflog::ClientAddressConstantPrefix LogServer::address_header_{}; LogServer::LogServer(int term_fd) @@ -45,7 +45,6 @@ LogServer::~LogServer() { void LogServer::Run() { struct pollfd pfd[2] = {{term_fd_, POLLIN, 0}, {log_socket_.fd(), POLLIN, 0}}; - do { for (int i = 0; i < 256; ++i) { char* buffer = current_stream_->current_buffer_position(); @@ -105,7 +104,6 @@ LogServer::LogStream* LogServer::GetStream(const char* msg_id, if (iter != log_streams_.end()) return iter->second; if (no_of_log_streams_ >= kMaxNoOfStreams) return nullptr; if (!ValidateLogName(msg_id, msg_id_size)) return nullptr; - LogStream* stream = new LogStream{log_name, no_of_backups_, max_file_size_}; auto result = log_streams_.insert( std::map<std::string, LogStream*>::value_type{log_name, stream}); @@ -193,15 +191,21 @@ bool LogServer::ValidateLogName(const char* msg_id, size_t msg_id_size) { return no_of_dots < 2; } -void LogServer::ExecuteCommand(const char* command, size_t size, +void LogServer::ExecuteCommand(const char* request, size_t size, const struct sockaddr_un& addr, socklen_t addrlen) { if (ValidateAddress(addr, addrlen)) { - struct Osaflog::Message result; - memset(&result, 0, sizeof(result)); - result.marker[0] = '!'; - result.command = ExecuteCommand(command, size); - log_socket_.SendTo(&result, sizeof(result), &addr, addrlen); + size_t command_size; + size_t argument_size; + const char* command = Osaflog::GetField(request, size, 0, &command_size); + const char* argument = Osaflog::GetField(request, size, 1, &argument_size); + std::string reply = ExecuteCommand(command != nullptr + ? std::string{command, command_size} + : std::string{}, + argument != nullptr + ? std::string{argument, argument_size} + : std::string{}); + log_socket_.SendTo(reply.data(), reply.size(), &addr, addrlen); } } @@ -214,26 +218,23 @@ bool LogServer::ValidateAddress(const struct sockaddr_un& addr, } } -Osaflog::Command LogServer::ExecuteCommand(const char * command, - size_t size) { - Osaflog::Message message; - - if (size != sizeof(message)) { - return Osaflog::kFailure; - } - memset(&message, 0, sizeof(message)); - memcpy(&message, command, size); - - if (message.command == Osaflog::kMaxfilesize) { - max_file_size_ = message.value; - return Osaflog::kMaxfilesize; - } else if (message.command == Osaflog::kMaxbackups) { - no_of_backups_ = message.value; - return Osaflog::kMaxbackups; - } else if (message.command == Osaflog::kFlush) { - return Osaflog::kFlush; +std::string LogServer::ExecuteCommand(const std::string& command, + const std::string& argument) { + if (command == "?max-file-size") { + max_file_size_ = atoi(argument.c_str()); + return std::string{"!max-file-size " + +std::to_string(max_file_size_)}; + } else if (command == "?max-backups") { + no_of_backups_ = atoi(argument.c_str()); + return std::string{"!max-backups " + +std::to_string(no_of_backups_)}; + } else if (command == "?flush") { + for (const auto& s : log_streams_) { + LogStream* stream = s.second; + stream->Flush(); + } + return std::string{"!flush"}; + } else { + return std::string{"!not_supported"}; } - return Osaflog::kFailure; } LogServer::LogStream::LogStream(const std::string& log_name, diff --git a/src/dtm/transport/log_server.h b/src/dtm/transport/log_server.h index 822e5e2b2..ee9a9685b 100644 --- a/src/dtm/transport/log_server.h +++ b/src/dtm/transport/log_server.h @@ -86,7 +86,8 @@ class LogServer { const struct sockaddr_un& addr, socklen_t addrlen); static bool ValidateAddress(const struct sockaddr_un& addr, socklen_t addrlen); - Osaflog::Command ExecuteCommand(const char* command, size_t size); + std::string ExecuteCommand(const std::string& command, + const std::string& argument); int term_fd_; // Configuration for LogServer size_t no_of_backups_; -- 2.13.3 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
