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
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
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
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
(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
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?
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
&(*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.
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
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.
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
//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
//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
