Please see my comments inline. Regards, Vu.
>-----Original Message----- >From: Mathivanan Naickan Palanivelu [mailto:mathi.naic...@oracle.com] >Sent: Tuesday, March 15, 2016 1:29 PM >To: Vu Minh Nguyen; 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] > >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. [Vu] Thanks. I will do the test before sending patches for pushing. (I understood that you acked for this patch if the test is ok) >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
