Re: [devel] [PATCH 1/1] dtm: Fix the osaflog --flush command, and revert osaflog protocol [#2812]

[Ravi Sekhar Reddy Konda]

Hi Anders,

Ack, code review only

Regards,
Ravi

-Original Message-
From: Anders Widell [mailto:anders.wid...@ericsson.com] 
Sent: Monday, March 19, 2018 8:38 PM
To: Ravi Sekhar Reddy Konda 
Cc: opensaf-devel@lists.sourceforge.net; Anders Widell 

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(CreateSocket());
 
   if (!sock) {
@@ -172,13 +172,12 @@ bool SendCommand(Osaflog::Message message,
   socklen_t addrlen = base::UnixSocket::SetAddress(Osaflog::kServerSocketPath,
_addr);
 
-  ssize_t result = sock->SendTo(, sizeof(message),
+  ssize_t result = sock->SendTo(request.data(), request.size(),
 _addr, addrlen);
   if (result < 0) {
 perror("Failed to send message to osaftransportd");
 return false;
-  } else if (static_cast(result) !=
- (sizeof(Osaflog::Message))) {
+  } else if (static_cast(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(result) !=
-   (sizeof(Osaflog::Message) )) {
-Osaflog::Message result_message;
-memset(_message, 0, sizeof(result_message));
-memcpy(_message, buf, result);
-if (result_message.command != command) {
-   fprintf(stderr, "Received unexpected reply from osaftransportd\n");
-   return false;
-}
+  } else if (static_cast(result) != expected_reply.size() ||
+ memcmp(buf, expected_reply.data(), result) != 0) {
+fprintf(stderr, "ERROR: osaftransportf replied '%s'\n",
+std::string{buf, static_cast(result)}.c_str());
+return false;
   }
   return true;
 }
 
 bool MaxTraceFileSize(size_t max_file_size) {
-  Osaflog::Message message;
-
-  memset(, 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(, 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(, 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 
 

[devel] [PATCH 1/1] dtm: Fix the osaflog --flush command, and revert osaflog protocol [#2812]

[Anders Widell]

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(CreateSocket());
 
   if (!sock) {
@@ -172,13 +172,12 @@ bool SendCommand(Osaflog::Message message,
   socklen_t addrlen = base::UnixSocket::SetAddress(Osaflog::kServerSocketPath,
_addr);
 
-  ssize_t result = sock->SendTo(, sizeof(message),
+  ssize_t result = sock->SendTo(request.data(), request.size(),
 _addr, addrlen);
   if (result < 0) {
 perror("Failed to send message to osaftransportd");
 return false;
-  } else if (static_cast(result) !=
- (sizeof(Osaflog::Message))) {
+  } else if (static_cast(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(result) !=
-   (sizeof(Osaflog::Message) )) {
-Osaflog::Message result_message;
-memset(_message, 0, sizeof(result_message));
-memcpy(_message, buf, result);
-if (result_message.command != command) {
-   fprintf(stderr, "Received unexpected reply from osaftransportd\n");
-   return false;
-}
+  } else if (static_cast(result) != expected_reply.size() ||
+ memcmp(buf, expected_reply.data(), result) != 0) {
+fprintf(stderr, "ERROR: osaftransportf replied '%s'\n",
+std::string{buf, static_cast(result)}.c_str());
+return false;
   }
   return true;
 }
 
 bool MaxTraceFileSize(size_t max_file_size) {
-  Osaflog::Message message;
-
-  memset(, 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(, 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(, 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 
 #include 
+#include 
 #include 
 #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_,