Hi Hoang , On 6/30/2016 9:22 AM, Vo Minh Hoang wrote: > Dear Mahesh, > > Now I am updating documents related to CKPT long DN. > It will be submitted soon. Ok you can ignore comment one.
I will provide patch by patch clarification & review comments, please find them for [PATCH 1 of 8] Comment 1 : ========================================================== @@ -26,6 +27,12 @@ static int __init_cpd(void) { NCS_LIB_REQ_INFO lib_create; + /* Enable extended SaNameT */ + if (setenv("SA_ENABLE_EXTENDED_NAMES", "1", 1) != 0) { + LOG_ER("Failed to set environment variable: SA_ENABLE_EXTENDED_NAMES"); + return m_LEAP_DBG_SINK(NCSCC_RC_FAILURE); + } + ========================================================== Can you please provide README about long DN Support , which will consist information such as `how steps to enable long DN setting` similar to LOG service, please find reference LOG service README Support Long DN `https://sourceforge.net/p/opensaf/tickets/_discuss/thread/6ba7a0c9/8103/attachment/README_LONGDN_R2` Comment 2 : ========================================================== @@ -334,10 +334,10 @@ uint32_t cpd_amf_register(CPD_CB *cpd_cb } if (saAmfComponentRegister(cpd_cb->amf_hdl, &cpd_cb->comp_name, (SaNameT *)NULL) == SA_AIS_OK) { - TRACE_LEAVE2("cpd amf register success for %s",cpd_cb->comp_name.value); + TRACE_LEAVE2("cpd amf register success for %s", osaf_extended_name_borrow(&cpd_cb->comp_name)); ========================================================== I think `cpd_cb->comp_name` `cluster_node.nodeName` kind of variables are fill once and thy are consistent , so is it possible to store such variable once by using `osaf_extended_name_borrow()` when the intial value getting assigned? this comment applies to all other consistent variables . Comment 3 : ========================================================== @@ -166,6 +162,9 @@ uint32_t cpd_ckpt_node_delete(CPD_CB *cb rc = NCSCC_RC_FAILURE; } + if (ckpt_node->ckpt_name != NULL) + free((void *)ckpt_node->ckpt_name); + ========================================================== Ideally frees can be part of m_MMGR_FREE_CPD_CKPT_INFO_NODE() , so that we can avoid multiple free calls ( code readability ) this comment applies to all other free chances. Comment 4 : ========================================================== @@ -123,8 +123,7 @@ typedef struct cpd_cpnd_info_node { uint32_t timer_state; bool ckpt_cpnd_scxb_exist; /* for imm */ - SaNameT node_name; - SaNameT ckpt_name; + SaConstStringT node_name; =========================================================== why ckpt_name removed form struct cpd_cpnd_info_node ? is it no more used in this struct ? Comment 5 : It will be more useful to provide or change the cpsv_demo application to use LONG DN /staging/samples/cpsv -AVM > > Thank you and best regards, > Hoang > > -----Original Message----- > From: A V Mahesh [mailto:mahesh.va...@oracle.com] > Sent: Wednesday, June 29, 2016 10:48 AM > To: Hoang Vo <hoang.m...@dektech.com.au> > Cc: opensaf-devel@lists.sourceforge.net > Subject: Re: [PATCH 0 of 8] Review Request for CKPT: Support DNs longer than > 255 bytes [#1574] > > Hi Hoang, > > On 6/23/2016 4:23 PM, Hoang Vo wrote: >> Testing Commands: >> ----------------- >> start all SCs and PLs >> log in to Sc-1 >> run ckpttest >> enable long DN setting > Can you please how steps to enable long DN setting. it will be more > useful to have README about long DN Support similar to > LOG service, please find reference LOG service README Support Long DN > `https://sourceforge.net/p/opensaf/tickets/_discuss/thread/6ba7a0c9/8103/att > achment/README_LONGDN_R2` > > > -AVM > > > > On 6/23/2016 4:23 PM, Hoang Vo wrote: >> Summary: CKPT: Support DNs longer than 255 bytes [#1574] >> Review request for Trac Ticket(s): 1574 >> Peer Reviewer(s): mahesh.va...@oracle.com; anders.wid...@ericsson.com >> Pull request to: mahesh.va...@oracle.com >> 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 y >> Other n >> >> >> Comments (indicate scope for each "y" above): >> --------------------------------------------- >> >> changeset 729f0b7318a9e628a00ed6ab0617c85d4527aa73 >> Author: Hoang Vo <hoang.m...@dektech.com.au> >> Date: Thu, 23 Jun 2016 17:42:12 +0700 >> >> cpd: Add support for extended SaNameT [#1574] >> >> changeset c2fae0887466adc3414fa64506832122896e6070 >> Author: Hoang Vo <hoang.m...@dektech.com.au> >> Date: Thu, 23 Jun 2016 17:42:12 +0700 >> >> cpnd: Add support for extended SaNameT v1 [#1574] >> >> changeset 98a1ed77d1de469b95022d5ea302b1e3d94b949b >> Author: Hoang Vo <hoang.m...@dektech.com.au> >> Date: Thu, 23 Jun 2016 17:42:12 +0700 >> >> cpa: Add support for extended SaNameT v1 [#1574] >> >> changeset adb30ae18b5972a4948403cabe379d5546ccdbea >> Author: Hoang Vo <hoang.m...@dektech.com.au> >> Date: Thu, 23 Jun 2016 17:42:12 +0700 >> >> cpsv: Add new message to support extended SaNameT v1 [#1574] New > messages >> supporting extended SaNameT are introduce. Encoding and decoding > funtions >> for them are also included. >> >> changeset dd4916df11a3e50a76f9002b00fefd63364f7599 >> Author: Hoang Vo <hoang.m...@dektech.com.au> >> Date: Thu, 23 Jun 2016 17:42:12 +0700 >> >> cpd: Add new mbcsv messages supporting extended SaNameT v1 [#1574] > New MBCSV >> messages supporting extended SaNameT and their encoding/decoding > functions >> are included. >> >> changeset f091e95968329ed246c613537c636efe1508f3e8 >> Author: Hoang Vo <hoang.m...@dektech.com.au> >> Date: Thu, 23 Jun 2016 17:42:12 +0700 >> >> cpsv: Apply new messages supporting extended SaNameT to CPD, CPND, > and CPA >> v1 [#1574] >> >> changeset d1bc0b21ac74f3eb2a6fff7c3d46dd7ef478ec92 >> Author: Hoang Vo <hoang.m...@dektech.com.au> >> Date: Thu, 23 Jun 2016 17:42:12 +0700 >> >> ckpt: Add new test cases to verify long DN feature on CPSV v1 > [#1574] >> changeset 4d297fdba6e02c0501ad9872f98b85372bb6f6ce >> Author: Hoang Vo <hoang.m...@dektech.com.au> >> Date: Thu, 23 Jun 2016 17:42:12 +0700 >> >> imported patch 1574_cpnd_support_recover_shm_version_0_v2.patch >> >> >> Complete diffstat: >> ------------------ >> osaf/libs/agents/saf/cpa/Makefile.am | 1 + >> osaf/libs/agents/saf/cpa/cpa_api.c | 50 ++++--- >> osaf/libs/agents/saf/cpa/cpa_db.c | 2 + >> osaf/libs/agents/saf/cpa/cpa_mds.c | 4 +- >> osaf/libs/agents/saf/cpa/cpa_proc.c | 2 +- >> osaf/libs/common/cpsv/cpsv_evt.c | 505 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > +-- >> osaf/libs/common/cpsv/include/cpa.h | 1 + >> osaf/libs/common/cpsv/include/cpa_cb.h | 2 +- >> osaf/libs/common/cpsv/include/cpa_proc.h | 2 +- >> osaf/libs/common/cpsv/include/cpd.h | 1 + >> osaf/libs/common/cpsv/include/cpd_cb.h | 17 +- >> osaf/libs/common/cpsv/include/cpd_imm.h | 4 +- >> osaf/libs/common/cpsv/include/cpd_proc.h | 2 +- >> osaf/libs/common/cpsv/include/cpnd.h | 1 + >> osaf/libs/common/cpsv/include/cpnd_cb.h | 5 +- >> osaf/libs/common/cpsv/include/cpnd_init.h | 3 +- >> osaf/libs/common/cpsv/include/cpsv_evt.h | 24 +++ >> osaf/libs/common/cpsv/include/cpsv_shm.h | 24 +++- >> osaf/services/saf/cpsv/cpd/Makefile.am | 1 + >> osaf/services/saf/cpsv/cpd/cpd_amf.c | 7 +- >> osaf/services/saf/cpsv/cpd/cpd_db.c | 110 +++++++++------- >> osaf/services/saf/cpsv/cpd/cpd_evt.c | 107 +++++++++++----- >> osaf/services/saf/cpsv/cpd/cpd_imm.c | 268 > ++++++++++++++++++++++++++++++------------ >> osaf/services/saf/cpsv/cpd/cpd_main.c | 7 + >> osaf/services/saf/cpsv/cpd/cpd_mbcsv.c | 31 ++++- >> osaf/services/saf/cpsv/cpd/cpd_mds.c | 85 ++++++++++++- >> osaf/services/saf/cpsv/cpd/cpd_proc.c | 197 > +++++++++++++++++-------------- >> osaf/services/saf/cpsv/cpd/cpd_red.c | 6 +- >> osaf/services/saf/cpsv/cpd/cpd_sbevt.c | 57 +++----- >> osaf/services/saf/cpsv/cpnd/Makefile.am | 1 + >> osaf/services/saf/cpsv/cpnd/cpnd_db.c | 6 +- >> osaf/services/saf/cpsv/cpnd/cpnd_evt.c | 91 +++++++++---- >> osaf/services/saf/cpsv/cpnd/cpnd_main.c | 7 + >> osaf/services/saf/cpsv/cpnd/cpnd_mds.c | 84 ++++++++++++- >> osaf/services/saf/cpsv/cpnd/cpnd_proc.c | 88 ++++++++++--- >> osaf/services/saf/cpsv/cpnd/cpnd_res.c | 823 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > ++++++++++++++++++++++-------------------------------- >> tests/cpsv/Makefile.am | 1 + >> tests/cpsv/test_cpa.c | 180 > ++++++++++++++++++++++++++++- >> tests/cpsv/test_cpa_util.c | 15 ++ >> tests/cpsv/test_cpsv.h | 6 + >> tests/cpsv/test_cpsv_conf.h | 3 + >> 41 files changed, 2205 insertions(+), 626 deletions(-) >> >> >> Testing Commands: >> ----------------- >> start all SCs and PLs >> log in to Sc-1 >> run ckpttest >> enable long DN setting >> run ckpttest again >> log in to PL-3 >> run ckpttest >> enable long DN setting >> run ckpttest again >> >> Testing, Expected Results: >> -------------------------- >> all ckpttest test cases return OK >> >> Conditions of Submission: >> ------------------------- >> ACK from maintainer >> >> Arch Built Started Linux distro >> ------------------------------------------- >> mips n n >> mips64 n n >> x86 n n >> x86_64 y y >> 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. >> > ------------------------------------------------------------------------------ Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San Francisco, CA to explore cutting-edge tech and listen to tech luminaries present their vision of the future. This family event has something for everyone, including kids. Get more information and register today. http://sdm.link/attshape _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel