Hi Hoang, Thanks for the clarifications.
Please hold on publishing new version of patch, I am in process of reviewing [PATCH 2 to 8] , so we may have some more comments , so we can have on single consolidated patch V2. -AVM On 7/1/2016 9:27 AM, Vo Minh Hoang wrote: > Dear Mahesh, > > I would like to answer to your comments. > >>> Comment 1 : > I will update PR document so there is not a README file. > >>> Comment 2: > I thought that we should keep current implementation even it is not > beautiful. The reasons are: > - keep a consistence way to access string value. > - 2 pointers to one string brings potential problem for future maintenance. > - For near future changing to CPP, this implementation can easily change to > nicer way. > >>> Comment 3 : > I'm agreed with your comments, will work on that in next version of patch. > >>> Comment 4 : > Firstly, I tried to look for ckpt_name usage in source code but found > nothing. > Secondly, struct cpd_cpnd_info_node contains cpnd node info so it does not > have any ckpt_name related sematic. And struct cpd_cpnd_info_node also have > ckpt_ref_list to refer all its check points. > So ckpt_name seems redundant and need to be removed. > >>> Comment 5 : > I'm agreed. This work has lower priority and will be submitted in different > patch/ticket. > > P.S. About the problem #1574 patches cannot apply on today's staging. I > tested and found that it need to be applied after #1874. I skip sending new > version for updating patch following your comments. > > Thank you and best regards, > Hoang > > -----Original Message----- > From: A V Mahesh [mailto:mahesh.va...@oracle.com] > Sent: Thursday, June 30, 2016 11:41 AM > To: Vo Minh Hoang <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/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/att > achment/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
