Hi Lennart, Please see my responses inline, with [Vu].
Regards, Vu. >-----Original Message----- >From: Lennart Lund [mailto:lennart.l...@ericsson.com] >Sent: Thursday, March 03, 2016 10:38 PM >To: Vu Minh Nguyen; Anders Widell; mathi.naic...@oracle.com >Cc: opensaf-devel@lists.sourceforge.net; Beatriz Brandao; Jorge Pacheco Garcia >Subject: RE: [PATCH 0 of 1] Review Request for log: Extend information about >origin of log record [#1480] > >Hi Vu > >My comments: > >--------------------------------------- >logtest.c >get_attr_value() >This function looks like it can get a value from any attribute in any object but >this does not seems to be the case. >It can only get values from some specific objects and also not for all attributes >in those objects. This is very unclean code and should be improved. Also the >test code should be kept as clean as possible especially global functions. >There are several ways of handling this e.g. >A function that takes the object name and the attribute name (as a C strings) >as in parameters and has a void pointer for the fetched value. Since no >attributes in the log service can be multivalue it should be ok to always give >value[0] as output. This is also a void pointer. The calling function should know >how to typecast the fetched value to the correct type. The only limitation for >this function is that it will only return the first value in case of multivalue but >this should be no problem. > [Vu] Thanks. I propose to improve that in an enhancement ticket. With that ticket, we can add points to be improved. > >--------------------------------------- >gcfg_classes.xml >gcfg_objects.xml > >Has to be moved to an appropriate directory. Cannot be placed with the log >service. Talk to Anders W. >Maybe other reviewers also have comments about this. > [Vu] Yes, I think so. Anders & Mathi may give comments on this. > >--------------------------------------- >lgs_imm_gcfg.cc >I hope you have thoroughly checked this code. This code is a rather quickly >created prototype code, is not very well tested and had not been reviewed by >anybody before you got it. It should also be verified with valgrind thread check >(has not been done) >- Spelling error on line 60. Replace "snd" with "and" >- There is no information telling what this function is doing and why >send_command() function [Vu] Thanks. I added the description on top of this function. > > >--------------------------------------- >lgs_mbcsv.cc >Remove TODO on line 1625 >Why adding inparameter nodeId to function dec_write_log_async_msg()? Not >used in function >Can also be removed from mds_dec() function? (line 799) [Vu] I removed them. > > >--------------------------------------- >README >I suggest some changes in the text added to README > >Original text: >3. New tokens are added (#1480) >------------------------------- >- @Cp: for showing the network name >- @Cq: for showing node name where the log record comes from. > >a) The network name comes from an configurable attribute >`opensafNetworkName` > which belongs to global configurable class `OpensafConfig`. > The attribute can be accessed via DN >`opensafConfigId=opensafGlobalConfig,safApp=OpenSAF`. > > LOG service is an applier to this object class, so that whenever there is >change in > network name attribute `opensafNetworkName`, LOG service will be notified. > >b) Regarding node name, LOG service gets this information when decoding >messages at MDS layer. > >Suggested modifications: >3. New tokens are added (#1480) >------------------------------- >- @Cp: for showing the network name >- @Cq: for showing node name where the log record comes from. > >a) The network name comes from an attribute, `opensafNetworkName` > which belongs to the `OpensafConfig` class. > The `opensafConfigId=opensafGlobalConfig,safApp=OpenSAF` object of this >class is an > OpenSAF global configuration object. > > LOG service is an applier for this object, so that whenever there is change of > network name attribute `opensafNetworkName`, LOG service will be notified. > >b) Regarding node name, LOG service gets this information when decoding >messages at MDS layer. > [Vu] Thanks for your suggestion. I will update README file accordingly. > >Thanks >Lennart > >> -----Original Message----- >> From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] >> Sent: den 26 februari 2016 02:49 >> To: Anders Widell; Lennart Lund; mathi.naic...@oracle.com >> Cc: opensaf-devel@lists.sourceforge.net >> Subject: [PATCH 0 of 1] Review Request for log: Extend information about >> origin of log record [#1480] >> >> Summary: log: Extend information about origin of log record [#1480] >> Review request for Trac Ticket(s): #1468 >> Peer Reviewer(s): Lennart, Anders W, Mathi >> Pull request to: Lennart >> Affected branch(es): Default >> Development branch: Default >> >> -------------------------------- >> Impacted area Impact y/n >> -------------------------------- >> Docs n >> Build system n >> RPM/packaging n >> Configuration files n >> Startup scripts n >> SAF services y >> OpenSAF services n >> Core libraries n >> Samples n >> Tests n >> Other n >> >> >> Comments (indicate scope for each "y" above): >> --------------------------------------------- >> <<EXPLAIN/COMMENT THE PATCH SERIES HERE>> >> >> changeset a6c0e7e9785c75c6dcf57404027fc92fc572a25f >> Author: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au> >> Date: Tue, 02 Feb 2016 10:02:37 +0700 >> >> log: Extend information about origin of log record [#1480] >> >> Add new tokens (@Cq and @Cp) to represent node name and >> network name. >> >> >> Added Files: >> ------------ >> osaf/services/saf/logsv/config/gcfg_classes.xml >> osaf/services/saf/logsv/config/gcfg_objects.xml >> osaf/services/saf/logsv/lgs/lgs_imm_gcfg.cc >> osaf/services/saf/logsv/lgs/lgs_imm_gcfg.h >> >> >> Complete diffstat: >> ------------------ >> osaf/services/saf/logsv/README | 16 + >> osaf/services/saf/logsv/config/Makefile.am | 4 +- >> osaf/services/saf/logsv/config/gcfg_classes.xml | 18 + >> osaf/services/saf/logsv/config/gcfg_objects.xml | 6 + >> osaf/services/saf/logsv/lgs/Makefile.am | 6 +- >> osaf/services/saf/logsv/lgs/lgs.h | 1 + >> osaf/services/saf/logsv/lgs/lgs_amf.cc | 9 +- >> osaf/services/saf/logsv/lgs/lgs_evt.cc | 11 +- >> osaf/services/saf/logsv/lgs/lgs_evt.h | 1 + >> osaf/services/saf/logsv/lgs/lgs_fmt.cc | 52 ++++- >> osaf/services/saf/logsv/lgs/lgs_fmt.h | 20 +- >> osaf/services/saf/logsv/lgs/lgs_imm_gcfg.cc | 1082 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> +++++++++++++++++++++++++++++++++++++++++++++++++ >> osaf/services/saf/logsv/lgs/lgs_imm_gcfg.h | 28 ++ >> osaf/services/saf/logsv/lgs/lgs_main.cc | 1 + >> osaf/services/saf/logsv/lgs/lgs_mbcsv.cc | 50 +++- >> osaf/services/saf/logsv/lgs/lgs_mbcsv.h | 1 + >> osaf/services/saf/logsv/lgs/lgs_mds.cc | 13 +- >> tests/logsv/logtest.c | 7 + >> tests/logsv/logtest.h | 2 + >> tests/logsv/tet_LogOiOps.c | 205 ++++++++++++++++++++ >> 20 files changed, 1498 insertions(+), 35 deletions(-) >> >> >> Testing Commands: >> ----------------- >> There are 02 added new test cases to test node name token >> and network name token. Run them (following) for testing. >> >> logtest 4 63 >> logtest 4 64 >> >> NOTE (dependencies): >> ---- >> This patch has to be merged on top of following patches (in review/not >> pushed yet) >> 1) #1522 MDS: Include node name as a part of control events >> 2) #1179 log: add support for cloud resilience feature >> >> >> Testing, Expected Results: >> -------------------------- >> All test cases passed >> >> >> Conditions of Submission: >> ------------------------- >> Get ack from peer reviewers >> >> >> Arch Built Started Linux distro >> ------------------------------------------- >> mips n n >> mips64 n n >> x86 n n >> x86_64 n n >> 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 ~/.hgrc file (i.e. username, 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 >> for in-service upgradability test. >> >> ___ Your changes affect user manual and documentation, your patch series >> do not contain the patch that updates the Doxygen manual. ------------------------------------------------------------------------------ Transform Data into Opportunity. Accelerate data analysis in your applications with Intel Data Analytics Acceleration Library. Click to learn more. http://makebettercode.com/inteldaal-eval _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
