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]
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]
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]
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-talluriDate: 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]
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-talluriDate: 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