Hi Gary, Thanks for the comments. Please see responses inline with [Praveen]
Thanks, Praveen On 26-Aug-16 10:36 AM, Gary Lee wrote: > Hi Praveen > > Some initial comments below: > > Thanks > > Gary > > > diff --git a/osaf/libs/agents/saf/amfa/amf_agent.cc > b/osaf/libs/agents/saf/amfa/amf_agent.cc > --- a/osaf/libs/agents/saf/amfa/amf_agent.cc > +++ b/osaf/libs/agents/saf/amfa/amf_agent.cc > @@ -2812,6 +2812,7 @@ > */ > SaAisErrorT saAmfInitialize_o4(SaAmfHandleT *o_hdl, const > SaAmfCallbacksT_o4 *reg_cbks, SaVersionT *io_ver) > { > + // GL: try to reuse AmfAgent::Initialize()? There is a lot of code > duplication [Praveen] Will try to do? But FC date is near. Other wise I will refactor it post FC. > AVA_CB *cb = 0; > AVA_HDL_DB *hdl_db = 0; > AVA_HDL_REC *hdl_rec = 0; > diff --git a/osaf/libs/agents/saf/amfa/ava_hdl.cc > b/osaf/libs/agents/saf/amfa/ava_hdl.cc > --- a/osaf/libs/agents/saf/amfa/ava_hdl.cc > +++ b/osaf/libs/agents/saf/amfa/ava_hdl.cc > @@ -773,6 +773,7 @@ > > break; > } > + // GL: please check indentation [Praveen] Will correct if before pushing. > case AVSV_AMF_CSI_ATTR_CHANGE: { > AVSV_AMF_CSI_ATTR_CHANGE_PARAM *csi_attr_change = > &info->param.csi_attr_change; > if (!ava_sanamet_is_valid(&csi_attr_change->csi_name)) { > diff --git a/osaf/libs/agents/saf/amfa/ava_mds.cc > b/osaf/libs/agents/saf/amfa/ava_mds.cc > --- a/osaf/libs/agents/saf/amfa/ava_mds.cc > +++ b/osaf/libs/agents/saf/amfa/ava_mds.cc > @@ -1378,6 +1378,8 @@ > LOG_CR("Calloc failed"); > goto done; > } > + > + // GL: reduce scope & declare i inside for loop [Praveen] Will correct it before pushing. > for (i=0; i<csi_attr_change->attrs.number; i++) { > rc = ncs_decode_n_octets_from_uba(uba, > (uint8_t *)&csi_attr_change->attrs.list[i].name, sizeof(SaNameT)); > diff --git a/osaf/libs/common/amf/d2nedu.c b/osaf/libs/common/amf/d2nedu.c > --- a/osaf/libs/common/amf/d2nedu.c > +++ b/osaf/libs/common/amf/d2nedu.c > @@ -402,6 +402,7 @@ > > /*AVSV_D2N_COMPCSI_ASSIGN_MSG_INFO*/ > {EDU_EXEC, ncs_edp_uns32, 0, 0, 0, > + // GL: please check indentation [Praveen] Will correct it before pushing. > (long)&((AVSV_DND_MSG > *)0)->msg_info.d2n_compcsi_assign_msg_info.msg_id, 0, NULL}, > {EDU_EXEC, m_NCS_EDP_SACLMNODEIDT, 0, 0, 0, > (long)&((AVSV_DND_MSG > *)0)->msg_info.d2n_compcsi_assign_msg_info.node_id, 0, NULL}, > diff --git a/osaf/libs/common/amf/d2nmsg.c b/osaf/libs/common/amf/d2nmsg.c > --- a/osaf/libs/common/amf/d2nmsg.c > +++ b/osaf/libs/common/amf/d2nmsg.c > @@ -496,6 +496,7 @@ > static void free_d2n_compcsi_info(AVSV_DND_MSG *compcsi_msg) > { > uint16_t i; > + // GL: please check indentation [Praveen] Will correct it before pushing. > AVSV_D2N_COMPCSI_ASSIGN_MSG_INFO *compcsi = > &compcsi_msg->msg_info.d2n_compcsi_assign_msg_info; > osaf_extended_name_free(&compcsi->comp_name); > osaf_extended_name_free(&compcsi->csi_name); > diff --git a/osaf/libs/common/amf/include/amf_d2nmsg.h > b/osaf/libs/common/amf/include/amf_d2nmsg.h > --- a/osaf/libs/common/amf/include/amf_d2nmsg.h > +++ b/osaf/libs/common/amf/include/amf_d2nmsg.h > @@ -639,6 +639,7 @@ > AVSV_COMPCSI_ACT msg_act; > SaNameT comp_name; > SaNameT csi_name; > + // GL: union required? [Praveen] This I intentionally did because as of now SUSI assign message is necessary overloaded for the handling of single csi assignment when the CSI is added dynamically. When a CSI is added dynamically su_si_assign message from AMFD lands in AMFND. In AMFND again all SUSI assignmets function are manipulated for single CSI assignment. Creating a separate message for COMP CSI assignment can be used for single csi assignments. Also as mentioned in README and updated in the ticket under the heading "Changes at AMFD (CSI Attribute Change Callback)" that such a message will help in tickets like #538 and #83 which required manipulation of assignments at compcsi level. We can add more message structure inside the union. > union { > AVSV_CSI_ATTRS attrs; > } info; > diff --git a/osaf/libs/common/amf/n2avamsg.c > b/osaf/libs/common/amf/n2avamsg.c > --- a/osaf/libs/common/amf/n2avamsg.c > +++ b/osaf/libs/common/amf/n2avamsg.c > @@ -356,6 +356,7 @@ > break; > case AVSV_AMF_CSI_ATTR_CHANGE: > osaf_extended_name_alloc(osaf_extended_name_borrow(&scbk->param.csi_attr_change.csi_name), > > + // GL: check indentation [Praveen]Will correct it before pushing. > &(*o_dcbk)->param.csi_attr_change.csi_name); > /* memset avsv & amf csi attr lists */ > memset(&(*o_dcbk)->param.csi_attr_change.attrs, 0, sizeof(AVSV_CSI_ATTRS)); > diff --git a/osaf/services/saf/amf/amfd/csi.cc > b/osaf/services/saf/amf/amfd/csi.cc > --- a/osaf/services/saf/amf/amfd/csi.cc > +++ b/osaf/services/saf/amf/amfd/csi.cc > @@ -622,6 +622,7 @@ > "Invalid modification of > osafAmfCSICommunicateCsiAttributeChange, valid (0 or 1)"); > goto done; > } > + // GL: double casting not required. Please check indentation. [Praveen] Will correct it before pushing. I missed removing C type. > uint32_t val = > static_cast<uint32_t>(*(uint32_t*)attr_mod->modAttr.attrValues[0]); > if ((val != true) && (val != false)) { > report_ccb_validation_error(opdata, > diff --git a/osaf/services/saf/amf/amfd/util.cc > b/osaf/services/saf/amf/amfd/util.cc > --- a/osaf/services/saf/amf/amfd/util.cc > +++ b/osaf/services/saf/amf/amfd/util.cc > @@ -1731,6 +1731,7 @@ > info->mem_list.numberOfItems = 0; > } > > +// GL: very similar version in d2nmsg.c. Also this isn't freeing the > SaNameTs attrs.list [Praveen] I did not get it completely. The one in utils.cc is used by AMFD to free the message inside avd_d2n_msg_dequeue() and other one that is present in d2nmsg.c is used by AMFND to free the message. In utils.cc: delete [] (compcsi->info.attrs.list) is already present. In d2nmsg.c: AMFND will have to free memory allocated for extended names names (if any) by leap. > static void free_d2n_compcsi_info(AVSV_DND_MSG *compcsi_msg) { > AVSV_D2N_COMPCSI_ASSIGN_MSG_INFO *compcsi = > &compcsi_msg->msg_info.d2n_compcsi_assign_msg_info; > > @@ -2030,6 +2031,7 @@ > /* Scan the list of attributes for the CSI and add it to the > message */ > while ((attr_ptr_db != nullptr) && > (ptr_csiattr_msg->number < compcsi->csi->num_attributes)) { > + // GL: AVSV_ATTR_NAME_VAL contains SaNameT. Does it need to > be deep copied. [Praveen] I got the same doubt while reviewing long dn patches because same mechamism is being done in avd_prep_csi_attr_info() for existing SUSI assign message. I thought it works because in case of long dn it will copy reference to the longdn and in case of short dn it will copy the original string. Since it is not freed in free_d2n_*() functions original referenced string in CSI is never gets corrupted. What do you think? > memcpy(i_ptr_msg, &attr_ptr_db->name_value, > sizeof(AVSV_ATTR_NAME_VAL)); > ptr_csiattr_msg->number++; > i_ptr_msg = i_ptr_msg + 1; > diff --git a/osaf/services/saf/amf/amfnd/cbq.cc > b/osaf/services/saf/amf/amfnd/cbq.cc > --- a/osaf/services/saf/amf/amfnd/cbq.cc > +++ b/osaf/services/saf/amf/amfnd/cbq.cc > @@ -408,6 +408,7 @@ > m_AVND_TMR_COMP_CBK_RESP_STOP(cb, *cbk_rec) > } > if (SA_AIS_OK != resp->err) { > + // GL: indentation Will correct it before pushing. > //generate a failure report. > err_info.src = > AVND_ERR_SRC_CBK_CSI_ATTR_CHANGE_FAILED; > err_info.rec_rcvr.avsv_ext = > static_cast<AVSV_ERR_RCVR>(comp->err_info.def_rec); > diff --git a/osaf/services/saf/amf/amfnd/su.cc > b/osaf/services/saf/amf/amfnd/su.cc > --- a/osaf/services/saf/amf/amfnd/su.cc > +++ b/osaf/services/saf/amf/amfnd/su.cc > @@ -920,6 +920,7 @@ > > switch (param->msg_act) { > case AVSV_COMPCSI_ATTR_CHANGE_AND_NO_ACK: { > + // GL: indentation Will correct it before pushing. > //Free ealrliar allocated memory by EDP utils. > if (csi_rec->attrs.list != nullptr) > free (csi_rec->attrs.list); > > > On 25/08/2016 2:35 PM, praveen malviya wrote: >> Hi All, >> >> Please provide your feedback on these patches. >> >> Thanks, >> Praveen >> >> On 16-Aug-16 11:20 AM, praveen.malv...@oracle.com wrote: >>> Summary: Support for CSI attribute change callback. [#1553] >>> Review request for Trac Ticket(s): #1553 >>> Peer Reviewer(s): AMF maintainers >>> Pull request to: <<LIST THE PERSON WITH PUSH ACCESS HERE>> >>> Affected branch(es): default(5.1) >>> Development branch: <<IF ANY GIVE THE REPO URL>> >>> >>> -------------------------------- >>> 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): >>> --------------------------------------------- >>> TODOs: >>> 1)Invocation of INSTANTIATE command for a Non Proxied NPI component. >>> 2)Introduce saAmf_B_04_02.h for new resources at agent. >>> 3)These patches are old (work done in 5.0), so >>> re-base over #1642(AMF long dn) and other C++ refactoring. >>> 4)These are old patches submitted in 5.0 and currently rebased for >>> 5.1. Refactor them for C++. >>> >>> changeset 3a558f178b570eb0cd1ba32d2312ad4a54d806d3 >>> Author: praveen.malv...@oracle.com >>> Date: Mon, 15 Aug 2016 20:13:21 +0530 >>> >>> amf: add README for implementation details [#1553] >>> >>> Contains information related to: >>> -details of implementation. >>> -changes at AMFD. >>> -changes at AMFND. >>> -changes at AMFA. >>> >>> changeset ab6e632ea3be607623f1ddaca50b55bbb0b96b51 >>> Author: praveen.malv...@oracle.com >>> Date: Mon, 15 Aug 2016 20:17:49 +0530 >>> >>> amf: update saAmf.h for new intialize API, callback and callback >>> structure >>> [#1553]. >>> >>> Added in saAmf.h: >>> -OsafCsiAttributeChangeCallbackT >>> -SaAmfCallbacksT_5() >>> -saAmfInitialize_5( SaAmfHandleT *amfHandle, const SaAmfCallbacksT_5 >>> *amfCallbacks, SaVersionT *version); TODO: Move >>> all this new resources in >>> saAmf_B_04_01. Details are in published README. >>> >>> changeset 8b41e99f3fb63f92fc77aa0c776c7f16fc5b030c >>> Author: praveen.malv...@oracle.com >>> Date: Mon, 15 Aug 2016 20:17:58 +0530 >>> >>> amf: add new D2ND message and new ND to Agent message. [#1553] >>> >>> -New message structure to be used by AMFD to send COMPCSI related >>> information to AMFND. >>> -New internal callback message AVSV_AMF_CSI_ATTR_CHANGE_PARAM >>> from AMFND to >>> agent to support newly introduced callback. >>> >>> changeset d6b7c8391c874932866aa0dcd65cb7de2d4df652 >>> Author: praveen.malv...@oracle.com >>> Date: Mon, 15 Aug 2016 20:18:07 +0530 >>> >>> amfd: send msg to AMFND upon modification of safCsiAttr [#1553] >>> >>> Contains: 1)Support for new attribute >>> osafAmfCSICommunicateCsiAttributeChange in class SaAmfCSI. 2)Upon >>> modification of CSI attribute value for a object of class >>> SaAmfCSIAttribute, >>> AMFD will send a message to AMFND with new list. For a NON >>> PROXIED NPI >>> component, message will not be sent if >>> osafAmfCSICommunicateCsiAttributeChange is false. 3)AMFD now also >>> maintains >>> MDS install version of all AMFNDs in std::map<SaClmNodeIdT, >>> MDS_SVC_PVT_SUB_PART_VER> nds_mds_ver_db. It will be updated >>> whenever AMFD >>> gets MDS_UP and MDS_DOWN for AMFND. Using this AMFD can decide >>> whether >>> message is meant for particular AMFND much before encode callback >>> given by >>> MDS. >>> >>> changeset 52a348f88ab0dca4f20ed0662e2fd7f6fc4f494d >>> Author: praveen.malv...@oracle.com >>> Date: Mon, 15 Aug 2016 20:18:18 +0530 >>> >>> amfnd: add support for csi attribute change callback at amfnd. >>> [#1553] >>> >>> Contains changes related to: 1)Upong receving CSI attribute list, >>> update own >>> databae with modified information. If compoent has registered >>> with new >>> callback OsafCsiAttributeChangeCallbackT, then send callback >>> OsafCsiAttributeChangeCallbackT to AMF agent. TODO: For Non >>> Proxied NPI >>> component run INSTANTIATE command. 2)AMFND now maintains MDS >>> install version >>> of AMF AGENTs in std::map<MDS_DEST, MDS_SVC_PVT_SUB_PART_VER> >>> agent_mds_ver_db. It will be updated whenever AMFND gets MDS_UP >>> and MDS_DOWN >>> for AMF Agent. 3)AMFND maintains now SAF version for each registered >>> component. AMFND will get it from Agent through existing component >>> registeration message. >>> >>> changeset 57e408c3ae5f8ccfa3a66f6645aebe83d52f7d7f >>> Author: praveen.malv...@oracle.com >>> Date: Mon, 15 Aug 2016 20:18:27 +0530 >>> >>> amfa: add support for csi attribute change callback at amfa. [#1553] >>> >>> 1)Because of different AMF callback struture correponding to each >>> of B.01.01 >>> B.04.01, this patch introduces an internal callback structure >>> OsafAmfCallbacksT. It consists of callback from each AMF version. >>> Callback >>> for each intialization needs to be maintained internally in >>> AVA_HDL_REC. >>> With this internal structure, same handle can used interanlly for >>> any >>> saAmfInitialize_<*> of AMF service. Also added utility functions >>> to copy >>> callbacks from AMF callbacks structure to this internal one. AMF >>> agent will >>> use OsafAmfCallbacksT internally instead of SaAmfCallbacksT_<*>. >>> 2)Implementation of saAmfInitialize_5() in ava_api.c. 3)Now >>> saAmfRegister() >>> also sends SAF version to AMFND. >>> >>> TODO:In saAmf_B_04_02, rename saAmfInitialize_5(0 to >>> saAmfInitialize_o4(). >>> >>> changeset 66a87c96e7230fef0c1586314dc9b60986cfca8e >>> Author: praveen.malv...@oracle.com >>> Date: Mon, 15 Aug 2016 20:18:43 +0530 >>> >>> samples/amf: update amf sa-aware demo for CSI attribute change >>> callback. >>> [#1553] >>> >>> >>> Complete diffstat: >>> ------------------ >>> osaf/libs/agents/saf/amfa/amf_agent.cc | 145 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++- >>> osaf/libs/agents/saf/amfa/ava_hdl.cc | 39 ++++++++----- >>> osaf/libs/agents/saf/amfa/ava_mds.cc | 54 >>> +++++++++++++++++++- >>> osaf/libs/agents/saf/amfa/ava_op.cc | 61 >>> +++++++++++++++++++++ >>> osaf/libs/agents/saf/amfa/include/ava_cb.h | 6 ++ >>> osaf/libs/agents/saf/amfa/include/ava_hdl.h | 22 +++++++- >>> osaf/libs/agents/saf/amfa/include/ava_mds.h | 4 +- >>> osaf/libs/common/amf/d2nedu.c | 19 ++++++- >>> osaf/libs/common/amf/d2nmsg.c | 13 ++++ >>> osaf/libs/common/amf/include/amf_amfparam.h | 10 +++ >>> osaf/libs/common/amf/include/amf_d2nmsg.h | 29 ++++++++++ >>> osaf/libs/common/amf/include/amf_n2avamsg.h | 3 + >>> osaf/libs/common/amf/n2avamsg.c | 29 +++++++++- >>> osaf/libs/saf/include/saAmf.h | 24 ++++++++ >>> osaf/services/saf/amf/README | 161 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> osaf/services/saf/amf/amfd/comp.cc | 18 ++++++ >>> osaf/services/saf/amf/amfd/csi.cc | 24 ++++++++ >>> osaf/services/saf/amf/amfd/csiattr.cc | 25 +++++++++ >>> osaf/services/saf/amf/amfd/include/comp.h | 2 + >>> osaf/services/saf/amf/amfd/include/csi.h | 2 + >>> osaf/services/saf/amf/amfd/include/mds.h | 4 +- >>> osaf/services/saf/amf/amfd/include/node.h | 2 +- >>> osaf/services/saf/amf/amfd/include/util.h | 2 + >>> osaf/services/saf/amf/amfd/mds.cc | 8 +- >>> osaf/services/saf/amf/amfd/node.cc | 3 + >>> osaf/services/saf/amf/amfd/util.cc | 97 >>> ++++++++++++++++++++++++++++++++++- >>> osaf/services/saf/amf/amfnd/amfnd.cc | 2 + >>> osaf/services/saf/amf/amfnd/cbq.cc | 23 +++++++- >>> osaf/services/saf/amf/amfnd/comp.cc | 42 +++++++++++++++ >>> osaf/services/saf/amf/amfnd/compdb.cc | 4 + >>> osaf/services/saf/amf/amfnd/err.cc | 4 +- >>> osaf/services/saf/amf/amfnd/evt.cc | 2 + >>> osaf/services/saf/amf/amfnd/include/avnd_cb.h | 2 + >>> osaf/services/saf/amf/amfnd/include/avnd_comp.h | 2 +- >>> osaf/services/saf/amf/amfnd/include/avnd_err.h | 2 + >>> osaf/services/saf/amf/amfnd/include/avnd_evt.h | 1 + >>> osaf/services/saf/amf/amfnd/include/avnd_mds.h | 6 +- >>> osaf/services/saf/amf/amfnd/include/avnd_su.h | 2 + >>> osaf/services/saf/amf/amfnd/main.cc | 1 + >>> osaf/services/saf/amf/amfnd/mds.cc | 56 >>> ++++++++++++++++++- >>> osaf/services/saf/amf/amfnd/su.cc | 150 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> osaf/services/saf/amf/amfnd/util.cc | 38 +++++++++++++- >>> osaf/services/saf/amf/config/amf_classes.xml | 6 ++ >>> samples/amf/sa_aware/AppConfig-2N.xml | 8 ++ >>> samples/amf/sa_aware/README | 2 + >>> samples/amf/sa_aware/amf_demo.c | 55 >>> +++++++++++++++---- >>> 46 files changed, 1158 insertions(+), 56 deletions(-) >>> >>> >>> Testing Commands: >>> ----------------- >>> Tested PI amfd demo app for: >>> 1)success case >>> 2)when comp-fails during callback processing. >>> >>> >>> Testing, Expected Results: >>> -------------------------- >>> PASS >>> >>> Conditions of Submission: >>> ------------------------- >>> Ack from reviewers before 5.1 FC >>> >>> 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. >>> >>> >>> ------------------------------------------------------------------------------ >>> >>> _______________________________________________ >>> Opensaf-devel mailing list >>> Opensaf-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/opensaf-devel >>> > ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel