Hi Lennart,
I've looked at your changes in the .diff file. They look good overall.
- Nevertheless, I think some updates, mostly regarding comments and
log/trace printouts, are needed to make the changes consistent. You can
find the places to update in the attached .diff file which can be
applied onto your review repository.
- Regarding renaming legacy function names in /SmfImmOperation.h/, you
missed to change two legacy functions of /SmfImmDeleteOperation/ class.
Please see the attached .diff file for details and double-check my
suggested update.
- About refactoring SMF TRY_AGAIN handling for IMM operations, I agree
that it should be done in a new ticket.
(I have removed the original patch from the mail thread to reduce the
message size)
Thanks,
Nguyen
On 5/14/2018 8:13 PM, Lennart Lund wrote:
Hi Nguyen,
Attached patch contains all fixes. Can be applied on the review request. I will
not send out a new review request unless Hans will find something that requires
significant changes.
This is how I have handled your comments:
SmfAdminState.cc
I have added most of your comments. Thanks for going through all comments and
finding the mismatches between comments and variable names etc. that was wrong.
A few of the suggested changes are not added.
SmfCampState.*
I have not changed the order of #include as suggested. The reason is that it
cannot easily be done since there are problems with .h files that does not have
all needed includes and circular dependencies. I tried to fix this in some
parts of the code but run into huge problems so I skipped that in the legacy
code.
SmfCampaignWrapup.cc
Your comment ...
// [Nguyen] smfCreateRollbackElement eventually calls
immutil_saImmOiRtObjectCreate_2()
// which already handles TRY_AGAIN. Should we remove the TRY_AGAIN handling
here?
// I've also looked at the smfCreateRollbackElement part of other campaign
stages and
// found no use of such doImmOpTimer.
... is correct regarding the try again loop. This "outer loop" adds a timed
loop with a timeout of 60 sec which is completely useless since the setting for immutil
set by smf is to always use 600 X 1000 ms which is not at all correct. However I will not
change any of this since it is not within scope of this ticket. Maybe create a new ticket
for this?
SmfImmOperation.h:
// [Nguyen] The naming of class methods in this file is not consistent,
// i.e some with capitalized initial letter, some with uncapitalized initial
letter
// Should we capitalize all initial letter for consistency?
// getClassName() -> GetClassName()
This again is legacy code. I have not renamed all legacy functions according to
Google style guide so there may be a mix in some classes. However this file is
fixed.
SmfUtils.cc:
lldtest_delete is test code that should have been removed. Fixed
Thanks
Lennart
-----Original Message-----
From: Nguyen Luu [mailto:nguyen.tk....@dektech.com.au]
Sent: den 10 maj 2018 09:46
To: Lennart Lund <lennart.l...@ericsson.com>; Hans Nordebäck
<hans.nordeb...@ericsson.com>
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [devel] [PATCH 1/1] smf: Add capability to redo CCBs that fail
[#1398]
Hi Lennart,
I've reviewed your patch. Code review only, not tested yet.
Please see my comments in the attached diff file which was generated on
top of the changes in your patch.
Regards,
Nguyen
---
src/smf/smfd/SmfAdminState.cc | 3 +--
src/smf/smfd/SmfCampState.cc | 4 +--
src/smf/smfd/SmfImmOperation.cc | 54 ++++++++++++++++++-------------------
src/smf/smfd/SmfImmOperation.h | 12 ++++-----
src/smf/smfd/SmfUpgradeProcedure.cc | 2 +-
5 files changed, 37 insertions(+), 38 deletions(-)
diff --git a/src/smf/smfd/SmfAdminState.cc b/src/smf/smfd/SmfAdminState.cc
index 3b8dbbf..0c0115b 100644
--- a/src/smf/smfd/SmfAdminState.cc
+++ b/src/smf/smfd/SmfAdminState.cc
@@ -520,7 +520,6 @@ bool
SmfAdminStateHandler::changeAdminState(SaAmfAdminStateT fromState,
}
// Set admin state for SUs
- // [Nguyen] just my suggestion to use suList_.empty()
if ((rc == true) && (!suList_.empty())) {
TRACE("%s: Use serialized for SUs", __FUNCTION__);
// Do only if setting admin state for nodes did not fail
@@ -728,7 +727,7 @@ done:
}
/// Create a SmfAdminStateHandler instance node group with the nodes in
-/// the nodeList.
+/// the nodeList_.
/// The saAmfNGAdminState attribute will be given fromState. The initState
/// must be set to a state so that a state change to the wanted state for the
/// nodes can be done.
diff --git a/src/smf/smfd/SmfCampState.cc b/src/smf/smfd/SmfCampState.cc
index 2e09f41..8f62fb0 100644
--- a/src/smf/smfd/SmfCampState.cc
+++ b/src/smf/smfd/SmfCampState.cc
@@ -145,13 +145,13 @@ SaAisErrorT SmfCampState::verify(SmfUpgradeCampaign
*i_camp,
}
//------------------------------------------------------------------------------
-// prerequsitescheck()
+// prerequisitescheck()
//------------------------------------------------------------------------------
SaAisErrorT SmfCampState::prerequisitescheck(SmfUpgradeCampaign *i_camp,
std::string &error) {
TRACE_ENTER();
LOG_ER(
- "SmfCampState::prerequsitescheck default implementation, should NEVER be
executed.");
+ "SmfCampState::prerequisitescheck default implementation, should NEVER
be executed.");
TRACE_LEAVE();
return SA_AIS_OK;
}
diff --git a/src/smf/smfd/SmfImmOperation.cc b/src/smf/smfd/SmfImmOperation.cc
index e5d9848..91d0b36 100644
--- a/src/smf/smfd/SmfImmOperation.cc
+++ b/src/smf/smfd/SmfImmOperation.cc
@@ -71,8 +71,8 @@ SaAisErrorT SmfImmCreateOperation::Execute(SmfRollbackData
*o_rollbackData) {
// Fill in a create descriptor. The create descriptor already declared in the
// inherited SmfImmOperation class shall be used
// Keep rollback handling "as is"
- // Note: This execute operation can no longer fail except when doing
- // prepareRollback()
+ // Note: This Execute() operation can no longer fail except when doing
+ // PrepareRollback()
//object_create_
// Verify that the create descriptor is filled in correctly,
@@ -95,7 +95,7 @@ SaAisErrorT SmfImmCreateOperation::Execute(SmfRollbackData
*o_rollbackData) {
SaAisErrorT rollbackResult;
if ((rollbackResult = this->PrepareRollback(o_rollbackData)) != SA_AIS_OK)
{
LOG_NO(
- "SmfImmCreateOperation::execute, Failed to prepare rollback data
rc=%s",
+ "SmfImmCreateOperation::Execute, Failed to prepare rollback data
rc=%s",
saf_error(rollbackResult));
result = SA_AIS_ERR_FAILED_OPERATION;
}
@@ -106,7 +106,7 @@ SaAisErrorT SmfImmCreateOperation::Execute(SmfRollbackData
*o_rollbackData) {
}
//------------------------------------------------------------------------------
-// prepareRollback()
+// PrepareRollback()
//------------------------------------------------------------------------------
SaAisErrorT SmfImmCreateOperation::PrepareRollback(
SmfRollbackData *o_rollbackData) {
@@ -114,7 +114,7 @@ SaAisErrorT SmfImmCreateOperation::PrepareRollback(
SaImmAttrDefinitionT_2 **attributeDefs = NULL;
if (immUtil.getClassDescription(class_name_, &attributeDefs) == false) {
- LOG_NO("SmfImmCreateOperation::prepareRollback, Could not find class %s",
+ LOG_NO("SmfImmCreateOperation::PrepareRollback, Could not find class %s",
class_name_.c_str());
return SA_AIS_ERR_FAILED_OPERATION;
}
@@ -132,7 +132,7 @@ SaAisErrorT SmfImmCreateOperation::PrepareRollback(
immUtil.classDescriptionMemoryFree(attributeDefs);
if (rdnAttrName.length() == 0) {
LOG_NO(
- "SmfImmCreateOperation::prepareRollback, could not find RDN attribute
in class %s",
+ "SmfImmCreateOperation::PrepareRollback, could not find RDN attribute
in class %s",
class_name_.c_str());
return SA_AIS_ERR_FAILED_OPERATION;
}
@@ -143,7 +143,7 @@ SaAisErrorT SmfImmCreateOperation::PrepareRollback(
if (rdnAttrName == (elem).m_name) {
if ((elem).m_values.size() != 1) {
LOG_NO(
- "SmfImmCreateOperation::prepareRollback, attribute %s contain %zu
values, must contain exactly one value",
+ "SmfImmCreateOperation::PrepareRollback, attribute %s contain %zu
values, must contain exactly one value",
rdnAttrName.c_str(), (elem).m_values.size());
return SA_AIS_ERR_FAILED_OPERATION;
}
@@ -153,12 +153,12 @@ SaAisErrorT SmfImmCreateOperation::PrepareRollback(
}
if (rdnAttrValue.length() == 0) {
LOG_NO(
- "SmfImmCreateOperation::prepareRollback, could not find RDN value for
%s, class %s",
+ "SmfImmCreateOperation::PrepareRollback, could not find RDN value for
%s, class %s",
rdnAttrName.c_str(), class_name_.c_str());
return SA_AIS_ERR_FAILED_OPERATION;
}
- TRACE("SmfImmCreateOperation::prepareRollback, Found RDN %s=%s",
+ TRACE("SmfImmCreateOperation::PrepareRollback, Found RDN %s=%s",
rdnAttrName.c_str(), rdnAttrValue.c_str());
/* Prepare deletion of created object at rollback */
@@ -191,9 +191,9 @@ SaAisErrorT SmfImmDeleteOperation::Execute(SmfRollbackData
*o_rollbackData) {
// Prepare a corresponding rollback operation
if (o_rollbackData != NULL) {
- if ((result = this->prepareRollback(o_rollbackData)) != SA_AIS_OK) {
+ if ((result = this->PrepareRollback(o_rollbackData)) != SA_AIS_OK) {
LOG_NO(
- "SmfImmDeleteOperation::execute, failed to prepare rollback data
rc=%s",
+ "SmfImmDeleteOperation::Execute, failed to prepare rollback data
rc=%s",
saf_error(result));
result = SA_AIS_ERR_FAILED_OPERATION;
}
@@ -203,7 +203,7 @@ SaAisErrorT SmfImmDeleteOperation::Execute(SmfRollbackData
*o_rollbackData) {
return result;
}
-SaAisErrorT SmfImmDeleteOperation::prepareRollback(
+SaAisErrorT SmfImmDeleteOperation::PrepareRollback(
SmfRollbackData *o_rollbackData) {
SmfImmUtils immUtil;
SaImmAttrDefinitionT_2 **attributeDefs;
@@ -221,13 +221,13 @@ SaAisErrorT SmfImmDeleteOperation::prepareRollback(
SA_IMM_ATTR_CLASS_NAME, 0);
if (className == NULL) {
LOG_NO(
- "SmfImmDeleteOperation::prepareRollback, could not find class name for
%s",
+ "SmfImmDeleteOperation::PrepareRollback, could not find class name for
%s",
m_dn.c_str());
return SA_AIS_ERR_FAILED_OPERATION;
}
if (immUtil.getClassDescription(className, &attributeDefs) == false) {
- LOG_NO("SmfImmDeleteOperation::prepareRollback, could not find class %s",
+ LOG_NO("SmfImmDeleteOperation::PrepareRollback, could not find class %s",
className);
return SA_AIS_ERR_FAILED_OPERATION;
}
@@ -356,7 +356,7 @@ SaAisErrorT SmfImmModifyOperation::Execute(SmfRollbackData
*o_rollbackData) {
if (o_rollbackData != NULL) {
if ((result = this->PrepareRollback(o_rollbackData)) != SA_AIS_OK) {
LOG_NO(
- "SmfImmModifyOperation::execute, failed to prepare rollback data %s",
+ "SmfImmModifyOperation::Execute, failed to prepare rollback data %s",
saf_error(result));
result = SA_AIS_ERR_FAILED_OPERATION;
}
@@ -455,7 +455,7 @@ bool SmfImmRTCreateOperation::CreateAttrValues(void) {
if (smf_stringToImmType((char *)(elem).m_type.c_str(),
attr->attrValueType) == false) {
LOG_NO(
- "SmfImmRTCreateOperation::createAttrValues, fail to convert attr "
+ "SmfImmRTCreateOperation::CreateAttrValues, fail to convert attr "
"[%s] type to valid IMM type", attr->attrName);
delete attr;
for (int k = 0; i > k; k++) {
@@ -474,7 +474,7 @@ bool SmfImmRTCreateOperation::CreateAttrValues(void) {
if (smf_stringsToValues(attr, (elem).m_values) ==
false) { // Convert the string to a SA Forum type
LOG_NO(
- "SmfImmRTCreateOperation::createAttrValues, fail to convert attr "
+ "SmfImmRTCreateOperation::CreateAttrValues, fail to convert attr "
"[%s] value string to attr value", attr->attrName);
free(attr->attrValues);
delete attr;
@@ -509,7 +509,7 @@ SaAisErrorT SmfImmRTCreateOperation::Execute() {
// Convert the strings to structures and types accepted by the IMM interface
if (this->CreateAttrValues() == false) {
LOG_NO(
- "SmfImmRTCreateOperation::execute, fail to convert to IMM attr "
+ "SmfImmRTCreateOperation::Execute, fail to convert to IMM attr "
"structure");
TRACE_LEAVE();
return SA_AIS_ERR_FAILED_OPERATION;
@@ -519,7 +519,7 @@ SaAisErrorT SmfImmRTCreateOperation::Execute() {
if (m_parentDn.length() > GetSmfMaxDnLength()) {
LOG_NO(
- "SmfImmRTCreateOperation::execute, createObject failed Too long "
+ "SmfImmRTCreateOperation::Execute, createObject failed Too long "
"parent name %zu",
m_parentDn.length());
TRACE_LEAVE();
@@ -527,7 +527,7 @@ SaAisErrorT SmfImmRTCreateOperation::Execute() {
}
if (!m_immAttrValues) {
- LOG_NO("SmfImmRTCreateOperation::execute, no SaImmAttrValuesT_2** is set");
+ LOG_NO("SmfImmRTCreateOperation::Execute, no SaImmAttrValuesT_2** is set");
TRACE_LEAVE();
return SA_AIS_ERR_UNAVAILABLE;
}
@@ -606,7 +606,7 @@ bool SmfImmRTUpdateOperation::CreateAttrMods(void) {
delete[] attributeModifications;
delete mod;
LOG_NO(
- "SmfImmRTUpdateOperation::createAttrMods, fail convert string to IMM
attr mod type [%s:%s]",
+ "SmfImmRTUpdateOperation::CreateAttrMods, fail convert string to IMM
attr mod type [%s:%s]",
m_dn.c_str(), (elem).m_name.c_str());
TRACE_LEAVE();
return false;
@@ -617,7 +617,7 @@ bool SmfImmRTUpdateOperation::CreateAttrMods(void) {
delete[] attributeModifications;
delete mod;
LOG_NO(
- "SmfImmRTUpdateOperation::createAttrMods, fail to convert string to
IMM attr type [%s:%s]",
+ "SmfImmRTUpdateOperation::CreateAttrMods, fail to convert string to
IMM attr type [%s:%s]",
m_dn.c_str(), (elem).m_name.c_str());
TRACE_LEAVE();
return false;
@@ -627,7 +627,7 @@ bool SmfImmRTUpdateOperation::CreateAttrMods(void) {
if ((elem).m_values.size() <= 0) { // Must have at least one value
LOG_NO(
- "SmfImmRTUpdateOperation::createAttrMods, No values %s:%s
(size=%zu)",
+ "SmfImmRTUpdateOperation::CreateAttrMods, No values %s:%s
(size=%zu)",
m_dn.c_str(), (elem).m_name.c_str(), (elem).m_values.size());
delete[] attributeModifications;
delete mod;
@@ -641,7 +641,7 @@ bool SmfImmRTUpdateOperation::CreateAttrMods(void) {
delete[] attributeModifications;
delete mod;
LOG_NO(
- "SmfImmRTUpdateOperation::createAttrMods, fail to conv string to
value [%s:%s = %s]",
+ "SmfImmRTUpdateOperation::CreateAttrMods, fail to conv string to
value [%s:%s = %s]",
m_dn.c_str(), (elem).m_name.c_str(),
(elem).m_values.front().c_str());
TRACE_LEAVE();
return false;
@@ -671,21 +671,21 @@ SaAisErrorT SmfImmRTUpdateOperation::Execute() {
// Convert the strings to structures and types accepted by the IMM interface
if (this->CreateAttrMods() == false) {
LOG_NO(
- "SmfImmRTUpdateOperation::execute, fail to convert to IMM attr
structure");
+ "SmfImmRTUpdateOperation::Execute, fail to convert to IMM attr
structure");
result = SA_AIS_ERR_FAILED_OPERATION;
goto exit;
}
if (m_dn.length() > GetSmfMaxDnLength()) {
LOG_NO(
- "SmfImmRTUpdateOperation::execute, too long DN [%zu], max=[%zu],
dn=[%s]",
+ "SmfImmRTUpdateOperation::Execute, too long DN [%zu], max=[%zu],
dn=[%s]",
m_dn.length(), static_cast<size_t>(GetSmfMaxDnLength()), m_dn.c_str());
result = SA_AIS_ERR_NAME_TOO_LONG;
goto exit;
}
if (m_immAttrMods == NULL) {
- LOG_NO("SmfImmRTUpdateOperation::execute, no SaImmAttrValuesT_2** is set");
+ LOG_NO("SmfImmRTUpdateOperation::Execute, no SaImmAttrValuesT_2** is set");
result = SA_AIS_ERR_UNAVAILABLE;
goto exit;
}
diff --git a/src/smf/smfd/SmfImmOperation.h b/src/smf/smfd/SmfImmOperation.h
index 5b5965c..fc855d4 100644
--- a/src/smf/smfd/SmfImmOperation.h
+++ b/src/smf/smfd/SmfImmOperation.h
@@ -217,12 +217,12 @@ class SmfImmCreateOperation : public SmfImmOperation {
///
SaAisErrorT PrepareRollback(SmfRollbackData* o_rollbackData);
- // Note: The following two variables are used in prepareRollback.
- // prepareRollback() could be refactored to use attributes_ instead
+ // Note: The following two variables are used in PrepareRollback.
+ // PrepareRollback() could be refactored to use attributes_ instead
std::string class_name_; /* class name for the object to be created */
std::string parent_dn_; /* dn to the parent object */
- // Note: Used in prepareRollback
+ // Note: Used in PrepareRollback
std::list<SmfImmAttribute> attributes_; /* Attributes set at creation */
};
@@ -266,7 +266,7 @@ class SmfImmDeleteOperation : public SmfImmOperation {
/// @param None
/// @return const std::string
///
- const std::string& getDn() { return object_delete_.object_name; }
+ const std::string& GetDn() { return object_delete_.object_name; }
private:
///
@@ -274,9 +274,9 @@ class SmfImmDeleteOperation : public SmfImmOperation {
/// @param None.
/// @return None.
///
- SaAisErrorT prepareRollback(SmfRollbackData* o_rollbackData);
+ SaAisErrorT PrepareRollback(SmfRollbackData* o_rollbackData);
- // Used in prepareRollback
+ // Used in PrepareRollback
std::string m_dn; /* dn to the object to delete */
};
diff --git a/src/smf/smfd/SmfUpgradeProcedure.cc
b/src/smf/smfd/SmfUpgradeProcedure.cc
index 908d5b3..f8d6d2f 100644
--- a/src/smf/smfd/SmfUpgradeProcedure.cc
+++ b/src/smf/smfd/SmfUpgradeProcedure.cc
@@ -4113,7 +4113,7 @@ bool SmfUpgradeProcedure::setEntitiesToAddRemMod(
return false;
}
} else if ((deleteOper = dynamic_cast<SmfImmDeleteOperation *>(*it)) != 0)
{
- io_smfEntityToAddRemove->AddAttributeValue(deleteOper->getDn());
+ io_smfEntityToAddRemove->AddAttributeValue(deleteOper->GetDn());
} else if ((modifyOper = dynamic_cast<SmfImmModifyOperation *>(*it)) != 0)
{
io_smfEntityToAddRemove->AddAttributeValue(modifyOper->GetObjectDn());
} else {
--
2.8.3
------------------------------------------------------------------------------
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
