Hi Vu Ack
Can push when the location of the .xml files is solved and is included in a patch to push and all other reviewers has acked. Thanks Lennart > -----Original Message----- > From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > Sent: den 14 mars 2016 11:03 > To: Lennart Lund; 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] > > Thanks, Lennart. > > I have updated the code. Also, correct the indentation in logtest.c/.h and > tet_LogOiOps.c > > ➤ sha1sum lgsv_node_originating_r3.patch > 85446d68fa3785b3eac1a132d6668a09aead2b7d > lgsv_node_originating_r3.patch > > For location of gcfg_classes.xml and gcfg_objects.xml files, I am looking for > Anders W & Mathi's feedback. > > Regards, Vu. > > >-----Original Message----- > >From: Lennart Lund [mailto:lennart.l...@ericsson.com] > >Sent: Friday, March 11, 2016 7:56 PM > >To: Vu Minh Nguyen; Anders Widell; mathi.naic...@oracle.com > >Cc: opensaf-devel@lists.sourceforge.net; Beatriz Brandao; Jorge Pacheco > >Garcia; Lennart Lund > >Subject: RE: [PATCH 0 of 1] Review Request for log: Extend information > about > >origin of log record [#1480] > > > >Hi Vu > > > >Ack with comment: > > > >I think you should fix the get_attr_value() function before pushing. > >What is the decision for placement of gcfg_classes.xml and > gcfg_objects.xml? > >Must of course also be fixed > > > >Thanks > >Lennart > > > >> -----Original Message----- > >> From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > >> Sent: den 8 mars 2016 03:49 > >> To: Lennart Lund; 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 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://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
