diff --git a/src/smf/smfd/SmfImmOperation.cc b/src/smf/smfd/SmfImmOperation.cc index a8e65c5fa..dfd6fc990 100644 --- a/src/smf/smfd/SmfImmOperation.cc +++ b/src/smf/smfd/SmfImmOperation.cc @@ -275,9 +275,8 @@ SaAisErrorT SmfImmDeleteOperation::PrepareRollback( if (saveAttribute == true) { if (attr->attrValuesNumber == 0) { - o_rollbackData->addAttrValue(attr->attrName, - smf_immTypeToString(attr->attrValueType), - "<_empty_>"); + // No need to re-create original attribute + continue; } else { for (unsigned int j = 0; j < attr->attrValuesNumber; j++) { o_rollbackData->addAttrValue( diff --git a/src/smf/smfd/SmfImmOperation.h b/src/smf/smfd/SmfImmOperation.h index fc855d462..d53414e9f 100644 --- a/src/smf/smfd/SmfImmOperation.h +++ b/src/smf/smfd/SmfImmOperation.h @@ -49,7 +49,12 @@ class SmfImmAttribute { // Note: More than one value can be added (multi-value) void AddAttributeValue(const std::string& i_value) { m_values.push_back(i_value); - attribute_.AddValue(i_value); + // [Lennart] Here it is Ok to handle SMF specific values like <_empty_> + // If a value is <_empty_> don't add it. Can be done in all places where + // a value is added to an attribute descriptor. Note that the legacy vector + // m_values still can contain <_empty_> if needed + if (i_value != "<_empty_>") + attribute_.AddValue(i_value); } // Note: This is the legacy function that returns a list of strings. Is kept diff --git a/src/smf/smfd/SmfRollback.cc b/src/smf/smfd/SmfRollback.cc index 5aedd6272..299a1cbda 100644 --- a/src/smf/smfd/SmfRollback.cc +++ b/src/smf/smfd/SmfRollback.cc @@ -408,7 +408,7 @@ SaAisErrorT SmfRollbackData::rollbackModifyOperation( SmfImmModifyOperation* immOp = new (std::nothrow) SmfImmModifyOperation(); if (immOp == NULL) { LOG_ER( - "SmfRollbackData::rollbackModifyOperation, could not create SmfImmCreateOperation"); + "SmfRollbackData::rollbackModifyOperation, could not create SmfImmModifyOperation"); return SA_AIS_ERR_FAILED_OPERATION; } diff --git a/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc b/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc index 44373cd17..e9dea7930 100644 --- a/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc +++ b/src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc @@ -284,12 +284,15 @@ int AddModifyToCcb(const SaImmCcbHandleT& ccb_handle, ais_error_ = ais_rc; break; } + } else { + // Unrecoverable Fail + LOG_NO("%s: AddObjectModifyToCcb() Fail, %s", __FUNCTION__, + saf_error(ais_rc)); + recovery_info = kFail; + api_name_ = "saImmOmCcbObjectModify_2"; + ais_error_ = ais_rc; + break; } - } else { - // Unrecoverable Fail - recovery_info = kFail; - api_name_ = "saImmOmCcbObjectModify_2"; - ais_error_ = ais_rc; } // Add Modify to CCB Success diff --git a/src/smf/smfd/imm_modify_config/attribute.cc b/src/smf/smfd/imm_modify_config/attribute.cc index 932911012..2f02e8ef0 100644 --- a/src/smf/smfd/imm_modify_config/attribute.cc +++ b/src/smf/smfd/imm_modify_config/attribute.cc @@ -88,10 +88,10 @@ static bool StringToNumericValue(const std::string& str_value, } } } catch(std::invalid_argument& e) { - LOG_NO("%s: Invalid argument", __FUNCTION__); + LOG_NO("%s Fail: Invalid argument", __FUNCTION__); rc = false; } catch(std::out_of_range& e) { - LOG_NO("%s: Out of range", __FUNCTION__); + LOG_NO("%s Fail: Out of range", __FUNCTION__); rc = false; } @@ -140,6 +140,9 @@ bool AttributeHandler::AddAttributesForModification(const ModifyDescriptor& rc = false; break; } + // break out of the 'for' loop if a 'switch case' fails + if (rc == false) + break; } TRACE_LEAVE(); @@ -231,10 +234,25 @@ StoreNumericAttribute(const AttributeDescriptor& attribute, Request request) { T numeric_value{0}; std::vector num_values; for (auto& value_str : attribute.values_as_strings) { + // [Lennart] This is the wrong place for handling any SMF specific oddities. + // Think of the code in imm_modify_config and imm_om_ccapi as 3PP which you + // are not allowed to adapt to SMF in any way. It must always be done the + // the other way around. If SMF uses <_empty_> as a tag for empty value it + // must be handled before adding the modify to an attribute descriptor. In + // this case the attribute descriptor shall contain an empty value vector. + // + // If the attribute value string is "<_empty_>", an empty + // set of values will be created. + // This is used to properly rollback an orginally empty + // attribute that got modified in the upgrade. + if (!value_str.compare("<_empty_>")) + break; + // Create a vector containing all values for this attribute if (StringToNumericValue (value_str, numeric_value, value_type) == false) { - LOG_NO("%s: StringToNumericValue() Fail", __FUNCTION__); + LOG_NO("%s Fail: name=%s, value=%s", __FUNCTION__, + attribute.attribute_name.c_str(), value_str.c_str()); rc = false; break; } @@ -263,6 +281,14 @@ StoreSaNametAttribute(const AttributeDescriptor& attribute, Request request) { SaNameT name_value; std::vector name_values; for (auto& value_str : attribute.values_as_strings) { + // [Lennart] Not allowed here. See my previous comment + // If the attribute value string is "<_empty_>", an empty + // set of values will be created. + // This is used to properly rollback an orginally empty + // attribute that got modified in the upgrade. + if (!value_str.compare("<_empty_>")) + break; + StringToSaNameT(value_str, &name_value); name_values.push_back(name_value); } @@ -288,6 +314,14 @@ StoreSaAnytAttribute(const AttributeDescriptor& attribute, Request request) { SaAnyT any_value; std::vector any_values; for (auto& value_str : attribute.values_as_strings) { + // [Lennart] Not allowed here. See previous comments + // If the attribute value string is "<_empty_>", an empty + // set of values will be created. + // This is used to properly rollback an orginally empty + // attribute that got modified in the upgrade. + if (!value_str.compare("<_empty_>")) + break; + StringToSaAnyT(value_str, &any_value); any_values.push_back(any_value); } @@ -308,6 +342,20 @@ StoreSaAnytAttribute(const AttributeDescriptor& attribute, Request request) { void AttributeHandler:: StoreStringAttribute(const AttributeDescriptor& attribute, Request request) { TRACE_ENTER(); + + // [Lennart] Not allowed here. See previous comments + std::vector str_values(attribute.values_as_strings); + for (auto& value_str : attribute.values_as_strings) { + // If the attribute value string is "<_empty_>", an empty + // set of values will be created. + // This is used to properly rollback an orginally empty + // attribute that got modified in the upgrade. + if (!value_str.compare("<_empty_>")) { + str_values.clear(); + break; + } + } + // Store the attribute give it to an attribute setter std::unique_ptr AnAttribute; if (request == Request::kCreate) { @@ -316,8 +364,7 @@ StoreStringAttribute(const AttributeDescriptor& attribute, Request request) { AnAttribute = std::unique_ptr(new SetAttribute(modifier_, request)); } - AnAttribute->SetAttributeValues(attribute.attribute_name, - attribute.values_as_strings); + AnAttribute->SetAttributeValues(attribute.attribute_name, str_values); set_attributesp_.push_back(std::move(AnAttribute)); TRACE_LEAVE(); } diff --git a/src/smf/smfd/imm_modify_config/immccb.cc b/src/smf/smfd/imm_modify_config/immccb.cc index f945bdcb5..cc62911db 100644 --- a/src/smf/smfd/imm_modify_config/immccb.cc +++ b/src/smf/smfd/imm_modify_config/immccb.cc @@ -189,9 +189,9 @@ int ModelModification::CreateHandles() { if (recovery_info == kContinue) { recovery_info = CreateCcb(); if (recovery_info == kFail) { - LOG_NO("%s: CreateAdminOwner() Fail", __FUNCTION__); + LOG_NO("%s: CreateCcb() Fail", __FUNCTION__); } else if (recovery_info == kRestartOm) { - TRACE("%s: CreateAdminOwner() Restart", __FUNCTION__); + TRACE("%s: CreateCcb() Restart", __FUNCTION__); } } TRACE_LEAVE(); @@ -692,4 +692,4 @@ int ModelModification::ApplyModifications() { return recovery_info; } -} // namespace modelmodify \ No newline at end of file +} // namespace modelmodify