Hi Hoang, Except [PATCH 8 of 8] , I am done with code review , please try to optimize the [PATCH 8 of 8] logic with single function , with three different version arguments.
Please provide updated V2 patch with review comments along with README, so that we can move for testing. -AVM On 7/1/2016 9:59 AM, A V Mahesh wrote: > 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
