Re: [devel] [PATCH 0/2] Review Request for dtm: Added following options --max-backups and --max-file-size to osaflog tool and in transportd [#2731]

[Anders Widell]

Hi!

Ack with comments:

1) The log file size is currently rounded up to the nearest 128KB. If 
this is to be considered a feature it ought to be documented, but I 
think you should instead fix it. The following patch will solve the problem:


diff --git a/src/dtm/transport/log_writer.cc 
b/src/dtm/transport/log_writer.cc

index 60c0bf557..94c3b3a83 100644
--- a/src/dtm/transport/log_writer.cc
+++ b/src/dtm/transport/log_writer.cc
@@ -89,7 +89,8 @@ void LogWriter::RotateLog() {

 void LogWriter::Write(size_t size) {
   current_buffer_size_ += size;
- ess if (current_buffer_size_ > kBufferSize - kMaxMessageSize) Flush();
+  if (current_buffer_size_ > kBufferSize - kMaxMageSize ||
+  current_buffer_size_ >= kmax_file_size_) Flush();
 }

2) You still haven't changed the protocol back to a text-based protocol. 
I am acking the patch anyway and we can fix this in a separate ticket, 
so that we get some progress on this issue.


3) Please read the style guide. A lot of my comments are about 
whitespace and naming of structures and variables.


4) See more comments inline in the code below, marked AndersW>. Fix them 
before pushing.


regards,
Anders Widell

On 03/14/2018 12:33 PM, Anders Widell wrote:

Hi!

The second patch in this patch series is just fixing review comments 
in the first patch. Please squash the two patches into one single 
patch using e.g. "git rebase -i". I have already done so and pasted 
the resulting single patch below in this mail. I will give my review 
comments to the squashed patch as a reply to this mail.


regards,

Anders Widell

---
 opensaf.spec.in    |   2 +
 src/dtm/Makefile.am    |   3 +
 src/dtm/common/osaflog_protocol.h  |  13 +++
 src/dtm/tools/osaflog.cc   | 168 
-

 src/dtm/transport/log_server.cc    | 111 ---
 src/dtm/transport/log_server.h |  16 ++-
 src/dtm/transport/log_writer.cc    |   9 +-
 src/dtm/transport/log_writer.h |   5 +-
 src/dtm/transport/main.cc  |   4 +
 src/dtm/transport/osaf-transport.in    |   1 +
 src/dtm/transport/tests/log_writer_test.cc |   2 +-
 src/dtm/transport/transportd.conf  |  13 +++
 12 files changed, 298 insertions(+), 49 deletions(-)
 create mode 100644 src/dtm/transport/transportd.conf

diff --git a/opensaf.spec.in b/opensaf.spec.in
index 187e20e7d..ac921747e 100644
--- a/opensaf.spec.in
+++ b/opensaf.spec.in
@@ -1400,6 +1400,7 @@ fi
 %config(noreplace) %{_pkgsysconfdir}/nodeinit.conf.controller
 %config(noreplace) %{_pkgsysconfdir}/nodeinit.conf.payload
 %config(noreplace) %{_pkgsysconfdir}/dtmd.conf
+%config(noreplace) %{_pkgsysconfdir}/transportd.conf
 %{_pkglibdir}/osafrded
 %{_pkgclcclidir}/osaf-rded
 %{_pkglibdir}/osaffmd
@@ -1426,6 +1427,7 @@ fi
 %dir %{_pkgsysconfdir}
 %config(noreplace) %{_pkgsysconfdir}/nodeinit.conf.payload
 %config(noreplace) %{_pkgsysconfdir}/dtmd.conf
+%config(noreplace) %{_pkgsysconfdir}/transportd.conf
 %{_pkglibdir}/osafdtmd
 %{_pkglibdir}/osaftransportd
 %{_pkgclcclidir}/osaf-dtm
diff --git a/src/dtm/Makefile.am b/src/dtm/Makefile.am
index f3ba7200b..822249cbe 100644
--- a/src/dtm/Makefile.am
+++ b/src/dtm/Makefile.am
@@ -57,6 +57,9 @@ nodist_pkgclccli_SCRIPTS += \
 dist_pkgsysconf_DATA += \
 src/dtm/dtmnd/dtmd.conf

