Hi
Two minor comments inlined.
praveen malviya wrote:
> Hi,
> One more change that can be brought into this patch. Ccb modification of
> saAmfSgtDefAutoRepair in SaAmfSGType is allowed only in the context of
> sufailover which is currently implemented for only 2N and NoRed. So for
> other red models modify operation should be rejected like this:
>
> diff --git a/osaf/services/saf/avsv/avd/avd_sgtype.cc
> b/osaf/services/saf/avsv/avd/avd_sgtype.cc
> --- a/osaf/services/saf/avsv/avd/avd_sgtype.cc
> +++ b/osaf/services/saf/avsv/avd/avd_sgtype.cc
> @@ -338,6 +338,12 @@ static SaAisErrorT sgtype_ccb_completed_
> continue;
>
> if (!strcmp(attr_mod->modAttr.attrName,
> "saAmfSgtDefAutoRepair")) {
> + if ((sgt->saAmfSgtRedundancyModel !=
> SA_AMF_2N_REDUNDANCY_MODEL) &&
> + (sgt->saAmfSgtRedundancyModel != SA_AMF_NO_REDUNDANCY_MODEL)) {
> + rc = SA_AIS_ERR_BAD_OPERATION;
> + LOG_ER("Modification of
> saAmfSgtDefAutoRepair supported for 2N and NoRed model only");
>
(1)
This would be a user error so the log severity should not be ERor but
NOtice.
(2)
If you are ambitious you could add a return error string, which
could/should reach the
end users tool/interface. For example, immcfg would pick it up and
display it in conjunction
with printing the error code.
Like this:
saImmOiCcbSetErrorString(immOiHandle, ccbId,
"Modification of saAmfSgtDefAutoRepair supported for 2N
and NoRed model only");
Otherwise the user will simply get the error code BAD_OPERATION and
most likely not have
a clue what the problem was.
Anders Bjornerstedt
> + goto done;
> + }
>
> uint32_t sgt_autorepair = *((SaUint32T
> *)attr_mod->modAttr.attrValues[0]);
> if (sgt_autorepair > SA_TRUE) {
>
> Thanks,
> Praveen
> On 28-Jun-13 11:51 AM, [email protected] wrote:
>
>> osaf/services/saf/avsv/avd/avd_sg.cc | 16 +++-
>> osaf/services/saf/avsv/avd/avd_sgtype.cc | 106
>> +++++++++++++++++++++++++++-
>> osaf/services/saf/avsv/avd/include/avd_sg.h | 3 +-
>> 3 files changed, 119 insertions(+), 6 deletions(-)
>>
>>
>> Refloating the patch after incorporating comments.
>>
>> diff --git a/osaf/services/saf/avsv/avd/avd_sg.cc
>> b/osaf/services/saf/avsv/avd/avd_sg.cc
>> --- a/osaf/services/saf/avsv/avd/avd_sg.cc
>> +++ b/osaf/services/saf/avsv/avd/avd_sg.cc
>> @@ -293,7 +293,10 @@ static AVD_SG *sg_create(const SaNameT *
>>
>> if (immutil_getAttr(const_cast<SaImmAttrNameT>("saAmfSGAutoRepair"),
>> attributes, 0, &sg->saAmfSGAutoRepair) != SA_AIS_OK) {
>> sg->saAmfSGAutoRepair = sgt->saAmfSgtDefAutoRepair;
>> + sg->saAmfSGAutoRepair_configured = false;
>> }
>> + else
>> + sg->saAmfSGAutoRepair_configured = true;
>>
>> if (immutil_getAttr(const_cast<SaImmAttrNameT>("saAmfSGAutoAdjust"),
>> attributes, 0, &sg->saAmfSGAutoAdjust) != SA_AIS_OK) {
>> sg->saAmfSGAutoAdjust = sgt->saAmfSgtDefAutoAdjust;
>> @@ -527,6 +530,12 @@ static SaAisErrorT ccb_completed_modify_
>> rc = SA_AIS_ERR_BAD_OPERATION;
>> goto done;
>> } else if (!strcmp(attribute->attrName,
>> "saAmfSGAutoRepair")) {
>> + uint32_t sg_autorepair = *((SaUint32T
>> *)attribute->attrValues[0]);
>> + if (sg_autorepair > SA_TRUE) {
>> + LOG_ER("Invalid saAmfSGAutoRepair
>> SG:'%s'", avd_sg->name.value);
>> + rc = SA_AIS_ERR_BAD_OPERATION;
>> + goto done;
>> + }
>> } else if (!strcmp(attribute->attrName,
>> "saAmfSGAutoAdjust")) {
>> } else if (!strcmp(attribute->attrName,
>> "saAmfSGNumPrefActiveSUs")) {
>> } else if (!strcmp(attribute->attrName,
>> "saAmfSGNumPrefStandbySUs")) {
>> @@ -744,10 +753,13 @@ static void ccb_apply_modify_hdlr(CcbUti
>> sg_nd_attribute_update(sg,
>> saAmfSGCompRestartProb_ID);
>> sg_nd_attribute_update(sg,
>> saAmfSGCompRestartMax_ID);
>> } else if (!strcmp(attribute->attrName,
>> "saAmfSGAutoRepair")) {
>> - if (value_is_deleted)
>> + if (value_is_deleted) {
>> sg->saAmfSGAutoRepair =
>> sg_type->saAmfSgtDefAutoRepair;
>> - else
>> + sg->saAmfSGAutoRepair_configured =
>> false;
>> + } else {
>> sg->saAmfSGAutoRepair =
>> static_cast<SaBoolT>(*((SaUint32T *)value));
>> + sg->saAmfSGAutoRepair_configured = true;
>> + }
>> } else if (!strcmp(attribute->attrName,
>> "saAmfSGAutoAdjust")) {
>> if (value_is_deleted)
>> sg->saAmfSGAutoAdjust =
>> sg_type->saAmfSgtDefAutoAdjust;
>> diff --git a/osaf/services/saf/avsv/avd/avd_sgtype.cc
>> b/osaf/services/saf/avsv/avd/avd_sgtype.cc
>> --- a/osaf/services/saf/avsv/avd/avd_sgtype.cc
>> +++ b/osaf/services/saf/avsv/avd/avd_sgtype.cc
>> @@ -221,7 +221,10 @@ static AVD_AMF_SG_TYPE *sgtype_create(Sa
>>
>> if
>> (immutil_getAttr(const_cast<SaImmAttrNameT>("saAmfSgtDefAutoRepair"),
>> attributes, 0, &sgt->saAmfSgtDefAutoRepair) != SA_AIS_OK) {
>> sgt->saAmfSgtDefAutoRepair = SA_TRUE;
>> + sgt->saAmfSgtDefAutoRepair_configured = false;
>> }
>> + else
>> + sgt->saAmfSgtDefAutoRepair_configured = true;
>>
>> if
>> (immutil_getAttr(const_cast<SaImmAttrNameT>("saAmfSgtDefAutoAdjust"),
>> attributes, 0, &sgt->saAmfSgtDefAutoAdjust) != SA_AIS_OK) {
>> sgt->saAmfSgtDefAutoAdjust = SA_FALSE;
>> @@ -314,6 +317,56 @@ SaAisErrorT avd_sgtype_config_get(void)
>> return error;
>> }
>>
>> +
>> +/**
>> + * This routine validates modify CCB operations on SaAmfSGType objects.
>> + * @param Ccb Util Oper Data
>> + * @return
>> + */
>> +
>> +static SaAisErrorT sgtype_ccb_completed_modify_hdlr(CcbUtilOperationData_t
>> *opdata)
>> +{
>> + SaAisErrorT rc = SA_AIS_OK;
>> + const SaImmAttrModificationT_2 *attr_mod;
>> + int i = 0;
>> + AVD_AMF_SG_TYPE *sgt = avd_sgtype_get(&opdata->objectName);
>> +
>> + TRACE_ENTER2("CCB ID %llu, '%s'", opdata->ccbId,
>> opdata->objectName.value);
>> + while ((attr_mod = opdata->param.modify.attrMods[i++]) != NULL) {
>> +
>> + if ((attr_mod->modType == SA_IMM_ATTR_VALUES_DELETE) ||
>> (attr_mod->modAttr.attrValues == NULL))
>> + continue;
>> +
>> + if (!strcmp(attr_mod->modAttr.attrName,
>> "saAmfSgtDefAutoRepair")) {
>> +
>> + uint32_t sgt_autorepair = *((SaUint32T
>> *)attr_mod->modAttr.attrValues[0]);
>> + if (sgt_autorepair > SA_TRUE) {
>> + LOG_ER("invalid saAmfSgtDefAutoRepair in:'%s'",
>> sgt->name.value);
>> + rc = SA_AIS_ERR_BAD_OPERATION;
>> + goto done;
>> + }
>> +
>> + AVD_SG *sg;
>> + for (sg = sgt->list_of_sg; sg; sg =
>> sg->sg_list_sg_type_next) {
>> + if (sg->saAmfSGAutoRepair_configured)
>> + continue;
>> + if (sg->saAmfSGAdminState !=
>> SA_AMF_ADMIN_LOCKED_INSTANTIATION) {
>> + rc = SA_AIS_ERR_BAD_OPERATION;
>> + LOG_WA("SG state is not in Lock-in
>> state:'%s'",sg->name.value);
>> + goto done;
>> + }
>> + }
>> + } else {
>> + LOG_ER("Modification of attribute is Not Supported:'%s'
>> ", attr_mod->modAttr.attrName);
>> + rc = SA_AIS_ERR_BAD_OPERATION;
>> + goto done;
>> + }
>> + }
>> +
>> +done:
>> + return rc;
>> +}
>> +
>> /**
>> * Validate SaAmfSGType CCB completed
>> * @param opdata
>> @@ -336,8 +389,7 @@ static SaAisErrorT sgtype_ccb_completed_
>> rc = SA_AIS_OK;
>> break;
>> case CCBUTIL_MODIFY:
>> - LOG_ER("Modification of SaAmfSGType not supported");
>> - goto done;
>> + rc = sgtype_ccb_completed_modify_hdlr(opdata);
>> break;
>> case CCBUTIL_DELETE:
>> sgt = avd_sgtype_get(&opdata->objectName);
>> @@ -373,6 +425,52 @@ done:
>> }
>>
>> /**
>> + * This function handles modify operations on SaAmfSGType objects.
>> + * @param ptr to opdata
>> + */
>> +static void sgtype_ccb_apply_modify_hdlr(struct CcbUtilOperationData
>> *opdata)
>> +{
>> + const SaImmAttrModificationT_2 *attr_mod;
>> + int i = 0;
>> +
>> + TRACE_ENTER2("CCB ID %llu, '%s'", opdata->ccbId,
>> opdata->objectName.value);
>> +
>> + AVD_AMF_SG_TYPE *sgt = avd_sgtype_get(&opdata->objectName);
>> +
>> + while ((attr_mod = opdata->param.modify.attrMods[i++]) != NULL) {
>> + bool value_is_deleted;
>> + if ((attr_mod->modType == SA_IMM_ATTR_VALUES_DELETE) ||
>> (attr_mod->modAttr.attrValues == NULL))
>> + /* Attribute value is deleted, revert to default value
>> */
>> + value_is_deleted = true;
>> + else
>> + /* Attribute value is modified */
>> + value_is_deleted = false;
>> +
>> + if (!strcmp(attr_mod->modAttr.attrName,
>> "saAmfSgtDefAutoRepair")) {
>> + SaBoolT old_value = sgt->saAmfSgtDefAutoRepair;
>> + if (value_is_deleted) {
>> + sgt->saAmfSgtDefAutoRepair =
>> static_cast<SaBoolT>(true);
>> + sgt->saAmfSgtDefAutoRepair_configured = false;
>> + } else {
>> + sgt->saAmfSgtDefAutoRepair =
>> static_cast<SaBoolT>(*((SaUint32T *)attr_mod->modAttr.attrValues[0]));
>> + sgt->saAmfSgtDefAutoRepair_configured = true;
>> + }
>> + /* Modify saAmfSGAutoRepair for SGs which had inherited
>> saAmfSgtDefAutoRepair.*/
>> + if (old_value != sgt->saAmfSgtDefAutoRepair) {
>> + AVD_SG *sg;
>> + for (sg = sgt->list_of_sg; sg; sg =
>> sg->sg_list_sg_type_next) {
>> + if (!sg->saAmfSGAutoRepair_configured)
>> + sg->saAmfSGAutoRepair =
>> static_cast<SaBoolT>(sgt->saAmfSgtDefAutoRepair);
>> + }
>> + }
>> + }
>> + }
>> +
>> + TRACE_LEAVE();
>> +}
>> +
>> +
>> +/**
>> * Apply SaAmfSGType CCB changes
>> * @param opdata
>> *
>> @@ -390,6 +488,9 @@ static void sgtype_ccb_apply_cb(CcbUtilO
>> osafassert(sgt);
>> sgtype_add_to_model(sgt);
>> break;
>> + case CCBUTIL_MODIFY:
>> + sgtype_ccb_apply_modify_hdlr(opdata);
>> + break;
>> case CCBUTIL_DELETE:
>> sgtype_delete(static_cast<AVD_AMF_SG_TYPE*>(opdata->userData));
>> break;
>> @@ -410,4 +511,3 @@ void avd_sgtype_constructor(void)
>> avd_class_impl_set(const_cast<SaImmClassNameT>("SaAmfSGType"), NULL,
>> NULL, sgtype_ccb_completed_cb, sgtype_ccb_apply_cb);
>> avd_class_impl_set(const_cast<SaImmClassNameT>("SaAmfSGBaseType"),
>> NULL, NULL, avd_imm_default_OK_completed_cb, NULL);
>> }
>> -
>> diff --git a/osaf/services/saf/avsv/avd/include/avd_sg.h
>> b/osaf/services/saf/avsv/avd/include/avd_sg.h
>> --- a/osaf/services/saf/avsv/avd/include/avd_sg.h
>> +++ b/osaf/services/saf/avsv/avd/include/avd_sg.h
>> @@ -69,6 +69,7 @@ typedef struct avd_sg_tag {
>> * Checkpointing - Sent as a one time update.
>> */
>>
>> + bool saAmfSGAutoRepair_configured; /* True when user configures
>> saAmfSGAutoRepair else false */
>> /******************** B.04 model
>> *************************************************/
>> SaNameT saAmfSGType; /* Network order. */
>> SaNameT saAmfSGSuHostNodeGroup; /* Network order. */
>> @@ -242,7 +243,7 @@ typedef struct avd_sg_tag {
>> typedef struct avd_amf_sg_type_tag {
>> NCS_PATRICIA_NODE tree_node; /* key will be sg type name */
>> SaNameT name;
>> -
>> + bool saAmfSgtDefAutoRepair_configured; /* True when user configures
>> saAmfSGDefAutoRepair else false */
>> /******************** B.04 model
>> *************************************************/
>> SaNameT *saAmfSGtValidSuTypes; /* array of DNs, size in number_su_type
>> */
>> SaAmfRedundancyModelT saAmfSgtRedundancyModel;
>>
>> ------------------------------------------------------------------------------
>> This SF.net email is sponsored by Windows:
>>
>> Build for Windows Store.
>>
>> http://p.sf.net/sfu/windows-dev2dev
>> _______________________________________________
>> Opensaf-devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
>>
>
>
> ------------------------------------------------------------------------------
> This SF.net email is sponsored by Windows:
>
> Build for Windows Store.
>
> http://p.sf.net/sfu/windows-dev2dev
> _______________________________________________
> Opensaf-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
>
------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
