Hi Nguyen,
Ack
Thanks
Lennart
> -----Original Message-----
> From: Nguyen Luu <nguyen.tk....@dektech.com.au>
> Sent: den 21 juni 2018 15:07
> To: Lennart Lund <lennart.l...@ericsson.com>
> Cc: opensaf-devel@lists.sourceforge.net; Nguyen Tran Khoi Luu
> <nguyen.tk....@dektech.com.au>
> Subject: [PATCH 1/1] smf: Fix failed rollback of DELETE/MODIFY CCB with
> originally empty attributes [#2877]
>
> Currently, if an upgrade DELETE or MODIFY CCB contains empty-value
> attributes, SMF will create rollback data for those attributes with
> the hard-coded "<_empty_>" string value. However, this hard-coded
> string will actually make the later rollback fail, especially for
> numeric-type attributes, when trying to convert "<_empty_>" to the
> real numeric type, which is considered an invalid conversion.
>
> This commit fixes that kind of rollback issue by:
> - Not re-create any originally empty-value attributes when rolling
> back DELETE CCBs.
> - Check for the hard-coded marker string "<_empty_>" and not add it
> to an attribute descriptor when rolling back empty attributes that
> got added with values by MODIFY CCBs in the upgrade.
>
> This commit also fixes a few semantic errors in the code, and improve
> some log/trace printouts.
> ---
> src/smf/smfd/SmfImmOperation.cc | 5 ++---
> src/smf/smfd/SmfImmOperation.h | 3 ++-
> src/smf/smfd/SmfRollback.cc | 2 +-
> src/smf/smfd/imm_modify_config/add_operation_to_ccb.cc | 13
> ++++++++-----
> src/smf/smfd/imm_modify_config/attribute.cc | 10 +++++++---
> src/smf/smfd/imm_modify_config/immccb.cc | 6 +++---
> 6 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/src/smf/smfd/SmfImmOperation.cc
> b/src/smf/smfd/SmfImmOperation.cc
> index a8e65c5..fc127fa 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 originally empty 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 fc855d4..bc91431 100644
> --- a/src/smf/smfd/SmfImmOperation.h
> +++ b/src/smf/smfd/SmfImmOperation.h
> @@ -49,7 +49,8 @@ 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);
> + if (i_value.compare("<_empty_>") != 0)
> + 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 5aedd62..299a1cb 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 44373cd..e9dea79 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 9329110..d31524f 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();
> @@ -234,7 +237,8 @@ StoreNumericAttribute(const AttributeDescriptor&
> attribute, Request request) {
> // Create a vector containing all values for this attribute
> if (StringToNumericValue<T>
> (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;
> }
> diff --git a/src/smf/smfd/imm_modify_config/immccb.cc
> b/src/smf/smfd/imm_modify_config/immccb.cc
> index f945bdc..cc62911 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
> --
> 2.7.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