+dist_pkgsysconf_DATA += \
+    src/dtm/transport/transportd.conf
+
 bin_osaftransportd_CXXFLAGS = $(AM_CXXFLAGS)

 bin_osaftransportd_CPPFLAGS = \
diff --git a/src/dtm/common/osaflog_protocol.h 
b/src/dtm/common/osaflog_protocol.h

index 61e9f6f39..54551ee48 100644
--- a/src/dtm/common/osaflog_protocol.h
+++ b/src/dtm/common/osaflog_protocol.h
@@ -24,6 +24,19 @@

 namespace Osaflog {

+enum cmd { FLUSH, MAXBACKUPS, MAXFILESIZE};


AndersW> According to the style guide, enum names shall start with a 
capital letter. Abbreviations should be avoided. So rename Cmd to Command.
AndersW> According to the style guide, enum values shall be named 
according to the same rules as constants. So rename them kFlush, 
kMaxBackups and kMaxFileSize.

+enum cmdreply { RFLUSH = 101, RMAXBACKUPS, RMAXFILESIZE, FAILURE};


AndersW> This enum is unnecessary and should be removed. We can use the 
Command enum also in replies.



+struct cmd_osaflog {


AndersW> Rename cmd_osaflog to Message.


+ char marker[4];
+    enum cmd  m_cmd;    // Command Enum


AndersW> The "enum" keyword is redundant and should be removed.
AndersW> The m_ prefix is not according to the naming conventions. Also, 
avoid abbreviations. So rename m_cmd to command.



+ size_t  m_value;   // Value based on the command

AndersW> Rename m_value to value.


+};
+
+
+struct cmd_osaflog_resp {
+    enum cmdreply  m_cmdreply;    // Command Enum
+};


AndersW> The cmd_osaflog_resp structure is unnecessary. Remove it and 
use the Message structure also for replies.



+
 static constexpr const char* kServerSocketPath =
 PKGLOCALSTATEDIR "/osaf_log.sock";

Re: [devel] [PATCH 0/2] Review Request for dtm: Added following options --max-backups and --max-file-size to osaflog tool and in transportd [#2731]

[Anders Widell]

Hi!

The second patch in this patch series is just fixing review comments in 
the first patch. Please squash the two patches into one single patch 
using e.g. "git rebase -i". I have already done so and pasted the 
resulting single patch below in this mail. I will give my review 
comments to the squashed patch as a reply to this mail.


regards,

Anders Widell

---
 opensaf.spec.in    |   2 +
 src/dtm/Makefile.am    |   3 +
 src/dtm/common/osaflog_protocol.h  |  13 +++
 src/dtm/tools/osaflog.cc   | 168 
-

 src/dtm/transport/log_server.cc    | 111 ---
 src/dtm/transport/log_server.h |  16 ++-
 src/dtm/transport/log_writer.cc    |   9 +-
 src/dtm/transport/log_writer.h |   5 +-
 src/dtm/transport/main.cc  |   4 +
 src/dtm/transport/osaf-transport.in    |   1 +
 src/dtm/transport/tests/log_writer_test.cc |   2 +-
 src/dtm/transport/transportd.conf  |  13 +++
 12 files changed, 298 insertions(+), 49 deletions(-)
 create mode 100644 src/dtm/transport/transportd.conf

diff --git a/opensaf.spec.in b/opensaf.spec.in
index 187e20e7d..ac921747e 100644
--- a/opensaf.spec.in
+++ b/opensaf.spec.in
@@ -1400,6 +1400,7 @@ fi
 %config(noreplace) %{_pkgsysconfdir}/nodeinit.conf.controller
 %config(noreplace) %{_pkgsysconfdir}/nodeinit.conf.payload
 %config(noreplace) %{_pkgsysconfdir}/dtmd.conf
+%config(noreplace) %{_pkgsysconfdir}/transportd.conf
 %{_pkglibdir}/osafrded
 %{_pkgclcclidir}/osaf-rded
 %{_pkglibdir}/osaffmd
@@ -1426,6 +1427,7 @@ fi
 %dir %{_pkgsysconfdir}
 %config(noreplace) %{_pkgsysconfdir}/nodeinit.conf.payload
 %config(noreplace) %{_pkgsysconfdir}/dtmd.conf
+%config(noreplace) %{_pkgsysconfdir}/transportd.conf
 %{_pkglibdir}/osafdtmd
 %{_pkglibdir}/osaftransportd
 %{_pkgclcclidir}/osaf-dtm
diff --git a/src/dtm/Makefile.am b/src/dtm/Makefile.am
index f3ba7200b..822249cbe 100644
--- a/src/dtm/Makefile.am
+++ b/src/dtm/Makefile.am
@@ -57,6 +57,9 @@ nodist_pkgclccli_SCRIPTS += \
 dist_pkgsysconf_DATA += \
 src/dtm/dtmnd/dtmd.conf

+dist_pkgsysconf_DATA += \
+    src/dtm/transport/transportd.conf
+
 bin_osaftransportd_CXXFLAGS = $(AM_CXXFLAGS)

 bin_osaftransportd_CPPFLAGS = \
diff --git a/src/dtm/common/osaflog_protocol.h 
b/src/dtm/common/osaflog_protocol.h

index 61e9f6f39..54551ee48 100644
--- a/src/dtm/common/osaflog_protocol.h
+++ b/src/dtm/common/osaflog_protocol.h
@@ -24,6 +24,19 @@

 namespace Osaflog {

+enum cmd { FLUSH, MAXBACKUPS, MAXFILESIZE};
+enum cmdreply { RFLUSH = 101, RMAXBACKUPS, RMAXFILESIZE, FAILURE};
+struct cmd_osaflog {
+    char marker[4];
+    enum cmd  m_cmd;    // Command Enum
+    size_t  m_value;   // Value based on the command
+};
+
+
+struct cmd_osaflog_resp {
+    enum cmdreply  m_cmdreply;    // Command Enum
+};
+
 static constexpr const char* kServerSocketPath =
 PKGLOCALSTATEDIR "/osaf_log.sock";

diff --git a/src/dtm/tools/osaflog.cc b/src/dtm/tools/osaflog.cc
index 3ce66f4eb..e0d135b8d 100644
--- a/src/dtm/tools/osaflog.cc
+++ b/src/dtm/tools/osaflog.cc
@@ -14,6 +14,7 @@
  */

 #include 
+#include 
 #include 
 #include 
 #include 
@@ -39,6 +40,8 @@ namespace {

 void PrintUsage(const char* program_name);
 bool Flush();
+bool MaxTraceFileSize(size_t maxfilesize);
+bool NoOfBackupFiles(size_t nooffiles);
 base::UnixServerSocket* CreateSocket();
 uint64_t Random64Bits(uint64_t seed);
 bool PrettyPrint(const std::string& log_stream);
@@ -53,20 +56,82 @@ char buf[65 * 1024];
 }  // namespace

 int main(int argc, char** argv) {
-  bool flush_option = false;
-  if (argc >= 2 && strcmp(argv[1], "--flush") == 0) {
-    flush_option = true;
-    --argc;
-    ++argv;
-  }
-  if ((argc != 2) && (argc != 1 || flush_option == false)) {
+  struct option long_options[] = {{"max-file-size", required_argument, 
0, 'm'},
+  {"max-backups", required_argument, 0, 
'b'},

+  {"flush", no_argument, 0, 'f'},
+  {"print", required_argument, 0, 'p'},
+  {0, 0, 0, 0}};
+
+  size_t maxfilesize = 0;
+  size_t maxbackups = 0;
+  char *pplog = NULL;
+  int opt = 0;
+
+  int long_index = 0;
+  bool flush_result =  true;
+  bool print_result =  true;
+  bool maxfilesize_result = true;
+  bool noof_backup_result = true;
+  bool flush_set = false;
+  bool prettyprint_set = false;
+  bool maxfilesize_set = false;
+  bool maxbackups_set = false;
+
+  if (argc == 1) {
 PrintUsage(argv[0]);
 exit(EXIT_FAILURE);
   }
-  bool flush_result = Flush();
-  bool print_result = true;
-  if (argc == 2) print_result = PrettyPrint(argv[1]);
-  exit((flush_result && print_result) ? EXIT_SUCCESS : EXIT_FAILURE);
+
+  while ((opt = getopt_long(argc, argv, "m:b:p:f",
+   long_options, _index)) != -1) {
+    switch (opt) {
+ case 'p':
+   pplog = optarg;
+   

[devel] [PATCH 0/2] Review Request for dtm: Added following options --max-backups and --max-file-size to osaflog tool and in transportd [#2731]

[syam-talluri]

Summary: dtm: configure trace file size and no of backups in transportd.conf 
[#2731]
Review request for Ticket(s): 2731
Peer Reviewer(s): anders.wid...@ericsson.com
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-2731
Base revision: 30b70f4a56ab0225d3ade3cc8dda3fe403b5492c
Personal repository: git://git.code.sf.net/u/syam-talluri/review


Impacted area   Impact y/n

 Docsn
 Build systemy
 RPM/packaging   n
 Configuration files n
 Startup scripts n
 SAF servicesn
 OpenSAF servicesn
 Core libraries  y
 Samples n
 Tests   n
 Other   n


Comments (indicate scope for each "y" above):
-
Fixed the cpplint warnings, review comments incorporated

revision 6992d2a095f653efdec428447bfa7841d01c5e53
Author: syam-talluri 
Date:   Fri, 9 Mar 2018 14:58:43 +0530

dtm: Added following options  --max-backups and --max-file-size to osaflog tool 
and in transportd [#2731]



revision 4d73f74f421743ef56ccce8c51c44996d3df6669
Author: syam-talluri 
Date:   Fri, 9 Mar 2018 14:58:43 +0530

dtm: configure trace file size and no of backups in transportd.conf [#2731]



Added Files:

 src/dtm/transport/transportd.conf


Complete diffstat:
--
 opensaf.spec.in|   2 +
 src/dtm/Makefile.am|   3 +
 src/dtm/common/osaflog_protocol.h  |  13 +++
 src/dtm/tools/osaflog.cc   | 168 -
 src/dtm/transport/log_server.cc| 111 ---
 src/dtm/transport/log_server.h |  16 ++-
 src/dtm/transport/log_writer.cc|   9 +-
 src/dtm/transport/log_writer.h |   5 +-
 src/dtm/transport/main.cc  |   4 +
 src/dtm/transport/osaf-transport.in|   1 +
 src/dtm/transport/tests/log_writer_test.cc |   2 +-
 src/dtm/transport/transportd.conf  |  13 +++
 12 files changed, 298 insertions(+), 49 deletions(-)


Testing Commands:
-
*** LIST THE COMMAND LINE TOOLS/STEPS TO TEST YOUR CHANGES ***


Testing, Expected Results:
--
*** PASTE COMMAND OUTPUTS / TEST RESULTS ***


Conditions of Submission:
-
Ack from the Reviewer


Arch  Built StartedLinux distro
---
mipsn  n
mips64  n  n
x86 n  n
x86_64  y  y
powerpc n  n
powerpc64   n  n


Reviewer Checklist:
---
[Submitters: make sure that your review doesn't trigger any checkmarks!]


Your checkin has not passed review because (see checked entries):

___ Your RR template is generally incomplete; it has too many blank entries
that need proper data filled in.

___ You have failed to nominate the proper persons for review and push.

___ Your patches do not have proper short+long header

___ You have grammar/spelling in your header that is unacceptable.

___ You have exceeded a sensible line length in your headers/comments/text.

___ You have failed to put in a proper Trac Ticket # into your commits.

___ You have incorrectly put/left internal data in your comments/files
(i.e. internal bug tracking tool IDs, product names etc)

___ You have not given any evidence of testing beyond basic build tests.
Demonstrate some level of runtime or other sanity testing.

___ You have ^M present in some of your files. These have to be removed.

___ You have needlessly changed whitespace or added whitespace crimes
like trailing spaces, or spaces before tabs.

___ You have mixed real technical changes with whitespace and other
cosmetic code cleanup changes. These have to be separate commits.

___ You need to refactor your submission into logical chunks; there is
too much content into a single commit.

___ You have extraneous garbage in your review (merge commits etc)

___ You have giant attachments which should never have been sent;
Instead you should place your content in a public tree to be pulled.

___ You have too many commits attached to an e-mail; resend as threaded
commits, or place in a public tree for a pull.

___ You have resent this content multiple times without a clear indication
of what has changed between each re-send.

___ You have failed to adequately and individually address all of the
comments and change requests that were proposed in the initial review.

___ You have a misconfigured ~/.gitconfig file (i.e. user.name, user.email etc)

___ Your computer have a badly configured date and time; confusing the
the threaded patch review.

___ Your changes affect IPC mechanism, and you don't present any results

[devel] [PATCH 0/2] Review Request for dtm: Added following options --max-backups and --max-file-size to osaflog tool and in transportd [#2731]

[syam-talluri]

Summary: dtm: configure trace file size and no of backups in transportd.conf 
[#2731]
Review request for Ticket(s): 2731
Peer Reviewer(s):anders.wid...@ericsson.com
Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE ***
Affected branch(es): develop
Development branch: ticket-2731
Base revision: a2ee56579bb33a05477b151c15d17f3b12025cf8
Personal repository: git://git.code.sf.net/u/syam-talluri/review


Impacted area   Impact y/n

 Docsy
 Build systemy
 RPM/packaging   y
 Configuration files y
 Startup scripts n
 SAF servicesn
 OpenSAF servicesy
 Core libraries  y
 Samples n
 Tests   n
 Other   n

NOTE: Patch(es) contain lines longer than 80 characers

Comments (indicate scope for each "y" above):
-
dtm: Added following options  --max-backups and --max-file-size to osaflog tool 
and in implemented the
functionality in the transportd. The messaging format between osaflog and 
transportd is modified by 
introducing a structure.

revision b289abf08100596c382765c82c94763ac486731e
Author: syam-talluri 
Date:   Mon, 12 Feb 2018 17:29:26 +0530

dtm: Added following options  --max-backups and --max-file-size to osaflog tool 
and in transportd [#2731]



revision 0723340e8f4802e82655037e8d2743dddc684180
Author: syam-talluri 
Date:   Mon, 12 Feb 2018 17:29:26 +0530

dtm: configure trace file size and no of backups in transportd.conf [#2731]



Added Files:

 src/dtm/transport/transportd.conf


Complete diffstat:
--
 opensaf.spec.in|   2 +
 src/dtm/Makefile.am|   3 +
 src/dtm/common/osaflog_protocol.h  |  15 ++
 src/dtm/tools/osaflog.cc   | 262 ++---
 src/dtm/transport/log_server.cc| 113 +++--
 src/dtm/transport/log_server.h |  14 +-
 src/dtm/transport/log_writer.cc|   6 +-
 src/dtm/transport/log_writer.h |   4 +-
 src/dtm/transport/main.cc  |   3 +
 src/dtm/transport/osaf-transport.in|   1 +
 src/dtm/transport/tests/log_writer_test.cc |   2 +-
 src/dtm/transport/transportd.conf  |  13 ++
 12 files changed, 391 insertions(+), 47 deletions(-)


Testing Commands:
-
Run osaflog command tool and tryied the --max-file-size=8 and --max--backups=9

Testing, Expected Results:
--
*** PASTE COMMAND OUTPUTS / TEST RESULTS ***


Conditions of Submission:
-
*** HOW MANY DAYS BEFORE PUSHING, CONSENSUS ETC ***


Arch  Built StartedLinux distro
---
mipsn  n
mips64  n  n
x86 n  n
x86_64  y  y
powerpc n  n
powerpc64   n  n


Reviewer Checklist:
---
[Submitters: make sure that your review doesn't trigger any checkmarks!]


Your checkin has not passed review because (see checked entries):

___ Your RR template is generally incomplete; it has too many blank entries
that need proper data filled in.

___ You have failed to nominate the proper persons for review and push.

___ Your patches do not have proper short+long header

___ You have grammar/spelling in your header that is unacceptable.

___ You have exceeded a sensible line length in your headers/comments/text.

___ You have failed to put in a proper Trac Ticket # into your commits.

___ You have incorrectly put/left internal data in your comments/files
(i.e. internal bug tracking tool IDs, product names etc)

___ You have not given any evidence of testing beyond basic build tests.
Demonstrate some level of runtime or other sanity testing.

___ You have ^M present in some of your files. These have to be removed.

___ You have needlessly changed whitespace or added whitespace crimes
like trailing spaces, or spaces before tabs.

___ You have mixed real technical changes with whitespace and other
cosmetic code cleanup changes. These have to be separate commits.

___ You need to refactor your submission into logical chunks; there is
too much content into a single commit.

___ You have extraneous garbage in your review (merge commits etc)

___ You have giant attachments which should never have been sent;
Instead you should place your content in a public tree to be pulled.

___ You have too many commits attached to an e-mail; resend as threaded
commits, or place in a public tree for a pull.

___ You have resent this content multiple times without a clear indication
of what has changed between each re-send.

___ You have failed to adequately and individually address all of the
comments and change requests that were proposed in the initial