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. And also to rename the files to osaf_globalconfig_classes.xml and osaf_globalconfig_objects.xml B.T.W A general comment, Please do breakdown patches into smaller logical changesets whenever possible. 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
