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;
>
------------------------------------------------------------------------------
"Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
Instantly run your Selenium tests across 300+ browser/OS combos.
Get unparalleled scalability from the best Selenium testing platform available
Simple to use. Nothing to install. Get started now for free."
http://p.sf.net/sfu/SauceLabs
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
