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 > >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
