Hi, Agree with Nags. If completed callbacks respond error then the assert in apply can be left. Different return codes are returned. BADOP seems to be most used. Guess we should we align using it.
Missed asserts: - si_ccb_apply_modify_hdlr Thanks, Hans > -----Original Message----- > From: Nagendra Kumar [mailto:nagendr...@oracle.com] > Sent: den 16 maj 2014 08:28 > To: Gary Lee; Hans Feldt; Hans Nordebäck; Praveen Malviya > Cc: opensaf-devel@lists.sourceforge.net > Subject: RE: [PATCH 1 of 1] amfd: Remove asserts from validation routines > [#849] > > Hi Gary, > I understand that. But from user perspective, if Amf > returns OK for not supported attributes, then user will feel that the > operation has been successful, but actually operation was not done. > So, my suggestion is return BAD operation for those unsupported attributes. > This will go inline with Amf PR document. > > Thanks > -Nagu > > -----Original Message----- > > From: Gary Lee [mailto:gary....@dektech.com.au] > > Sent: 16 May 2014 11:48 > > To: Nagendra Kumar; hans.fe...@ericsson.com; > > hans.nordeb...@ericsson.com; Praveen Malviya > > Cc: opensaf-devel@lists.sourceforge.net > > Subject: Re: [PATCH 1 of 1] amfd: Remove asserts from validation routines > > [#849] > > > > Hi Nagu > > > > I decided to be less restrictive and ignore the unknown attributes, in case > > the > > AMF model is extended in the future. > > > > Eg. an attribute may be added that does not conflict with the current > > version, so > > an application designer may deploy the same configuration on multiple > > versions > > of OpenSAF. But perhaps that is not a valid use case. > > > > Thanks > > Gary > > > > On 15/05/14 22:10, Nagendra Kumar wrote: > > > Some comments inlined with Nagu .... > > > > > > Thanks > > > -Nagu > > >> -----Original Message----- > > >> From: Gary Lee [mailto:gary....@dektech.com.au] > > >> Sent: 08 May 2014 12:14 > > >> To: hans.fe...@ericsson.com; hans.nordeb...@ericsson.com; Nagendra > > >> Kumar; Praveen Malviya > > >> Cc: opensaf-devel@lists.sourceforge.net > > >> Subject: [PATCH 1 of 1] amfd: Remove asserts from validation routines > > >> [#849] > > >> > > >> osaf/services/saf/amf/amfd/app.cc | 18 +++++++++++------- > > >> osaf/services/saf/amf/amfd/comp.cc | 5 +++-- > > >> osaf/services/saf/amf/amfd/compcstype.cc | 5 +++-- > > >> osaf/services/saf/amf/amfd/hlt.cc | 10 ++++++---- > > >> osaf/services/saf/amf/amfd/sg.cc | 9 +++++---- > > >> osaf/services/saf/amf/amfd/si.cc | 5 +++-- > > >> osaf/services/saf/amf/amfd/su.cc | 5 +++-- > > >> 7 files changed, 34 insertions(+), 23 deletions(-) > > >> > > >> > > >> When an unknown attribute in encountered in various callbacks, > > >> sometimes an assert is called. > > >> In other cases, the operation is rejected. > > >> > > >> This may create forward compatibility issues if new attributes are > > >> added in the future. > > >> > > >> In this patch, the asserts are replaced with a warning and the > > >> corresponding attribute change will be permitted but ignored. > > >> > > >> diff --git a/osaf/services/saf/amf/amfd/app.cc > > >> b/osaf/services/saf/amf/amfd/app.cc > > >> --- a/osaf/services/saf/amf/amfd/app.cc > > >> +++ b/osaf/services/saf/amf/amfd/app.cc > > >> @@ -243,12 +243,15 @@ static SaAisErrorT app_ccb_completed_cb( > > >> SaNameT dn = *((SaNameT*)attribute- > > >>> attrValues[0]); > > >> if (NULL == avd_apptype_get(&dn)) { > > >> > > >> report_ccb_validation_error(opdata, > > >> "saAmfAppType '%s' not found", dn.value); > > >> + rc = SA_AIS_ERR_BAD_OPERATION; > > >> goto done; > > >> } > > >> rc = SA_AIS_OK; > > >> break; > > >> - } else > > >> - osafassert(0); > > >> + } else { > > >> + LOG_WA("Ignoring unknown attribute > > >> '%s'", > > >> attribute->attrName); > > >> + rc = SA_AIS_OK; > > >> + } > > > [Nagu]:: Returning ok for bad attribute may not be appropriate.BAD_OP can > > be returned. > > > > > >> } > > >> break; > > >> case CCBUTIL_DELETE: > > >> @@ -294,10 +297,10 @@ static void app_ccb_apply_cb(CcbUtilOper > > >> app->saAmfAppType = > > >> *((SaNameT*)attribute->attrValues[0]); > > >> app->app_type = avd_apptype_get(&app- > > >>> saAmfAppType); > > >> avd_apptype_add_app(app); > > >> - break; > > >> } > > >> - else > > >> - osafassert(0); > > >> + else { > > >> + TRACE("Ignoring unknown attribute '%s'", > > >> attribute->attrName); > > >> + } > > > [Nagu]: If above comments are accepted, then this check is not required. > > >> } > > >> break; > > >> } > > >> @@ -396,8 +399,9 @@ static SaAisErrorT app_rt_attr_cb(SaImmO > > >> if (!strcmp(attributeName, > > >> "saAmfApplicationCurrNumSGs")) { > > >> avd_saImmOiRtObjectUpdate_sync(objectName, > > >> attributeName, > > >> SA_IMM_ATTR_SAUINT32T, &app- > > >>> saAmfApplicationCurrNumSGs); > > >> - } else > > >> - osafassert(0); > > >> + } else { > > >> + LOG_WA("Ignoring unknown attribute '%s'", > > >> attributeName); > > >> + } > > >> } > > >> > > >> return SA_AIS_OK; > > >> diff --git a/osaf/services/saf/amf/amfd/comp.cc > > >> b/osaf/services/saf/amf/amfd/comp.cc > > >> --- a/osaf/services/saf/amf/amfd/comp.cc > > >> +++ b/osaf/services/saf/amf/amfd/comp.cc > > >> @@ -830,8 +830,9 @@ static SaAisErrorT comp_rt_attr_cb(SaImm > > >> /* TODO */ > > >> } else if (!strcmp("saAmfCompCurrProxiedNames", > > >> attributeName)) { > > >> /* TODO */ > > >> - } else > > >> - osafassert(0); > > >> + } else { > > >> + LOG_WA("Ignoring unknown attribute '%s'", > > >> attributeName); > > >> + } > > >> } > > >> > > >> return SA_AIS_OK; > > >> diff --git a/osaf/services/saf/amf/amfd/compcstype.cc > > >> b/osaf/services/saf/amf/amfd/compcstype.cc > > >> --- a/osaf/services/saf/amf/amfd/compcstype.cc > > >> +++ b/osaf/services/saf/amf/amfd/compcstype.cc > > >> @@ -435,8 +435,9 @@ static SaAisErrorT compcstype_rt_attr_ca > > >> SA_IMM_ATTR_SAUINT32T, &cst- > > >>> saAmfCompNumCurrStandbyCSIs); > > >> } else if (!strcmp("saAmfCompAssignedCsi", > > >> attributeName)) { > > >> /* TODO */ > > >> - } else > > >> - osafassert(0); > > >> + } else { > > >> + LOG_WA("Ignoring unknown attribute '%s'", > > >> attributeName); > > >> + } > > > [Nagu]: Same comment here. > > > > > >> } > > >> > > >> return SA_AIS_OK; > > >> diff --git a/osaf/services/saf/amf/amfd/hlt.cc > > >> b/osaf/services/saf/amf/amfd/hlt.cc > > >> --- a/osaf/services/saf/amf/amfd/hlt.cc > > >> +++ b/osaf/services/saf/amf/amfd/hlt.cc > > >> @@ -48,8 +48,9 @@ static SaAisErrorT ccb_completed_modify_ > > >> *value, opdata- > > >>> objectName.value); > > >> return SA_AIS_ERR_BAD_OPERATION; > > >> } > > >> - } else > > >> - osafassert(0); > > >> + } else { > > >> + LOG_WA("Ignoring unknown attribute '%s'", > > >> attribute- > > >>> attrName); > > >> + } > > > [Nagu]: Same comment here. > > >> } > > >> > > >> return SA_AIS_OK; > > >> @@ -144,8 +145,9 @@ static void ccb_apply_modify_hdlr(CcbUti > > >> TRACE("saAmfHealthcheckMaxDuration modified to > > '%llu' for > > >> Comp'%s'", *param_val, > > >> comp->comp_info.name.value); > > >> param.attr_id = saAmfHealthcheckMaxDuration_ID; > > >> - } else > > >> - osafassert(0); > > >> + } else { > > >> + TRACE("Ignoring unknown attribute '%s'", > > >> attribute- > > >>> attrName); > > >> + } > > > [Nagu]: Same comment here. > > > > > >> avd_snd_op_req_msg(avd_cb, comp->su->su_on_node, > > ¶m); > > >> } > > >> diff --git a/osaf/services/saf/amf/amfd/sg.cc > > >> b/osaf/services/saf/amf/amfd/sg.cc > > >> --- a/osaf/services/saf/amf/amfd/sg.cc > > >> +++ b/osaf/services/saf/amf/amfd/sg.cc > > >> @@ -627,7 +627,7 @@ static SaAisErrorT ccb_completed_modify_ > > >> goto done; > > >> } > > >> } else { > > >> - osafassert(0); > > >> + LOG_WA("Ignoring unknown attribute > > >> '%s'", > > >> attribute->attrName); > > > [Nagu]: Same comment here. > > > > > >> } > > >> } /* while (attr_mod != NULL) */ > > >> > > >> @@ -933,7 +933,7 @@ static void ccb_apply_modify_hdlr(CcbUti > > >> } else if (!strcmp(attribute->attrName, > > >> "saAmfSGSuHostNodeGroup")) { > > >> sg->saAmfSGSuHostNodeGroup = *((SaNameT > > *)value); > > >> } else { > > >> - osafassert(0); > > >> + TRACE("Ignoring unknown attribute '%s'", > > >> attribute->attrName); > > >> } > > >> > > >> attr_mod = opdata->param.modify.attrMods[i++]; > > >> @@ -1322,8 +1322,9 @@ static SaAisErrorT sg_rt_attr_cb(SaImmOi > > >> } else if (!strcmp("saAmfSGNumCurrInstantiatedSpareSUs", > > >> attributeName)) { > > >> avd_saImmOiRtObjectUpdate_sync(objectName, > > >> attributeName, > > >> SA_IMM_ATTR_SAUINT32T, &sg- > > >>> saAmfSGNumCurrInstantiatedSpareSUs); > > >> - } else > > >> - osafassert(0); > > >> + } else { > > >> + LOG_WA("Ignoring unknown attribute '%s'", > > >> attributeName); > > >> + } > > >> } > > >> > > >> return SA_AIS_OK; > > >> diff --git a/osaf/services/saf/amf/amfd/si.cc > > >> b/osaf/services/saf/amf/amfd/si.cc > > >> --- a/osaf/services/saf/amf/amfd/si.cc > > >> +++ b/osaf/services/saf/amf/amfd/si.cc > > >> @@ -973,8 +973,9 @@ static SaAisErrorT si_rt_attr_cb(SaImmOi > > >> } else if (!strcmp("saAmfSINumCurrStandbyAssignments", > > >> attributeName)) { > > >> avd_saImmOiRtObjectUpdate_sync(objectName, > > >> attributeName, > > >> SA_IMM_ATTR_SAUINT32T, &si- > > >>> saAmfSINumCurrStandbyAssignments); > > >> - } else > > >> - osafassert(0); > > >> + } else { > > >> + LOG_WA("Ignoring unknown attribute '%s'", > > >> attributeName); > > >> + } > > >> } > > >> > > >> return SA_AIS_OK; > > >> diff --git a/osaf/services/saf/amf/amfd/su.cc > > >> b/osaf/services/saf/amf/amfd/su.cc > > >> --- a/osaf/services/saf/amf/amfd/su.cc > > >> +++ b/osaf/services/saf/amf/amfd/su.cc > > >> @@ -1197,8 +1197,9 @@ static SaAisErrorT su_rt_attr_cb(SaImmOi > > >> } else if (!strcmp("saAmfSURestartCount", > > >> attributeName)) { > > >> avd_saImmOiRtObjectUpdate_sync(objectName, > > >> attributeName, > > >> SA_IMM_ATTR_SAUINT32T, &su- > > >>> saAmfSURestartCount); > > >> - } else > > >> - osafassert(0); > > >> + } else { > > >> + LOG_WA("Ignoring unknown attribute '%s'", > > >> attributeName); > > >> + } > > >> } > > >> > > >> return SA_AIS_OK; > > ------------------------------------------------------------------------------ The best possible search technologies are now affordable for all companies. Download your FREE open source Enterprise Search Engine today! Our experts will assist you in its installation for $59/mo, no commitment. Test it for FREE on our Cloud platform anytime! http://pubads.g.doubleclick.net/gampad/clk?id=145328191&iu=/4140/ostg.clktrk _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
