Please see a comment inline: > -----Original Message----- > From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > Sent: Tuesday, March 15, 2016 11:53 AM > To: Mathivanan Naickan Palanivelu; Anders Widell; Lennart Lund > 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 Mathi, > > See my responses inline, with [Vu]. > > Regards, Vu. > > >-----Original Message----- > >From: Mathivanan Naickan Palanivelu [mailto:mathi.naic...@oracle.com] > >Sent: Tuesday, March 15, 2016 1:07 PM > >To: Anders Widell; Vu Minh Nguyen; Lennart Lund > >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, > > > >Comments inline: > > > >> -----Original Message----- > >> From: Anders Widell [mailto:anders.wid...@ericsson.com] > >> Sent: Monday, March 14, 2016 4:52 PM > >> To: Vu Minh Nguyen; Lennart Lund; Mathivanan Naickan Palanivelu > >> 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] > >> > >> See my comments inline. > >> > >> regards, > >> Anders Widell > >> > >> On 03/08/2016 03:49 AM, Vu Minh Nguyen wrote: > >> > 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. > >> [AndersW] We have one existing service that could be a candidate for > >> owning these classes: NID. So one idea is to put them in the > >> services/infrastructure/nid/config/ directory. Another alternative is > >> to > create > >> a new config directory at the global scope: either osaf/config/ or > >> osaf/services/config/. I don't have any strong preference for any of > these > >> alternatives, but if I must pick one I think it would be > osaf/services/config/. > >[Mathi] > >Okay. > [Vu] I will create the new directory "config" under osaf/services > > > > >And also to rename the files to osaf_globalconfig_classes.xml and > >osaf_globalconfig_objects.xml > [Vu] Ok. I will rename them. > > > > >B.T.W A general comment, Please do breakdown patches into smaller > >logical changesets whenever possible. > [Vu] Ok. Will do it from now on for huge patch file. > I am going to resent the patch out for review with following patches: > 1) One patch contains xml files and new added directory > 2) One patch contain changes in log services and updated readme file > 3) One patch contains test code
If you indeed have them as separate changesets you may perhaps just push them instead of another review request. B.T.W since this is about adding a new class, I hope you have tried to test backward compatibility. i.e. For eg:- ACTIVE - 4.7 (with old imm.xml) STANDBY - 5.0 Switchover, and see if appropriate error handling is printed and/or that perhaps the new ACTIVE doesn't reboots!? Thanks, Mathi. > > > > >Thanks, > >Mathi. > > > >> Mathi, do you have any preference or other suggestion? > >> > >> > > >> >> --------------------------------------- > >> >> 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://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140 _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
